diff --git a/src/renderers/dom/client/__tests__/ReactMount-test.js b/src/renderers/dom/client/__tests__/ReactMount-test.js index eaaa38f6ed..bec3a3bf11 100644 --- a/src/renderers/dom/client/__tests__/ReactMount-test.js +++ b/src/renderers/dom/client/__tests__/ReactMount-test.js @@ -245,4 +245,34 @@ describe('ReactMount', function() { 'render the new components instead of calling ReactDOM.render.' ); }); + + it('should not crash in node cache when unmounting', function() { + var Component = React.createClass({ + render: function() { + // Add refs to some nodes so that they get traversed and cached + return ( +
+
b
+ {this.props.showC &&
c
} +
+ ); + }, + }); + + var container = document.createElement('container'); + + ReactDOM.render(
, container); + + // Right now, A and B are in the cache. When we add C, it won't get added to + // the cache (assuming markup-string mode). + ReactDOM.render(
, container); + + // Remove A, B, and C. Unmounting C shouldn't cause B to get recached. + ReactDOM.render(
, container); + + // Add them back -- this shouldn't cause a cached node collision. + ReactDOM.render(
, container); + + ReactDOM.unmountComponentAtNode(container); + }); }); diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index c274c4a60c..cf7c1587c1 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -1068,6 +1068,10 @@ ReactDOMComponent.Mixin = { } }, + getNativeNode: function() { + return getNode(this); + }, + /** * Destroys all event registrations for this instance. Does not remove from * the DOM. That must be done by the parent. @@ -1112,19 +1116,16 @@ ReactDOMComponent.Mixin = { break; } - var nativeNode = getNode(this); - this._nativeNode = null; if (this._nodeHasLegacyProperties) { - nativeNode._reactInternalComponent = null; + this._nativeNode._reactInternalComponent = null; } + this._nativeNode = null; this.unmountChildren(); ReactBrowserEventEmitter.deleteAllListeners(this._rootNodeID); ReactComponentBrowserEnvironment.unmountIDFromEnvironment(this._rootNodeID); this._rootNodeID = null; this._wrapperState = null; - - return nativeNode; }, getPublicInstance: function() { diff --git a/src/renderers/dom/shared/ReactDOMTextComponent.js b/src/renderers/dom/shared/ReactDOMTextComponent.js index d5e3cc38c2..ded6a67381 100644 --- a/src/renderers/dom/shared/ReactDOMTextComponent.js +++ b/src/renderers/dom/shared/ReactDOMTextComponent.js @@ -23,6 +23,14 @@ var escapeTextContentForBrowser = require('escapeTextContentForBrowser'); var setTextContent = require('setTextContent'); var validateDOMNesting = require('validateDOMNesting'); +function getNode(inst) { + if (inst._nativeNode) { + return inst._nativeNode; + } else { + return inst._nativeNode = ReactMount.getNode(inst._rootNodeID); + } +} + /** * Text nodes violate a couple assumptions that React makes about components: * @@ -133,20 +141,18 @@ assign(ReactDOMTextComponent.prototype, { // and/or updateComponent to do the actual update for consistency with // other component types? this._stringText = nextStringText; - var node = this._nativeNode; - if (!node) { - node = this._nativeNode = ReactMount.getNode(this._rootNodeID); - } - DOMChildrenOperations.updateTextContent(node, nextStringText); + DOMChildrenOperations.updateTextContent(getNode(this), nextStringText); } } }, + getNativeNode: function() { + return getNode(this); + }, + 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 3b564715f5..0e30a0128d 100644 --- a/src/renderers/shared/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/reconciler/ReactCompositeComponent.js @@ -309,6 +309,10 @@ var ReactCompositeComponentMixin = { return markup; }, + getNativeNode: function() { + return ReactReconciler.getNativeNode(this._renderedComponent); + }, + /** * Releases any resources allocated by `mountComponent`. * @@ -322,8 +326,7 @@ var ReactCompositeComponentMixin = { inst.componentWillUnmount(); } - var unmountedNativeNode = - ReactReconciler.unmountComponent(this._renderedComponent); + ReactReconciler.unmountComponent(this._renderedComponent); this._renderedComponent = null; this._instance = null; @@ -352,7 +355,6 @@ var ReactCompositeComponentMixin = { // TODO: inst.props = null; // TODO: inst.state = null; // TODO: inst.context = null; - return unmountedNativeNode; }, /** @@ -741,8 +743,14 @@ var ReactCompositeComponentMixin = { this._processChildContext(context) ); } else { - var oldNativeNode = - ReactReconciler.unmountComponent(prevComponentInstance); + // TODO: This is currently necessary due to the unfortunate caching + // that ReactMount does which makes it exceedingly difficult to unmount + // a set of siblings without accidentally repopulating the node cache (see + // #5151). Once ReactMount no longer stores the nodes by ID, this method + // can go away. + var oldNativeNode = ReactReconciler.getNativeNode(prevComponentInstance); + + ReactReconciler.unmountComponent(prevComponentInstance); this._renderedComponent = this._instantiateReactComponent( nextRenderedElement diff --git a/src/renderers/shared/reconciler/ReactEmptyComponent.js b/src/renderers/shared/reconciler/ReactEmptyComponent.js index aaf41858fd..48430d6009 100644 --- a/src/renderers/shared/reconciler/ReactEmptyComponent.js +++ b/src/renderers/shared/reconciler/ReactEmptyComponent.js @@ -53,12 +53,14 @@ assign(ReactEmptyComponent.prototype, { }, receiveComponent: function() { }, + getNativeNode: function() { + return ReactReconciler.getNativeNode(this._renderedComponent); + }, unmountComponent: function(rootID, transaction, context) { - var nativeNode = ReactReconciler.unmountComponent(this._renderedComponent); + 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 b25df837c8..7b3f453e1a 100644 --- a/src/renderers/shared/reconciler/ReactReconciler.js +++ b/src/renderers/shared/reconciler/ReactReconciler.js @@ -57,6 +57,14 @@ var ReactReconciler = { return markup; }, + /** + * Returns a value that can be passed to + * ReactComponentEnvironment.replaceNodeWithMarkup. + */ + getNativeNode: function(internalInstance) { + return internalInstance.getNativeNode(); + }, + /** * Releases any resources allocated by `mountComponent`. * diff --git a/src/renderers/shared/reconciler/instantiateReactComponent.js b/src/renderers/shared/reconciler/instantiateReactComponent.js index 8a320f4fdc..588014dc93 100644 --- a/src/renderers/shared/reconciler/instantiateReactComponent.js +++ b/src/renderers/shared/reconciler/instantiateReactComponent.js @@ -105,6 +105,7 @@ function instantiateReactComponent(node) { typeof instance.construct === 'function' && typeof instance.mountComponent === 'function' && typeof instance.receiveComponent === 'function' && + typeof instance.getNativeNode === 'function' && typeof instance.unmountComponent === 'function', 'Only React Components can be mounted.' ); diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index cd8b568207..8151a90a70 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -386,6 +386,10 @@ NoopInternalComponent.prototype = { this._currentElement = element; }, + getNativeNode: function() { + return undefined; + }, + unmountComponent: function() { },