Pass shouldHaveDebugID flag to instantiateComponent (#7193)

* Add failing tests for #7187 and #7190

* Pass shouldHaveDebugID flag to instantiateComponent

This allows us to remove a hack that was added in #6770 and caused #7187 and #7190.

* Move logic for choosing whether to use debugID outside instantiate

src/isomorphic/devtools/ReactComponentTreeDevtool.js

@@ -32,10 +32,6 @@ function updateTree(id, update) {
       isMounted: false,
       updateCount: 0,
     };
-    // TODO: We need to do this awkward dance because TopLevelWrapper "never
-    // gets mounted" but its display name gets set in instantiateReactComponent
-    // before its debug ID is set to 0.
-    unmountedIDs[id] = true;
   }
   update(tree[id]);
 }
@@ -148,7 +144,6 @@ var ReactComponentTreeDevtool = {
 
   onMountComponent(id) {
     updateTree(id, item => item.isMounted = true);
-    delete unmountedIDs[id];
   },
 
   onMountRootComponent(id) {

src/renderers/dom/client/ReactMount.js

@@ -342,13 +342,7 @@ var ReactMount = {
     );
 
     ReactBrowserEventEmitter.ensureScrollValueMonitoring();
-    var componentInstance = instantiateReactComponent(nextElement);
-
-    if (__DEV__) {
-      // Mute future events from the top level wrapper.
-      // It is an implementation detail that devtools should not know about.
-      componentInstance._debugID = 0;
-    }
+    var componentInstance = instantiateReactComponent(nextElement, false);
 
     // The initial render is synchronous but any updates that happen during
     // rendering, in componentWillMount or componentDidMount, will be batched

src/renderers/dom/server/ReactServerRendering.js

@@ -40,7 +40,7 @@ function renderToStringImpl(element, makeStaticMarkup) {
       if (__DEV__) {
         ReactInstrumentation.debugTool.onBeginFlush();
       }
-      var componentInstance = instantiateReactComponent(element);
+      var componentInstance = instantiateReactComponent(element, true);
       var markup = ReactReconciler.mountComponent(
         componentInstance,
         transaction,

src/renderers/native/ReactNativeMount.js

@@ -135,17 +135,11 @@ var ReactNativeMount = {
 
     ReactNativeTagHandles.assertRootTag(containerTag);
 
-    var instance = instantiateReactComponent(nextWrappedElement);
+    var instance = instantiateReactComponent(nextWrappedElement, false);
     ReactNativeMount._instancesByContainerID[containerTag] = instance;
 
     if (__DEV__) {
-      // Mute future events from the top level wrapper.
-      // It is an implementation detail that devtools should not know about.
-      instance._debugID = 0;
-
-      if (__DEV__) {
-        ReactInstrumentation.debugTool.onBeginFlush();
-      }
+      ReactInstrumentation.debugTool.onBeginFlush();
     }
 
     // The initial render is synchronous but any updates that happen during

src/renderers/shared/ReactDebugTool.js

@@ -263,6 +263,7 @@ var ReactDebugTool = {
   },
   onSetChildren(debugID, childDebugIDs) {
     checkDebugID(debugID);
+    childDebugIDs.forEach(checkDebugID);
     emitEvent('onSetChildren', debugID, childDebugIDs);
   },
   onSetOwner(debugID, ownerDebugID) {

src/renderers/shared/devtools/__tests__/ReactComponentTreeDevtool-test.js

@@ -1716,103 +1716,137 @@ describe('ReactComponentTreeDevtool', () => {
     expect(ReactComponentTreeTestUtils.getRegisteredDisplayNames()).toEqual([]);
   });
 
-  it('creates current stack addenda', () => {
-    function getAddendum(element) {
-      var addendum = ReactComponentTreeDevtool.getCurrentStackAddendum(element);
-      return addendum.replace(/\(at .+?:\d+\)/g, '(at **)');
-    }
-
-    var Anon = React.createClass({displayName: null, render: () => null});
-    var Orange = React.createClass({render: () => null});
-
-    expect(getAddendum()).toBe(
-      ''
-    );
-    expect(getAddendum(<div />)).toBe(
-      '\n    in div (at **)'
-    );
-    expect(getAddendum(<Anon />)).toBe(
-      '\n    in Unknown (at **)'
-    );
-    expect(getAddendum(<Orange />)).toBe(
-      '\n    in Orange (at **)'
-    );
-    expect(getAddendum(React.createElement(Orange))).toBe(
-      '\n    in Orange'
-    );
-
-    var renders = 0;
-    var rOwnedByQ;
-
-    function Q() {
-      return (rOwnedByQ = React.createElement(R));
-    }
-    function R() {
-      return <div><S /></div>;
-    }
-    class S extends React.Component {
-      componentDidMount() {
-        // Check that the parent path is still fetched when only S itself is on
-        // the stack.
-        this.forceUpdate();
+  describe('stack addenda', () => {
+    it('gets created', () => {
+      function getAddendum(element) {
+        var addendum = ReactComponentTreeDevtool.getCurrentStackAddendum(element);
+        return addendum.replace(/\(at .+?:\d+\)/g, '(at **)');
       }
-      render() {
-        expect(getAddendum()).toBe(
-          '\n    in S (at **)' +
-          '\n    in div (at **)' +
-          '\n    in R (created by Q)' +
-          '\n    in Q (at **)'
-        );
-        expect(getAddendum(<span />)).toBe(
-          '\n    in span (at **)' +
-          '\n    in S (at **)' +
-          '\n    in div (at **)' +
-          '\n    in R (created by Q)' +
-          '\n    in Q (at **)'
-        );
-        expect(getAddendum(React.createElement('span'))).toBe(
-          '\n    in span (created by S)' +
-          '\n    in S (at **)' +
-          '\n    in div (at **)' +
-          '\n    in R (created by Q)' +
-          '\n    in Q (at **)'
-        );
-        renders++;
-        return null;
+
+      var Anon = React.createClass({displayName: null, render: () => null});
+      var Orange = React.createClass({render: () => null});
+
+      expect(getAddendum()).toBe(
+        ''
+      );
+      expect(getAddendum(<div />)).toBe(
+        '\n    in div (at **)'
+      );
+      expect(getAddendum(<Anon />)).toBe(
+        '\n    in Unknown (at **)'
+      );
+      expect(getAddendum(<Orange />)).toBe(
+        '\n    in Orange (at **)'
+      );
+      expect(getAddendum(React.createElement(Orange))).toBe(
+        '\n    in Orange'
+      );
+
+      var renders = 0;
+      var rOwnedByQ;
+
+      function Q() {
+        return (rOwnedByQ = React.createElement(R));
       }
-    }
-    ReactDOM.render(<Q />, document.createElement('div'));
-    expect(renders).toBe(2);
-
-    // Make sure owner is fetched for the top element too.
-    expect(getAddendum(rOwnedByQ)).toBe(
-      '\n    in R (created by Q)'
-    );
-  });
-
-  it('creates stack addenda by ID', () => {
-    function getAddendum(id) {
-      var addendum = ReactComponentTreeDevtool.getStackAddendumByID(id);
-      return addendum.replace(/\(at .+?:\d+\)/g, '(at **)');
-    }
-
-    class Q extends React.Component {
-      render() {
-        return null;
+      function R() {
+        return <div><S /></div>;
       }
-    }
+      class S extends React.Component {
+        componentDidMount() {
+          // Check that the parent path is still fetched when only S itself is on
+          // the stack.
+          this.forceUpdate();
+        }
+        render() {
+          expect(getAddendum()).toBe(
+            '\n    in S (at **)' +
+            '\n    in div (at **)' +
+            '\n    in R (created by Q)' +
+            '\n    in Q (at **)'
+          );
+          expect(getAddendum(<span />)).toBe(
+            '\n    in span (at **)' +
+            '\n    in S (at **)' +
+            '\n    in div (at **)' +
+            '\n    in R (created by Q)' +
+            '\n    in Q (at **)'
+          );
+          expect(getAddendum(React.createElement('span'))).toBe(
+            '\n    in span (created by S)' +
+            '\n    in S (at **)' +
+            '\n    in div (at **)' +
+            '\n    in R (created by Q)' +
+            '\n    in Q (at **)'
+          );
+          renders++;
+          return null;
+        }
+      }
+      ReactDOM.render(<Q />, document.createElement('div'));
+      expect(renders).toBe(2);
 
-    var q = ReactDOM.render(<Q />, document.createElement('div'));
-    expect(getAddendum(ReactInstanceMap.get(q)._debugID)).toBe(
-      '\n    in Q (at **)'
-    );
+      // Make sure owner is fetched for the top element too.
+      expect(getAddendum(rOwnedByQ)).toBe(
+        '\n    in R (created by Q)'
+      );
+    });
 
-    spyOn(console, 'error');
-    getAddendum(-17);
-    expect(console.error.calls.count()).toBe(1);
-    expect(console.error.calls.argsFor(0)[0]).toBe(
-      'Warning: ReactComponentTreeDevtool: Missing React element for debugID ' +
-      '-17 when building stack'
-    );
-  });
+    it('can be retrieved by ID', () => {
+      function getAddendum(id) {
+        var addendum = ReactComponentTreeDevtool.getStackAddendumByID(id);
+        return addendum.replace(/\(at .+?:\d+\)/g, '(at **)');
+      }
+
+      class Q extends React.Component {
+        render() {
+          return null;
+        }
+      }
+
+      var q = ReactDOM.render(<Q />, document.createElement('div'));
+      expect(getAddendum(ReactInstanceMap.get(q)._debugID)).toBe(
+        '\n    in Q (at **)'
+      );
+
+      spyOn(console, 'error');
+      getAddendum(-17);
+      expect(console.error.calls.count()).toBe(1);
+      expect(console.error.calls.argsFor(0)[0]).toBe(
+        'Warning: ReactComponentTreeDevtool: Missing React element for ' +
+        'debugID -17 when building stack'
+      );
+    });
+
+    it('is created during mounting', () => {
+      // https://github.com/facebook/react/issues/7187
+      var el = document.createElement('div');
+      var portalEl = document.createElement('div');
+      class Foo extends React.Component {
+        componentWillMount() {
+          ReactDOM.render(<div />, portalEl);
+        }
+        render() {
+          return <div><div /></div>;
+        }
+      }
+      ReactDOM.render(<Foo />, el);
+    });
+
+    it('is created when calling renderToString during render', () => {
+      // https://github.com/facebook/react/issues/7190
+      var el = document.createElement('div');
+      class Foo extends React.Component {
+        render() {
+          return (
+            <div>
+              <div>
+                {ReactDOMServer.renderToString(<div />)}
+              </div>
+            </div>
+          );
+        }
+      }
+      ReactDOM.render(<Foo />, el);
+    });
+  })
 });

src/renderers/shared/stack/reconciler/ReactChildReconciler.js

@@ -34,7 +34,7 @@ function instantiateChild(childInstances, child, name, selfDebugID) {
     );
   }
   if (child != null && keyUnique) {
-    childInstances[name] = instantiateReactComponent(child);
+    childInstances[name] = instantiateReactComponent(child, true);
   }
 }
 
@@ -128,7 +128,7 @@ var ReactChildReconciler = {
           ReactReconciler.unmountComponent(prevChild, false);
         }
         // The child must be instantiated before it's mounted.
-        var nextChildInstance = instantiateReactComponent(nextElement);
+        var nextChildInstance = instantiateReactComponent(nextElement, true);
         nextChildren[name] = nextChildInstance;
         // Creating mount image now ensures refs are resolved in right order
         // (see https://github.com/facebook/react/pull/7101 for explanation).

src/renderers/shared/stack/reconciler/ReactCompositeComponent.js

@@ -473,9 +473,11 @@ var ReactCompositeComponentMixin = {
       renderedElement = this._renderValidatedComponent();
     }
 
-    this._renderedNodeType = ReactNodeTypes.getType(renderedElement);
+    var nodeType = ReactNodeTypes.getType(renderedElement);
+    this._renderedNodeType = nodeType;
     var child = this._instantiateReactComponent(
-      renderedElement
+      renderedElement,
+      nodeType !== ReactNodeTypes.EMPTY /* shouldHaveDebugID */
     );
     this._renderedComponent = child;
     if (__DEV__) {
@@ -990,9 +992,11 @@ var ReactCompositeComponentMixin = {
       var oldHostNode = ReactReconciler.getHostNode(prevComponentInstance);
       ReactReconciler.unmountComponent(prevComponentInstance, false);
 
-      this._renderedNodeType = ReactNodeTypes.getType(nextRenderedElement);
+      var nodeType = ReactNodeTypes.getType(nextRenderedElement);
+      this._renderedNodeType = nodeType;
       var child = this._instantiateReactComponent(
-        nextRenderedElement
+        nextRenderedElement,
+        nodeType !== ReactNodeTypes.EMPTY /* shouldHaveDebugID */
       );
       this._renderedComponent = child;
       if (__DEV__) {

src/renderers/shared/stack/reconciler/ReactMultiChild.js

@@ -162,10 +162,15 @@ if (__DEV__) {
     }
   };
   setChildrenForInstrumentation = function(children) {
-    ReactInstrumentation.debugTool.onSetChildren(
-      getDebugID(this),
-      children ? Object.keys(children).map(key => children[key]._debugID) : []
-    );
+    var debugID = getDebugID(this);
+    // TODO: React Native empty components are also multichild.
+    // This means they still get into this method but don't have _debugID.
+    if (debugID !== 0) {
+      ReactInstrumentation.debugTool.onSetChildren(
+        debugID,
+        children ? Object.keys(children).map(key => children[key]._debugID) : []
+      );
+    }
   };
 }
 

src/renderers/shared/stack/reconciler/instantiateReactComponent.js

@@ -78,14 +78,14 @@ var nextDebugID = 1;
  * Given a ReactNode, create an instance that will actually be mounted.
  *
  * @param {ReactNode} node
+ * @param {boolean} shouldHaveDebugID
  * @return {object} A new instance of the element's constructor.
  * @protected
  */
-function instantiateReactComponent(node) {
+function instantiateReactComponent(node, shouldHaveDebugID) {
   var instance;
 
-  var isEmpty = node === null || node === false;
-  if (isEmpty) {
+  if (node === null || node === false) {
     instance = ReactEmptyComponent.create(instantiateReactComponent);
   } else if (typeof node === 'object') {
     var element = node;
@@ -141,16 +141,17 @@ function instantiateReactComponent(node) {
   instance._mountImage = null;
 
   if (__DEV__) {
-    var debugID = isEmpty ? 0 : nextDebugID++;
-    instance._debugID = debugID;
-
-    if (debugID !== 0) {
+    if (shouldHaveDebugID) {
+      var debugID = nextDebugID++;
+      instance._debugID = debugID;
       var displayName = getDisplayName(instance);
       ReactInstrumentation.debugTool.onSetDisplayName(debugID, displayName);
       var owner = node && node._owner;
       if (owner) {
         ReactInstrumentation.debugTool.onSetOwner(debugID, owner._debugID);
       }
+    } else {
+      instance._debugID = 0;
     }
   }
 

src/renderers/testing/ReactTestMount.js

@@ -120,16 +120,10 @@ var ReactHostMount = {
     //   }
     // }
 
-    var instance = instantiateReactComponent(nextWrappedElement);
+    var instance = instantiateReactComponent(nextWrappedElement, false);
 
     if (__DEV__) {
-      // Mute future events from the top level wrapper.
-      // It is an implementation detail that devtools should not know about.
-      instance._debugID = 0;
-
-      if (__DEV__) {
-        ReactInstrumentation.debugTool.onBeginFlush();
-      }
+      ReactInstrumentation.debugTool.onBeginFlush();
     }
 
     // The initial render is synchronous but any updates that happen during