Merge pull request #5160 from spicyj/gh-5151

Fetch node to unmount separately from unmounting

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 (
+          <div ref="a">
+            <div ref="b">b</div>
+            {this.props.showC && <div>c</div>}
+          </div>
+        );
+      },
+    });
+
+    var container = document.createElement('container');
+
+    ReactDOM.render(<div><Component showC={false} /></div>, 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(<div><Component showC={true} /></div>, container);
+
+    // Remove A, B, and C. Unmounting C shouldn't cause B to get recached.
+    ReactDOM.render(<div></div>, container);
+
+    // Add them back -- this shouldn't cause a cached node collision.
+    ReactDOM.render(<div><Component showC={true} /></div>, container);
+
+    ReactDOM.unmountComponentAtNode(container);
+  });
 });

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() {

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;
   },
 
 });

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

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;
   },
 });
 

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`.
    *

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.'
     );

src/test/ReactTestUtils.js

@@ -386,6 +386,10 @@ NoopInternalComponent.prototype = {
     this._currentElement = element;
   },
 
+  getNativeNode: function() {
+    return undefined;
+  },
+
   unmountComponent: function() {
   },