Kill ReactMount.getNode/getID/purgeID with fire

sophiebits · sophiebits · commit 4ba0e95a9650 · 2015-11-04T11:39:43.000-08:00
diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js
diff --git a/src/renderers/dom/client/__tests__/ReactMount-test.js b/src/renderers/dom/client/__tests__/ReactMount-test.js
@@ -218,61 +218,6 @@ describe('ReactMount', function() {
     );
   });
 
-  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);
-  });
-
-  it('should not crash in node cache when unmounting, case 2', function() {
-    var A = React.createClass({
-      render: function() {
-        return <a key={this.props.innerKey}>{this.props.innerKey}</a>;
-      },
-    });
-    var Component = React.createClass({
-      render: function() {
-        return (
-          <b>
-            <i>{this.props.step === 1 && <q />}</i>
-            {this.props.step === 1 && <A innerKey={this.props.step} />}
-          </b>
-        );
-      },
-    });
-
-    var container = document.createElement('container');
-
-    ReactDOM.render(<Component step={1} />, container);
-    ReactDOM.render(<Component step={2} />, container);
-    ReactDOM.render(<Component step={1} />, container);
-    ReactMount.getID(container.querySelector('a'));
-  });
-
   it('passes the correct callback context', function() {
     var container = document.createElement('div');
     var calls = 0;
diff --git a/src/renderers/dom/client/__tests__/ReactRenderDocument-test.js b/src/renderers/dom/client/__tests__/ReactRenderDocument-test.js
@@ -17,8 +17,6 @@ jest
 var React;
 var ReactDOM;
 var ReactDOMServer;
-var ReactInstanceMap;
-var ReactMount;
 
 var getTestDocument;
 
@@ -46,14 +44,12 @@ describe('rendering React components at document', function() {
     React = require('React');
     ReactDOM = require('ReactDOM');
     ReactDOMServer = require('ReactDOMServer');
-    ReactInstanceMap = require('ReactInstanceMap');
-    ReactMount = require('ReactMount');
     getTestDocument = require('getTestDocument');
 
     testDocument = getTestDocument();
   });
 
-  it('should be able to get root component id for document node', function() {
+  it('should be able to adopt server markup', function() {
     expect(testDocument).not.toBeUndefined();
 
     var Root = React.createClass({
@@ -64,22 +60,24 @@ describe('rendering React components at document', function() {
               <title>Hello World</title>
             </head>
             <body>
-              Hello world
+              {'Hello ' + this.props.hello}
             </body>
           </html>
         );
       },
     });
 
-    var markup = ReactDOMServer.renderToString(<Root />);
+    var markup = ReactDOMServer.renderToString(<Root hello="world" />);
     testDocument = getTestDocument(markup);
-    var component = ReactDOM.render(<Root />, testDocument);
+    var body = testDocument.body;
+
+    ReactDOM.render(<Root hello="world" />, testDocument);
     expect(testDocument.body.innerHTML).toBe('Hello world');
 
-    // TODO: This is a bad test. I have no idea what this is testing.
-    // Node IDs is an implementation detail and not part of the public API.
-    var componentID = ReactMount.getReactRootID(testDocument);
-    expect(componentID).toBe(ReactInstanceMap.get(component)._rootNodeID);
+    ReactDOM.render(<Root hello="moon" />, testDocument);
+    expect(testDocument.body.innerHTML).toBe('Hello moon');
+
+    expect(body).toBe(testDocument.body);
   });
 
   it('should not be able to unmount component from document node', function() {
diff --git a/src/renderers/dom/client/wrappers/ReactDOMInput.js b/src/renderers/dom/client/wrappers/ReactDOMInput.js
@@ -14,7 +14,6 @@
 var DOMPropertyOperations = require('DOMPropertyOperations');
 var LinkedValueUtils = require('LinkedValueUtils');
 var ReactDOMComponentTree = require('ReactDOMComponentTree');
-var ReactMount = require('ReactMount');
 var ReactUpdates = require('ReactUpdates');
 
 var assign = require('Object.assign');
@@ -189,19 +188,13 @@ function _handleChange(event) {
       // This will throw if radio buttons rendered by different copies of React
       // and the same name are rendered into the same form (same as #1939).
       // That's probably okay; we don't support it just as we don't support
-      // mixing React with non-React.
-      var otherID = ReactMount.getID(otherNode);
+      // mixing React radio buttons with non-React ones.
+      var otherInstance = ReactDOMComponentTree.getInstanceFromNode(otherNode);
       invariant(
-        otherID,
+        otherInstance,
         'ReactDOMInput: Mixing React and non-React radio inputs with the ' +
         'same `name` is not supported.'
       );
-      var otherInstance = instancesByReactID[otherID];
-      invariant(
-        otherInstance,
-        'ReactDOMInput: Unknown radio button ID %s.',
-        otherID
-      );
       // If this is a controlled radio button group, forcing the input that
       // was previously checked to update will cause it to be come re-checked
       // as appropriate.
diff --git a/src/renderers/dom/shared/ReactComponentBrowserEnvironment.js b/src/renderers/dom/shared/ReactComponentBrowserEnvironment.js
@@ -13,7 +13,6 @@
 
 var DOMChildrenOperations = require('DOMChildrenOperations');
 var ReactDOMIDOperations = require('ReactDOMIDOperations');
-var ReactMount = require('ReactMount');
 var ReactPerf = require('ReactPerf');
 
 /**
@@ -37,7 +36,6 @@ var ReactComponentBrowserEnvironment = {
    * @private
    */
   unmountIDFromEnvironment: function(rootNodeID) {
-    ReactMount.purgeID(rootNodeID);
   },
 
 };
diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js
@@ -33,7 +33,6 @@ var ReactDOMInput = require('ReactDOMInput');
 var ReactDOMOption = require('ReactDOMOption');
 var ReactDOMSelect = require('ReactDOMSelect');
 var ReactDOMTextarea = require('ReactDOMTextarea');
-var ReactMount = require('ReactMount');
 var ReactMultiChild = require('ReactMultiChild');
 var ReactPerf = require('ReactPerf');
 var ReactUpdateQueue = require('ReactUpdateQueue');
@@ -662,8 +661,6 @@ ReactDOMComponent.Mixin = {
       ReactDOMComponentTree.precacheNode(this, el);
       this._flags |= Flags.hasCachedChildNodes;
       DOMPropertyOperations.setAttributeForID(el, this._rootNodeID);
-      // Populate node cache
-      ReactMount.getID(el);
       this._updateDOMProperties(null, props, transaction);
       var lazyTree = DOMLazyTree(el);
       this._createInitialChildren(transaction, props, context, lazyTree);
diff --git a/src/renderers/dom/shared/ReactDOMTextComponent.js b/src/renderers/dom/shared/ReactDOMTextComponent.js
@@ -18,7 +18,6 @@ var DOMPropertyOperations = require('DOMPropertyOperations');
 var ReactComponentBrowserEnvironment =
   require('ReactComponentBrowserEnvironment');
 var ReactDOMComponentTree = require('ReactDOMComponentTree');
-var ReactMount = require('ReactMount');
 
 var assign = require('Object.assign');
 var escapeTextContentForBrowser = require('escapeTextContentForBrowser');
@@ -99,10 +98,8 @@ assign(ReactDOMTextComponent.prototype, {
     if (transaction.useCreateElement) {
       var ownerDocument = nativeContainerInfo._ownerDocument;
       var el = ownerDocument.createElement('span');
-      this._nativeNode = el;
+      ReactDOMComponentTree.precacheNode(this, el);
       DOMPropertyOperations.setAttributeForID(el, rootID);
-      // Populate node cache
-      ReactMount.getID(el);
       var lazyTree = DOMLazyTree(el);
       DOMLazyTree.queueText(lazyTree, this._stringText);
       return lazyTree;
diff --git a/src/renderers/shared/reconciler/__tests__/ReactCompositeComponent-test.js b/src/renderers/shared/reconciler/__tests__/ReactCompositeComponent-test.js
@@ -16,7 +16,6 @@ var MorphingComponent;
 var React;
 var ReactDOM;
 var ReactCurrentOwner;
-var ReactMount;
 var ReactPropTypes;
 var ReactServerRendering;
 var ReactTestUtils;
@@ -34,7 +33,6 @@ describe('ReactCompositeComponent', function() {
     ReactCurrentOwner = require('ReactCurrentOwner');
     ReactPropTypes = require('ReactPropTypes');
     ReactTestUtils = require('ReactTestUtils');
-    ReactMount = require('ReactMount');
     ReactServerRendering = require('ReactServerRendering');
     ReactUpdates = require('ReactUpdates');
 
@@ -487,8 +485,6 @@ describe('ReactCompositeComponent', function() {
     var container = document.createElement('div');
     var innerUnmounted = false;
 
-    spyOn(ReactMount, 'purgeID').andCallThrough();
-
     var Component = React.createClass({
       render: function() {
         return (
@@ -501,11 +497,6 @@ describe('ReactCompositeComponent', function() {
     });
     var Inner = React.createClass({
       componentWillUnmount: function() {
-        // It's important that ReactMount.purgeID is called after any component
-        // lifecycle methods, because a componentWillUnmount implementation is
-        // likely to call ReactDOM.findDOMNode(this), which will repopulate the
-        // node cache after it's been cleared, causing a memory leak.
-        expect(ReactMount.purgeID.calls.length).toBe(0);
         innerUnmounted = true;
       },
       render: function() {
@@ -516,12 +507,6 @@ describe('ReactCompositeComponent', function() {
     ReactDOM.render(<Component />, container);
     ReactDOM.unmountComponentAtNode(container);
     expect(innerUnmounted).toBe(true);
-
-    // The text and both <div /> elements and their wrappers each call
-    // unmountIDFromEnvironment which calls purgeID, for a total of 3.
-    // TODO: Test the effect of this. E.g. does the node cache get repopulated
-    // after a getDOMNode call?
-    expect(ReactMount.purgeID.calls.length).toBe(3);
   });
 
   it('should warn when shouldComponentUpdate() returns undefined', function() {
diff --git a/src/renderers/shared/reconciler/__tests__/ReactIdentity-test.js b/src/renderers/shared/reconciler/__tests__/ReactIdentity-test.js
@@ -13,24 +13,26 @@
 
 var React;
 var ReactDOM;
+var ReactDOMComponentTree;
 var ReactFragment;
 var ReactTestUtils;
-var ReactMount;
 
 describe('ReactIdentity', function() {
 
   beforeEach(function() {
     jest.resetModuleRegistry();
     React = require('React');
     ReactDOM = require('ReactDOM');
+    ReactDOMComponentTree = require('ReactDOMComponentTree');
     ReactFragment = require('ReactFragment');
     ReactTestUtils = require('ReactTestUtils');
-    ReactMount = require('ReactMount');
   });
 
   var idExp = /^\.[^.]+(.*)$/;
   function checkID(child, expectedID) {
-    var actual = idExp.exec(ReactMount.getID(child));
+    // TODO: Node IDs are not public API; rewrite these tests.
+    var rootID = ReactDOMComponentTree.getInstanceFromNode(child)._rootNodeID;
+    var actual = idExp.exec(rootID);
     var expected = idExp.exec(expectedID);
     expect(actual).toBeTruthy();
     expect(expected).toBeTruthy();
@@ -297,14 +299,16 @@ describe('ReactIdentity', function() {
     var wrapped = <TestContainer first={instance0} second={instance1} />;
 
     wrapped = ReactDOM.render(wrapped, document.createElement('div'));
+    var div = ReactDOM.findDOMNode(wrapped);
 
-    var beforeID = ReactMount.getID(ReactDOM.findDOMNode(wrapped).firstChild);
-
+    var beforeA = div.childNodes[0];
+    var beforeB = div.childNodes[1];
     wrapped.swap();
+    var afterA = div.childNodes[1];
+    var afterB = div.childNodes[0];
 
-    var afterID = ReactMount.getID(ReactDOM.findDOMNode(wrapped).firstChild);
-
-    expect(beforeID).not.toEqual(afterID);
+    expect(beforeA).toBe(afterA);
+    expect(beforeB).toBe(afterB);
 
   });
 
diff --git a/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js b/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js
diff --git a/src/renderers/shared/reconciler/__tests__/ReactMultiChildReconcile-test.js b/src/renderers/shared/reconciler/__tests__/ReactMultiChildReconcile-test.js
diff --git a/src/test/ReactDefaultPerf.js b/src/test/ReactDefaultPerf.js
