From 622db4ee4fb61451b32fceb13d68a9589a2faecd Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Tue, 13 Oct 2015 12:07:57 -0700 Subject: [PATCH] Fetch node to unmount separately from unmounting My last strategy of getting each node recursively while unmounting was a pain to make work properly with ReactMount's confusing cache. Now, we get the node before unmounting anything in the subtree (and we don't try to find the nodes of descendants). This is a temporary solution and can go away when we get rid of the giant ReactMount node hash map. Fixes #5151. --- .../dom/client/__tests__/ReactMount-test.js | 30 +++++++++++++++++++ src/renderers/dom/shared/ReactDOMComponent.js | 11 +++---- .../dom/shared/ReactDOMTextComponent.js | 20 ++++++++----- .../reconciler/ReactCompositeComponent.js | 18 +++++++---- .../shared/reconciler/ReactEmptyComponent.js | 6 ++-- .../shared/reconciler/ReactReconciler.js | 8 +++++ .../reconciler/instantiateReactComponent.js | 1 + src/test/ReactTestUtils.js | 4 +++ 8 files changed, 79 insertions(+), 19 deletions(-) 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() { },