From 9ea867ff432dabb3f4fbe6dab81018c4542d8a33 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 5 Jul 2016 20:50:53 +0100 Subject: [PATCH 1/3] Add failing tests for #7187 and #7190 --- .../ReactComponentTreeDevtool-test.js | 216 ++++++++++-------- 1 file changed, 125 insertions(+), 91 deletions(-) diff --git a/src/renderers/shared/devtools/__tests__/ReactComponentTreeDevtool-test.js b/src/renderers/shared/devtools/__tests__/ReactComponentTreeDevtool-test.js index 534e9bd913c49..f62dd2a2aacf4 100644 --- a/src/renderers/shared/devtools/__tests__/ReactComponentTreeDevtool-test.js +++ b/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 **)'); - } + describe('stack addenda', () => { + it('gets created', () => { + 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(
)).toBe( - '\n in div (at **)' - ); - expect(getAddendum()).toBe( - '\n in Unknown (at **)' - ); - expect(getAddendum()).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
; - } - class S extends React.Component { - componentDidMount() { - // Check that the parent path is still fetched when only S itself is on - // the stack. - this.forceUpdate(); + var Anon = React.createClass({displayName: null, render: () => null}); + var Orange = React.createClass({render: () => null}); + + expect(getAddendum()).toBe( + '' + ); + expect(getAddendum(
)).toBe( + '\n in div (at **)' + ); + expect(getAddendum()).toBe( + '\n in Unknown (at **)' + ); + expect(getAddendum()).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)); } - render() { - expect(getAddendum()).toBe( - '\n in S (at **)' + - '\n in div (at **)' + - '\n in R (created by Q)' + - '\n in Q (at **)' - ); - expect(getAddendum()).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; + function R() { + return
; } - } - ReactDOM.render(, document.createElement('div')); - expect(renders).toBe(2); + 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()).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(, 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)' - ); - }); + // 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 **)'); - } + 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; + class Q extends React.Component { + render() { + return null; + } } - } - var q = ReactDOM.render(, 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' - ); - }); + var q = ReactDOM.render(, 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(
, portalEl); + } + render() { + return
; + } + } + ReactDOM.render(, 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 ( +
+
+ {ReactDOMServer.renderToString(
)} +
+
+ ); + } + } + ReactDOM.render(, el); + }); + }) }); From e24dae0e60cfb102f846c481897c9f87648cd4e3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 5 Jul 2016 21:02:18 +0100 Subject: [PATCH 2/3] Pass shouldHaveDebugID flag to instantiateComponent This allows us to remove a hack that was added in #6770 and caused #7187 and #7190. --- .../devtools/ReactComponentTreeDevtool.js | 5 ----- src/renderers/dom/client/ReactMount.js | 8 +------- src/renderers/dom/server/ReactServerRendering.js | 2 +- src/renderers/native/ReactNativeMount.js | 10 ++-------- src/renderers/shared/ReactDebugTool.js | 4 ++++ .../shared/stack/reconciler/ReactChildReconciler.js | 4 ++-- .../stack/reconciler/ReactCompositeComponent.js | 8 ++------ .../shared/stack/reconciler/ReactMultiChild.js | 13 +++++++++---- .../stack/reconciler/instantiateReactComponent.js | 5 +++-- src/renderers/testing/ReactTestMount.js | 10 ++-------- 10 files changed, 26 insertions(+), 43 deletions(-) diff --git a/src/isomorphic/devtools/ReactComponentTreeDevtool.js b/src/isomorphic/devtools/ReactComponentTreeDevtool.js index fd61c2f0006b5..622bad984749f 100644 --- a/src/isomorphic/devtools/ReactComponentTreeDevtool.js +++ b/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) { diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index d7fb1b561f835..baab5099fc43e 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/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 diff --git a/src/renderers/dom/server/ReactServerRendering.js b/src/renderers/dom/server/ReactServerRendering.js index ee57e07438e98..a07bc05b48a79 100644 --- a/src/renderers/dom/server/ReactServerRendering.js +++ b/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, diff --git a/src/renderers/native/ReactNativeMount.js b/src/renderers/native/ReactNativeMount.js index d9fe2f28623fe..7e2e0fb8628f1 100644 --- a/src/renderers/native/ReactNativeMount.js +++ b/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 diff --git a/src/renderers/shared/ReactDebugTool.js b/src/renderers/shared/ReactDebugTool.js index 62113ba8de05f..1d1a33f3b7670 100644 --- a/src/renderers/shared/ReactDebugTool.js +++ b/src/renderers/shared/ReactDebugTool.js @@ -102,6 +102,9 @@ function resetMeasurements() { } function checkDebugID(debugID) { + if (!debugID) { + throw new Error('wat') + } warning(debugID, 'ReactDebugTool: debugID may not be empty.'); } @@ -263,6 +266,7 @@ var ReactDebugTool = { }, onSetChildren(debugID, childDebugIDs) { checkDebugID(debugID); + childDebugIDs.forEach(checkDebugID); emitEvent('onSetChildren', debugID, childDebugIDs); }, onSetOwner(debugID, ownerDebugID) { diff --git a/src/renderers/shared/stack/reconciler/ReactChildReconciler.js b/src/renderers/shared/stack/reconciler/ReactChildReconciler.js index 2e7479bd0d940..40f589c435234 100644 --- a/src/renderers/shared/stack/reconciler/ReactChildReconciler.js +++ b/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). diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 142598ecd4361..570469995bb95 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -474,9 +474,7 @@ var ReactCompositeComponentMixin = { } this._renderedNodeType = ReactNodeTypes.getType(renderedElement); - var child = this._instantiateReactComponent( - renderedElement - ); + var child = this._instantiateReactComponent(renderedElement, true); this._renderedComponent = child; if (__DEV__) { if (child._debugID !== 0 && this._debugID !== 0) { @@ -991,9 +989,7 @@ var ReactCompositeComponentMixin = { ReactReconciler.unmountComponent(prevComponentInstance, false); this._renderedNodeType = ReactNodeTypes.getType(nextRenderedElement); - var child = this._instantiateReactComponent( - nextRenderedElement - ); + var child = this._instantiateReactComponent(nextRenderedElement, true); this._renderedComponent = child; if (__DEV__) { if (child._debugID !== 0 && this._debugID !== 0) { diff --git a/src/renderers/shared/stack/reconciler/ReactMultiChild.js b/src/renderers/shared/stack/reconciler/ReactMultiChild.js index 0e59d32d948aa..474ec774f055d 100644 --- a/src/renderers/shared/stack/reconciler/ReactMultiChild.js +++ b/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) : [] + ); + } }; } diff --git a/src/renderers/shared/stack/reconciler/instantiateReactComponent.js b/src/renderers/shared/stack/reconciler/instantiateReactComponent.js index bbedb68cfa8b1..5adb68d8c8283 100644 --- a/src/renderers/shared/stack/reconciler/instantiateReactComponent.js +++ b/src/renderers/shared/stack/reconciler/instantiateReactComponent.js @@ -78,10 +78,11 @@ 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; @@ -141,7 +142,7 @@ function instantiateReactComponent(node) { instance._mountImage = null; if (__DEV__) { - var debugID = isEmpty ? 0 : nextDebugID++; + var debugID = (shouldHaveDebugID && !isEmpty) ? nextDebugID++ : 0; instance._debugID = debugID; if (debugID !== 0) { diff --git a/src/renderers/testing/ReactTestMount.js b/src/renderers/testing/ReactTestMount.js index 45e6cfd290ed2..cfa9325b925a3 100644 --- a/src/renderers/testing/ReactTestMount.js +++ b/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 From b7e6505501a410b46de19811d389d2b7be09e193 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 5 Jul 2016 21:45:26 +0100 Subject: [PATCH 3/3] Move logic for choosing whether to use debugID outside instantiate --- src/renderers/shared/ReactDebugTool.js | 3 --- .../stack/reconciler/ReactCompositeComponent.js | 16 ++++++++++++---- .../reconciler/instantiateReactComponent.js | 12 ++++++------ 3 files changed, 18 insertions(+), 13 deletions(-) diff --git a/src/renderers/shared/ReactDebugTool.js b/src/renderers/shared/ReactDebugTool.js index 1d1a33f3b7670..97907beadc603 100644 --- a/src/renderers/shared/ReactDebugTool.js +++ b/src/renderers/shared/ReactDebugTool.js @@ -102,9 +102,6 @@ function resetMeasurements() { } function checkDebugID(debugID) { - if (!debugID) { - throw new Error('wat') - } warning(debugID, 'ReactDebugTool: debugID may not be empty.'); } diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 570469995bb95..22bd62ff70e31 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -473,8 +473,12 @@ var ReactCompositeComponentMixin = { renderedElement = this._renderValidatedComponent(); } - this._renderedNodeType = ReactNodeTypes.getType(renderedElement); - var child = this._instantiateReactComponent(renderedElement, true); + var nodeType = ReactNodeTypes.getType(renderedElement); + this._renderedNodeType = nodeType; + var child = this._instantiateReactComponent( + renderedElement, + nodeType !== ReactNodeTypes.EMPTY /* shouldHaveDebugID */ + ); this._renderedComponent = child; if (__DEV__) { if (child._debugID !== 0 && this._debugID !== 0) { @@ -988,8 +992,12 @@ var ReactCompositeComponentMixin = { var oldHostNode = ReactReconciler.getHostNode(prevComponentInstance); ReactReconciler.unmountComponent(prevComponentInstance, false); - this._renderedNodeType = ReactNodeTypes.getType(nextRenderedElement); - var child = this._instantiateReactComponent(nextRenderedElement, true); + var nodeType = ReactNodeTypes.getType(nextRenderedElement); + this._renderedNodeType = nodeType; + var child = this._instantiateReactComponent( + nextRenderedElement, + nodeType !== ReactNodeTypes.EMPTY /* shouldHaveDebugID */ + ); this._renderedComponent = child; if (__DEV__) { if (child._debugID !== 0 && this._debugID !== 0) { diff --git a/src/renderers/shared/stack/reconciler/instantiateReactComponent.js b/src/renderers/shared/stack/reconciler/instantiateReactComponent.js index 5adb68d8c8283..658983fa0dbf8 100644 --- a/src/renderers/shared/stack/reconciler/instantiateReactComponent.js +++ b/src/renderers/shared/stack/reconciler/instantiateReactComponent.js @@ -85,8 +85,7 @@ var nextDebugID = 1; 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; @@ -142,16 +141,17 @@ function instantiateReactComponent(node, shouldHaveDebugID) { instance._mountImage = null; if (__DEV__) { - var debugID = (shouldHaveDebugID && !isEmpty) ? nextDebugID++ : 0; - 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; } }