Skip to content

Commit

Permalink
Merge pull request #4983 from spicyj/id-swap
Browse files Browse the repository at this point in the history 
Refactor how composite type changes work, fix memory leak in ReactMount caching
  • Loading branch information
@sophiebits
sophiebits committed Oct 7, 2015
2 parents 34d84a3 + 8ce7b71 commit f31a46c
Show file tree
Hide file tree
Showing 12 changed files with 169 additions and 139 deletions.
14 changes: 0 additions & 14 deletions src/renderers/dom/client/ReactDOMIDOperations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,19 +63,6 @@ var ReactDOMIDOperations = {
}
},

/**
* Replaces a DOM node that exists in the document with markup.
*
* @param {string} id ID of child to be replaced.
* @param {string} markup Dangerous markup to inject in place of child.
* @internal
* @see {Danger.dangerouslyReplaceNodeWithMarkup}
*/
dangerouslyReplaceNodeWithMarkupByID: function(id, markup) {
var node = ReactMount.getNode(id);
DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup(node, markup);
},

/**
* Updates a component's children by processing a series of updates.
*
Expand All @@ -92,7 +79,6 @@ var ReactDOMIDOperations = {
};

ReactPerf.measureMethods(ReactDOMIDOperations, 'ReactDOMIDOperations', {
dangerouslyReplaceNodeWithMarkupByID: 'dangerouslyReplaceNodeWithMarkupByID',
dangerouslyProcessChildrenUpdates: 'dangerouslyProcessChildrenUpdates',
});

Expand Down
73 changes: 40 additions & 33 deletions src/renderers/dom/client/ReactMount.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ function getReactRootElementInContainer(container) {
*/
function getReactRootID(container) {
var rootElement = getReactRootElementInContainer(container);
return rootElement && ReactMount.getID(rootElement);
return rootElement && internalGetID(rootElement);
}

/**
Expand All @@ -116,20 +116,12 @@ function getReactRootID(container) {
function getID(node) {
var id = internalGetID(node);
if (id) {
if (nodeCache.hasOwnProperty(id)) {
var cached = nodeCache[id];
if (cached !== node) {
invariant(
!isValid(cached, id),
'ReactMount: Two valid but unequal nodes with the same `%s`: %s',
ATTR_NAME, id
);

nodeCache[id] = node;
}
} else {
nodeCache[id] = node;
}
invariant(
!nodeCache.hasOwnProperty(id) || nodeCache[id] === node,
'ReactMount: Two unequal nodes with the same `%s`: %s',
ATTR_NAME, id
);
nodeCache[id] = node;
}

return id;
Expand Down Expand Up @@ -157,6 +149,25 @@ function setID(node, id) {
nodeCache[id] = node;
}

/**
* Finds the node with the supplied ID if present in the cache.
*/
function getNodeIfCached(id) {
var node = nodeCache[id];
// TODO: Since this "isValid" business is now just a sanity check, we can
// probably drop it with no consequences.
invariant(
!node || isValid(node, id),
'ReactMount: Cached node with `%s`: %s is missing from the document. ' +
'This probably means the DOM was unexpectedly mutated -- when removing ' +
'React-rendered children from the DOM, rerender without those children ' +
'or call ReactDOM.unmountComponentAtNode on the container to unmount an ' +
'entire subtree.',
ATTR_NAME, id
);
return node;
}

/**
* Finds the node with the supplied React-generated DOM ID.
*
Expand All @@ -165,10 +176,12 @@ function setID(node, id) {
* @internal
*/
function getNode(id) {
if (!nodeCache.hasOwnProperty(id) || !isValid(nodeCache[id], id)) {
nodeCache[id] = ReactMount.findReactNodeByID(id);
var node = getNodeIfCached(id);
if (node) {
return node;
} else {
return nodeCache[id] = ReactMount.findReactNodeByID(id);
}
return nodeCache[id];
}

/**
Expand All @@ -183,10 +196,7 @@ function getNodeFromInstance(instance) {
if (ReactEmptyComponentRegistry.isNullComponentID(id)) {
return null;
}
if (!nodeCache.hasOwnProperty(id) || !isValid(nodeCache[id], id)) {
nodeCache[id] = ReactMount.findReactNodeByID(id);
}
return nodeCache[id];
return getNode(id);
}

/**
Expand Down Expand Up @@ -227,8 +237,8 @@ function purgeID(id) {

var deepestNodeSoFar = null;
function findDeepestCachedAncestorImpl(ancestorID) {
var ancestor = nodeCache[ancestorID];
if (ancestor && isValid(ancestor, ancestorID)) {
var ancestor = getNodeIfCached(ancestorID);
if (ancestor) {
deepestNodeSoFar = ancestor;
} else {
// This node isn't populated in the cache, so presumably none of its
Expand Down Expand Up @@ -724,19 +734,16 @@ var ReactMount = {
* @return {string} The "reactRoot" ID of elements rendered within.
*/
registerContainer: function(container) {
var reactRootID = getReactRootID(container);
if (reactRootID) {
// If one exists, make sure it is a valid "reactRoot" ID.
reactRootID = ReactInstanceHandles.getReactRootIDFromNodeID(reactRootID);
}
if (!reactRootID) {
var id = getReactRootID(container);
// If one exists, make sure it is a valid "reactRoot" ID.
if (!id || id !== ReactInstanceHandles.getReactRootIDFromNodeID(id)) {
// No valid "reactRoot" ID found, create one.
reactRootID = ReactInstanceHandles.createReactRootID(
id = ReactInstanceHandles.createReactRootID(
ClientReactRootIndex.createReactRootIndex()
);
}
containersByReactRootID[reactRootID] = container;
return reactRootID;
containersByReactRootID[id] = container;
return id;
},

/**
Expand Down
14 changes: 12 additions & 2 deletions src/renderers/dom/shared/ReactComponentBrowserEnvironment.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@

'use strict';

var DOMChildrenOperations = require('DOMChildrenOperations');
var ReactDOMIDOperations = require('ReactDOMIDOperations');
var ReactMount = require('ReactMount');
var ReactPerf = require('ReactPerf');

/**
* Abstracts away all functionality of the reconciler that requires knowledge of
Expand All @@ -24,8 +26,8 @@ var ReactComponentBrowserEnvironment = {
processChildrenUpdates:
ReactDOMIDOperations.dangerouslyProcessChildrenUpdates,

replaceNodeWithMarkupByID:
ReactDOMIDOperations.dangerouslyReplaceNodeWithMarkupByID,
replaceNodeWithMarkup:
DOMChildrenOperations.dangerouslyReplaceNodeWithMarkup,

/**
* If a particular environment requires that some resources be cleaned up,
Expand All @@ -40,4 +42,12 @@ var ReactComponentBrowserEnvironment = {

};

ReactPerf.measureMethods(
ReactComponentBrowserEnvironment,
'ReactComponentBrowserEnvironment',
{
replaceNodeWithMarkup: 'replaceNodeWithMarkup',
}
);

module.exports = ReactComponentBrowserEnvironment;
67 changes: 35 additions & 32 deletions src/renderers/dom/shared/ReactDOMComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ function trapBubbledEventsLocal() {
// If a component renders to null or if another component fatals and causes
// the state of the tree to be corrupted, `node` here can be null.
invariant(inst._rootNodeID, 'Must be mounted to trap events');
var node = ReactMount.getNode(inst._rootNodeID);
var node = getNode(inst);
invariant(
node,
'trapBubbledEvent(...): Requires node to be rendered.'
Expand Down Expand Up @@ -493,6 +493,14 @@ function isCustomComponent(tagName, props) {
return tagName.indexOf('-') >= 0 || props.is != null;
}

function getNode(inst) {
if (inst._nativeNode) {
return inst._nativeNode;
} else {
return inst._nativeNode = ReactMount.getNode(inst._rootNodeID);
}
}

/**
* Creates a new React class that is idempotent and capable of containing other
* React components. It accepts event listeners and DOM properties that are
Expand All @@ -513,10 +521,11 @@ function ReactDOMComponent(tag) {
this._renderedChildren = null;
this._previousStyle = null;
this._previousStyleCopy = null;
this._nativeNode = null;
this._rootNodeID = null;
this._wrapperState = null;
this._topLevelWrapper = null;
this._nodeWithLegacyProperties = null;
this._nodeHasLegacyProperties = false;
if (__DEV__) {
this._unprocessedContextDev = null;
this._processedContextDev = null;
Expand Down Expand Up @@ -600,10 +609,11 @@ ReactDOMComponent.Mixin = {
if (transaction.useCreateElement) {
var ownerDocument = context[ReactMount.ownerDocumentContextKey];
var el = ownerDocument.createElement(this._currentElement.type);
this._nativeNode = el;
DOMPropertyOperations.setAttributeForID(el, this._rootNodeID);
// Populate node cache
ReactMount.getID(el);
this._updateDOMProperties({}, props, transaction, el);
this._updateDOMProperties({}, props, transaction);
this._createInitialChildren(transaction, props, context, el);
mountImage = el;
} else {
Expand Down Expand Up @@ -844,16 +854,16 @@ ReactDOMComponent.Mixin = {


assertValidProps(this, nextProps);
this._updateDOMProperties(lastProps, nextProps, transaction, null);
this._updateDOMProperties(lastProps, nextProps, transaction);
this._updateDOMChildren(
lastProps,
nextProps,
transaction,
context
);

if (!canDefineProperty && this._nodeWithLegacyProperties) {
this._nodeWithLegacyProperties.props = nextProps;
if (!canDefineProperty && this._nodeHasLegacyProperties) {
this._nativeNode.props = nextProps;
}

if (this._tag === 'select') {
Expand All @@ -877,10 +887,9 @@ ReactDOMComponent.Mixin = {
* @private
* @param {object} lastProps
* @param {object} nextProps
* @param {ReactReconcileTransaction} transaction
* @param {?DOMElement} node
*/
_updateDOMProperties: function(lastProps, nextProps, transaction, node) {
_updateDOMProperties: function(lastProps, nextProps, transaction) {
var propKey;
var styleName;
var styleUpdates;
Expand Down Expand Up @@ -908,10 +917,7 @@ ReactDOMComponent.Mixin = {
} else if (
DOMProperty.properties[propKey] ||
DOMProperty.isCustomAttribute(propKey)) {
if (!node) {
node = ReactMount.getNode(this._rootNodeID);
}
DOMPropertyOperations.deleteValueForProperty(node, propKey);
DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey);
}
}
for (propKey in nextProps) {
Expand Down Expand Up @@ -964,20 +970,15 @@ ReactDOMComponent.Mixin = {
deleteListener(this._rootNodeID, propKey);
}
} else if (isCustomComponent(this._tag, nextProps)) {
if (!node) {
node = ReactMount.getNode(this._rootNodeID);
}
DOMPropertyOperations.setValueForAttribute(
node,
getNode(this),
propKey,
nextProp
);
} else if (
DOMProperty.properties[propKey] ||
DOMProperty.isCustomAttribute(propKey)) {
if (!node) {
node = ReactMount.getNode(this._rootNodeID);
}
var node = getNode(this);
// If we're updating to null or undefined, we should remove the property
// from the DOM node instead of inadvertantly setting to a string. This
// brings us in line with the same behavior we have on initial render.
Expand All @@ -989,10 +990,7 @@ ReactDOMComponent.Mixin = {
}
}
if (styleUpdates) {
if (!node) {
node = ReactMount.getNode(this._rootNodeID);
}
CSSPropertyOperations.setValueForStyles(node, styleUpdates);
CSSPropertyOperations.setValueForStyles(getNode(this), styleUpdates);
}
},

Expand Down Expand Up @@ -1089,21 +1087,26 @@ ReactDOMComponent.Mixin = {
break;
}

var nativeNode = getNode(this);
this._nativeNode = null;
if (this._nodeHasLegacyProperties) {
nativeNode._reactInternalComponent = null;
}

this.unmountChildren();
ReactBrowserEventEmitter.deleteAllListeners(this._rootNodeID);
ReactComponentBrowserEnvironment.unmountIDFromEnvironment(this._rootNodeID);
this._rootNodeID = null;
this._wrapperState = null;
if (this._nodeWithLegacyProperties) {
var node = this._nodeWithLegacyProperties;
node._reactInternalComponent = null;
this._nodeWithLegacyProperties = null;
}

return nativeNode;
},

getPublicInstance: function() {
if (!this._nodeWithLegacyProperties) {
var node = ReactMount.getNode(this._rootNodeID);
if (this._nodeHasLegacyProperties) {
return this._nativeNode;
} else {
var node = getNode(this);

node._reactInternalComponent = this;
node.getDOMNode = legacyGetDOMNode;
Expand All @@ -1126,9 +1129,9 @@ ReactDOMComponent.Mixin = {
node.props = this._currentElement.props;
}

this._nodeWithLegacyProperties = node;
this._nodeHasLegacyProperties = true;
return node;
}
return this._nodeWithLegacyProperties;
},

};
Expand Down
Loading

0 comments on commit f31a46c

Please sign in to comment.