From e4a43389b84aa9b9278f432de64bbd5174e076ef Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 26 Sep 2015 14:15:43 -0700 Subject: [PATCH 1/5] Don't use existing "root" ID if non-root Before, if you had ``` container =
; ``` and did `ReactDOM.render(, container)` you would get ```
; ``` (along with a warning not to replace React-rendered children with a new tree like that). But that makes no sense -- the span should have a new index, not truncate the ID of the old child it's replacing. (Now tests pass again with useCreateElement on; before they threw a "valid but unequal" on our test for this warning.) --- src/renderers/dom/client/ReactMount.js | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index 2a231a420b0bf..41a370f783b46 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -724,19 +724,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; }, /** From 743ccf090fcf1d769df7b146f17a6e6726483051 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 26 Sep 2015 13:59:37 -0700 Subject: [PATCH 2/5] Cache native node on native components, return it when unmounting This is probably slightly slower for unmounts in the case that no updates were ever performed, but caching the node on the instance should make updates faster. In any case, the more important consequence of this change is that we can fix the current memory leak that happens when swapping composite types. --- src/renderers/dom/shared/ReactDOMComponent.js | 67 ++++++++++--------- .../dom/shared/ReactDOMTextComponent.js | 10 ++- .../reconciler/ReactCompositeComponent.js | 4 +- .../shared/reconciler/ReactEmptyComponent.js | 3 +- .../shared/reconciler/ReactReconciler.js | 2 +- 5 files changed, 50 insertions(+), 36 deletions(-) diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index 4e30c5f699941..827f06d945c94 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -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.' @@ -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 @@ -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; @@ -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 { @@ -844,7 +854,7 @@ ReactDOMComponent.Mixin = { assertValidProps(this, nextProps); - this._updateDOMProperties(lastProps, nextProps, transaction, null); + this._updateDOMProperties(lastProps, nextProps, transaction); this._updateDOMChildren( lastProps, nextProps, @@ -852,8 +862,8 @@ ReactDOMComponent.Mixin = { context ); - if (!canDefineProperty && this._nodeWithLegacyProperties) { - this._nodeWithLegacyProperties.props = nextProps; + if (!canDefineProperty && this._nodeHasLegacyProperties) { + this._nativeNode.props = nextProps; } if (this._tag === 'select') { @@ -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; @@ -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) { @@ -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. @@ -989,10 +990,7 @@ ReactDOMComponent.Mixin = { } } if (styleUpdates) { - if (!node) { - node = ReactMount.getNode(this._rootNodeID); - } - CSSPropertyOperations.setValueForStyles(node, styleUpdates); + CSSPropertyOperations.setValueForStyles(getNode(this), styleUpdates); } }, @@ -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; @@ -1126,9 +1129,9 @@ ReactDOMComponent.Mixin = { node.props = this._currentElement.props; } - this._nodeWithLegacyProperties = node; + this._nodeHasLegacyProperties = true; + return node; } - return this._nodeWithLegacyProperties; }, }; diff --git a/src/renderers/dom/shared/ReactDOMTextComponent.js b/src/renderers/dom/shared/ReactDOMTextComponent.js index 7212058905edc..9aab346986d10 100644 --- a/src/renderers/dom/shared/ReactDOMTextComponent.js +++ b/src/renderers/dom/shared/ReactDOMTextComponent.js @@ -52,6 +52,7 @@ assign(ReactDOMTextComponent.prototype, { // TODO: This is really a ReactText (ReactNode), not a ReactElement this._currentElement = text; this._stringText = '' + text; + this._nativeNode = null; // Properties this._rootNodeID = null; @@ -82,6 +83,7 @@ assign(ReactDOMTextComponent.prototype, { if (transaction.useCreateElement) { var ownerDocument = context[ReactMount.ownerDocumentContextKey]; var el = ownerDocument.createElement('span'); + this._nativeNode = el; DOMPropertyOperations.setAttributeForID(el, rootID); // Populate node cache ReactMount.getID(el); @@ -121,14 +123,20 @@ assign(ReactDOMTextComponent.prototype, { // and/or updateComponent to do the actual update for consistency with // other component types? this._stringText = nextStringText; - var node = ReactMount.getNode(this._rootNodeID); + var node = this._nativeNode; + if (!node) { + node = this._nativeNode = ReactMount.getNode(this._rootNodeID); + } DOMChildrenOperations.updateTextContent(node, nextStringText); } } }, unmountComponent: function() { + var node = this._nativeNode || ReactMount.getNode(this._rootNodeID); + this._nativeNode = null; ReactComponentBrowserEnvironment.unmountIDFromEnvironment(this._rootNodeID); + return node; }, }); diff --git a/src/renderers/shared/reconciler/ReactCompositeComponent.js b/src/renderers/shared/reconciler/ReactCompositeComponent.js index fb570f7fe3d62..452a59dcef852 100644 --- a/src/renderers/shared/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/reconciler/ReactCompositeComponent.js @@ -310,7 +310,8 @@ var ReactCompositeComponentMixin = { inst.componentWillUnmount(); } - ReactReconciler.unmountComponent(this._renderedComponent); + var unmountedNativeNode = + ReactReconciler.unmountComponent(this._renderedComponent); this._renderedComponent = null; this._instance = null; @@ -339,6 +340,7 @@ var ReactCompositeComponentMixin = { // TODO: inst.props = null; // TODO: inst.state = null; // TODO: inst.context = null; + return unmountedNativeNode; }, /** diff --git a/src/renderers/shared/reconciler/ReactEmptyComponent.js b/src/renderers/shared/reconciler/ReactEmptyComponent.js index 715dece5f1b56..1829265b9c034 100644 --- a/src/renderers/shared/reconciler/ReactEmptyComponent.js +++ b/src/renderers/shared/reconciler/ReactEmptyComponent.js @@ -46,10 +46,11 @@ assign(ReactEmptyComponent.prototype, { receiveComponent: function() { }, unmountComponent: function(rootID, transaction, context) { - ReactReconciler.unmountComponent(this._renderedComponent); + var nativeNode = ReactReconciler.unmountComponent(this._renderedComponent); ReactEmptyComponentRegistry.deregisterNullComponentID(this._rootNodeID); this._rootNodeID = null; this._renderedComponent = null; + return nativeNode; }, }); diff --git a/src/renderers/shared/reconciler/ReactReconciler.js b/src/renderers/shared/reconciler/ReactReconciler.js index ab3fbb0f6c14d..4e50d387b3b9f 100644 --- a/src/renderers/shared/reconciler/ReactReconciler.js +++ b/src/renderers/shared/reconciler/ReactReconciler.js @@ -50,7 +50,7 @@ var ReactReconciler = { */ unmountComponent: function(internalInstance) { ReactRef.detachRefs(internalInstance, internalInstance._currentElement); - internalInstance.unmountComponent(); + return internalInstance.unmountComponent(); }, /** From 60491d89f88a04a6be29a52b652090ba276ba7a9 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 26 Sep 2015 14:07:01 -0700 Subject: [PATCH 3/5] Use returned native node for composite type-change With this change, all unmounted components should be properly purged from ReactMount's cache. --- .../dom/client/ReactDOMIDOperations.js | 14 -------------- .../shared/ReactComponentBrowserEnvironment.js | 14 ++++++++++++-- .../reconciler/ReactComponentEnvironment.js | 6 +++--- .../reconciler/ReactCompositeComponent.js | 18 +++++++++--------- src/test/ReactDefaultPerf.js | 3 ++- src/test/ReactTestUtils.js | 2 +- 6 files changed, 27 insertions(+), 30 deletions(-) diff --git a/src/renderers/dom/client/ReactDOMIDOperations.js b/src/renderers/dom/client/ReactDOMIDOperations.js index d8630ce0a51b5..7b419e2bf9663 100644 --- a/src/renderers/dom/client/ReactDOMIDOperations.js +++ b/src/renderers/dom/client/ReactDOMIDOperations.js @@ -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. * @@ -92,7 +79,6 @@ var ReactDOMIDOperations = { }; ReactPerf.measureMethods(ReactDOMIDOperations, 'ReactDOMIDOperations', { - dangerouslyReplaceNodeWithMarkupByID: 'dangerouslyReplaceNodeWithMarkupByID', dangerouslyProcessChildrenUpdates: 'dangerouslyProcessChildrenUpdates', }); diff --git a/src/renderers/dom/shared/ReactComponentBrowserEnvironment.js b/src/renderers/dom/shared/ReactComponentBrowserEnvironment.js index e1ee6fc66b4c6..3f31a52a2174c 100644 --- a/src/renderers/dom/shared/ReactComponentBrowserEnvironment.js +++ b/src/renderers/dom/shared/ReactComponentBrowserEnvironment.js @@ -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 @@ -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, @@ -40,4 +42,12 @@ var ReactComponentBrowserEnvironment = { }; +ReactPerf.measureMethods( + ReactComponentBrowserEnvironment, + 'ReactComponentBrowserEnvironment', + { + replaceNodeWithMarkup: 'replaceNodeWithMarkup', + } +); + module.exports = ReactComponentBrowserEnvironment; diff --git a/src/renderers/shared/reconciler/ReactComponentEnvironment.js b/src/renderers/shared/reconciler/ReactComponentEnvironment.js index 91359f2bbe649..274f53f3b4698 100644 --- a/src/renderers/shared/reconciler/ReactComponentEnvironment.js +++ b/src/renderers/shared/reconciler/ReactComponentEnvironment.js @@ -28,7 +28,7 @@ var ReactComponentEnvironment = { * Optionally injectable hook for swapping out mount images in the middle of * the tree. */ - replaceNodeWithMarkupByID: null, + replaceNodeWithMarkup: null, /** * Optionally injectable hook for processing a queue of child updates. Will @@ -44,8 +44,8 @@ var ReactComponentEnvironment = { ); ReactComponentEnvironment.unmountIDFromEnvironment = environment.unmountIDFromEnvironment; - ReactComponentEnvironment.replaceNodeWithMarkupByID = - environment.replaceNodeWithMarkupByID; + ReactComponentEnvironment.replaceNodeWithMarkup = + environment.replaceNodeWithMarkup; ReactComponentEnvironment.processChildrenUpdates = environment.processChildrenUpdates; injected = true; diff --git a/src/renderers/shared/reconciler/ReactCompositeComponent.js b/src/renderers/shared/reconciler/ReactCompositeComponent.js index 452a59dcef852..c7c9a8c3542a5 100644 --- a/src/renderers/shared/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/reconciler/ReactCompositeComponent.js @@ -729,30 +729,30 @@ var ReactCompositeComponentMixin = { this._processChildContext(context) ); } else { - // These two IDs are actually the same! But nothing should rely on that. - var thisID = this._rootNodeID; - var prevComponentID = prevComponentInstance._rootNodeID; - ReactReconciler.unmountComponent(prevComponentInstance); + var oldNativeNode = + ReactReconciler.unmountComponent(prevComponentInstance); this._renderedComponent = this._instantiateReactComponent( nextRenderedElement ); var nextMarkup = ReactReconciler.mountComponent( this._renderedComponent, - thisID, + this._rootNodeID, transaction, this._processChildContext(context) ); - this._replaceNodeWithMarkupByID(prevComponentID, nextMarkup); + this._replaceNodeWithMarkup(oldNativeNode, nextMarkup); } }, /** + * Overridden in shallow rendering. + * * @protected */ - _replaceNodeWithMarkupByID: function(prevComponentID, nextMarkup) { - ReactComponentEnvironment.replaceNodeWithMarkupByID( - prevComponentID, + _replaceNodeWithMarkup: function(oldNativeNode, nextMarkup) { + ReactComponentEnvironment.replaceNodeWithMarkup( + oldNativeNode, nextMarkup ); }, diff --git a/src/test/ReactDefaultPerf.js b/src/test/ReactDefaultPerf.js index 8930573461b13..22b613a11d58f 100644 --- a/src/test/ReactDefaultPerf.js +++ b/src/test/ReactDefaultPerf.js @@ -169,7 +169,8 @@ var ReactDefaultPerf = { moduleName === 'ReactDOMIDOperations' || moduleName === 'CSSPropertyOperations' || moduleName === 'DOMChildrenOperations' || - moduleName === 'DOMPropertyOperations') { + moduleName === 'DOMPropertyOperations' || + moduleName === 'ReactComponentBrowserEnvironment') { start = performanceNow(); rv = func.apply(this, args); totalTime = performanceNow() - start; diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index b28de910029d2..2782df32da10d 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -395,7 +395,7 @@ assign( _instantiateReactComponent: function(element) { return new NoopInternalComponent(element); }, - _replaceNodeWithMarkupByID: function() {}, + _replaceNodeWithMarkup: function() {}, _renderValidatedComponent: ReactCompositeComponent.Mixin ._renderValidatedComponentWithoutOwnerOrContext, From fe9a76ef25bd0cd274598572d5cb0a952edd00da Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 26 Sep 2015 15:11:21 -0700 Subject: [PATCH 4/5] Rewrite ReactInstanceHandles-test to be less brittle --- .../__tests__/ReactInstanceHandles-test.js | 92 +++++++++++-------- 1 file changed, 52 insertions(+), 40 deletions(-) diff --git a/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js b/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js index f5dbc051ffc80..7cb1ba9d95fc1 100644 --- a/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js +++ b/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js @@ -76,72 +76,84 @@ describe('ReactInstanceHandles', function() { describe('findComponentRoot', function() { it('should find the correct node with prefix sibling IDs', function() { - var parentNode = document.createElement('div'); - var childNodeA = document.createElement('div'); - var childNodeB = document.createElement('div'); - parentNode.appendChild(childNodeA); - parentNode.appendChild(childNodeB); - - ReactMount.setID(parentNode, '.0'); - ReactMount.setID(childNodeA, '.0.0'); - ReactMount.setID(childNodeB, '.0.0:1'); + var parentNode = ReactTestUtils.renderIntoDocument( +
+
+ {[
]} +
+ ); + var childNodeB = parentNode.childNodes[1]; expect( - ReactMount.findComponentRoot( - parentNode, - ReactMount.getID(childNodeB) + ReactMount.getNode( + getNodeID(childNodeB) ) ).toBe(childNodeB); }); it('should work around unidentified nodes', function() { - var parentNode = document.createElement('div'); - var childNodeA = document.createElement('div'); - var childNodeB = document.createElement('div'); - parentNode.appendChild(childNodeA); - parentNode.appendChild(childNodeB); + var parentNode = ReactTestUtils.renderIntoDocument( +
+ {[
]} +
+ ); + var childNodeB = parentNode.childNodes[0]; - ReactMount.setID(parentNode, '.0'); // No ID on `childNodeA`. - ReactMount.setID(childNodeB, '.0.0:1'); + var childNodeA = document.createElement('div'); + parentNode.insertBefore(childNodeA, childNodeB); expect( - ReactMount.findComponentRoot( - parentNode, - ReactMount.getID(childNodeB) + ReactMount.getNode( + getNodeID(childNodeB) ) ).toBe(childNodeB); }); it('should throw if a rendered element cannot be found', function() { - var parentNode = document.createElement('table'); - var childNodeA = document.createElement('tbody'); - var childNodeB = document.createElement('tr'); - parentNode.appendChild(childNodeA); - childNodeA.appendChild(childNodeB); - - ReactMount.setID(parentNode, '.0'); - // No ID on `childNodeA`, it was "rendered by the browser". - ReactMount.setID(childNodeB, '.0.1:0'); + spyOn(console, 'error'); + var parentNode = ReactTestUtils.renderIntoDocument( + + +
+ ); + var childNodeA = parentNode.childNodes[0]; + var childNodeB; + if (childNodeA.tagName === 'TR') { + childNodeB = childNodeA; + // No ID on `childNodeA`, it was "rendered by the browser". + childNodeA = document.createElement('tbody'); + childNodeA.appendChild(childNodeB); + parentNode.appendChild(childNodeA); + } else { + childNodeB = childNodeA.childNodes[0]; + } + expect(childNodeA.tagName).toBe('TBODY'); - expect(ReactMount.findComponentRoot( - parentNode, - ReactMount.getID(childNodeB) - )).toBe(childNodeB); + expect( + ReactMount.getNode( + getNodeID(childNodeB) + ) + ).toBe(childNodeB); + var junkID = getNodeID(childNodeB) + ':junk'; expect(function() { - ReactMount.findComponentRoot( - parentNode, - ReactMount.getID(childNodeB) + ':junk' + ReactMount.getNode( + junkID ); }).toThrow( - 'Invariant Violation: findComponentRoot(..., .0.1:0:junk): ' + + 'Invariant Violation: findComponentRoot(..., ' + junkID + '): ' + 'Unable to find element. This probably means the DOM was ' + 'unexpectedly mutated (e.g., by the browser), usually due to ' + 'forgetting a when using tables, nesting tags ' + 'like
,

, or , or using non-SVG elements in an ' + 'parent. ' + - 'Try inspecting the child nodes of the element with React ID `.0`.' + 'Try inspecting the child nodes of the element with React ID ``.' + ); + + expect(console.error.argsForCall.length).toBe(1); + expect(console.error.argsForCall[0][0]).toContain( + '
cannot appear as a child of
' ); }); }); From 8ce7b7120cff9f46237fef73120c9ddca25e30fc Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Sat, 26 Sep 2015 15:11:44 -0700 Subject: [PATCH 5/5] ReactMount now never expects invalid nodes in its cache It never really made sense for us to have "invalid" nodes in the cache -- when we unmount things, we should always remove them from the cache properly. Now that swapping composite types doesn't repopulate the cache, we should be okay to now assume that everything in the cache is good. --- src/renderers/dom/client/ReactMount.js | 58 +++++++++++++++----------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index 41a370f783b46..8ab20475fe8c5 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -100,7 +100,7 @@ function getReactRootElementInContainer(container) { */ function getReactRootID(container) { var rootElement = getReactRootElementInContainer(container); - return rootElement && ReactMount.getID(rootElement); + return rootElement && internalGetID(rootElement); } /** @@ -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; @@ -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. * @@ -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]; } /** @@ -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); } /** @@ -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