From c8a7988bf9e59df6db89192f87c22a550fc10d49 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 11 May 2016 23:27:45 +0100 Subject: [PATCH 1/6] Add a failing test case for #6742 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getOperations() blows up when replacing null with a native because the null component has debugID of 0 that isn’t registered in the component tree. --- src/isomorphic/__tests__/ReactPerf-test.js | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/isomorphic/__tests__/ReactPerf-test.js b/src/isomorphic/__tests__/ReactPerf-test.js index 98df00938b06..456ce9011eea 100644 --- a/src/isomorphic/__tests__/ReactPerf-test.js +++ b/src/isomorphic/__tests__/ReactPerf-test.js @@ -67,6 +67,13 @@ describe('ReactPerf', function() { ReactPerf.start(); fn(); ReactPerf.stop(); + + // Make sure none of the methods crash. + ReactPerf.getWasted(); + ReactPerf.getInclusive(); + ReactPerf.getExclusive(); + ReactPerf.getOperations(); + return ReactPerf.getLastMeasurements(); } @@ -208,6 +215,32 @@ describe('ReactPerf', function() { }); }); + it('should not count replacing null with a native as waste', function() { + var element = null; + function Foo () { + return element; + } + var container = document.createElement('div'); + ReactDOM.render(, container); + expectNoWaste(() => { + element =
; + ReactDOM.render(, container); + }); + }); + + it('should not count replacing a native with null as waste', function() { + var element =
; + function Foo () { + return element; + } + var container = document.createElement('div'); + ReactDOM.render(, container); + expectNoWaste(() => { + element = null; + ReactDOM.render(, container); + }); + }); + it('warns once when using getMeasurementsSummaryMap', function() { var measurements = measure(() => {}); spyOn(console, 'error'); From 0cd1d3b5a252ec99b4452c0d3e91a952dea780ff Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 11 May 2016 23:31:48 +0100 Subject: [PATCH 2/6] Allow empty components in the native operation logs Fixes #6742 --- src/isomorphic/ReactPerf.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/isomorphic/ReactPerf.js b/src/isomorphic/ReactPerf.js index 4c631a7f13e4..f89de6764c1a 100644 --- a/src/isomorphic/ReactPerf.js +++ b/src/isomorphic/ReactPerf.js @@ -236,11 +236,17 @@ function getWasted(flushHistory = getFlushHistory()) { function getOperations(flushHistory = getFlushHistory()) { var stats = []; + flushHistory.forEach((flush, flushIndex) => { var {operations, treeSnapshot} = flush; + operations.forEach(operation => { var {instanceID, type, payload} = operation; - var {displayName, ownerID} = treeSnapshot[instanceID]; + var {displayName, ownerID} = instanceID !== 0 ? + treeSnapshot[instanceID] : + // Empty components may appear in some operation logs. + {displayName: '#empty', ownerID: null}; + var owner = treeSnapshot[ownerID]; var key = (owner ? owner.displayName + ' > ' : '') + displayName; From 1559111bf9b1dbb355bf89037c90f1abf8226108 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 11 May 2016 23:58:32 +0100 Subject: [PATCH 3/6] Switching between null and a native should not be counted as a waste --- src/isomorphic/__tests__/ReactPerf-test.js | 4 +- ...ReactNativeOperationHistoryDevtool-test.js | 42 +++++++++++++++++++ .../dom/client/utils/DOMChildrenOperations.js | 18 +++++--- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/isomorphic/__tests__/ReactPerf-test.js b/src/isomorphic/__tests__/ReactPerf-test.js index 456ce9011eea..c81a4e82a79d 100644 --- a/src/isomorphic/__tests__/ReactPerf-test.js +++ b/src/isomorphic/__tests__/ReactPerf-test.js @@ -217,7 +217,7 @@ describe('ReactPerf', function() { it('should not count replacing null with a native as waste', function() { var element = null; - function Foo () { + function Foo() { return element; } var container = document.createElement('div'); @@ -230,7 +230,7 @@ describe('ReactPerf', function() { it('should not count replacing a native with null as waste', function() { var element =
; - function Foo () { + function Foo() { return element; } var container = document.createElement('div'); diff --git a/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js b/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js index 667e0f398184..879f203a35e9 100644 --- a/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js +++ b/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js @@ -80,6 +80,27 @@ describe('ReactNativeOperationHistoryDevtool', () => { '', }]); }); + + it('gets recorded when a native is mounted deeply instead of null', () => { + var element; + function Foo() { + return element; + } + + var node = document.createElement('div'); + element = null; + ReactDOM.render(, node); + + ReactNativeOperationHistoryDevtool.clearHistory(); + element = ; + ReactDOM.render(, node); + var inst = ReactDOMComponentTree.getInstanceFromNode(node.firstChild); + assertHistoryMatches([{ + instanceID: inst._debugID, + type: 'mount', + payload: 'SPAN', + }]); + }); }); describe('update styles', () => { @@ -500,6 +521,27 @@ describe('ReactNativeOperationHistoryDevtool', () => { }]); }); + it('gets recorded when composite renders to null after a native', () => { + var element; + function Foo() { + return element; + } + + var node = document.createElement('div'); + element = ; + ReactDOM.render(, node); + var inst = ReactDOMComponentTree.getInstanceFromNode(node.firstChild); + + ReactNativeOperationHistoryDevtool.clearHistory(); + element = null; + ReactDOM.render(, node); + assertHistoryMatches([{ + instanceID: inst._debugID, + type: 'replace with', + payload: '#comment', + }]); + }); + it('gets ignored if the type has not changed', () => { var element; function Foo() { diff --git a/src/renderers/dom/client/utils/DOMChildrenOperations.js b/src/renderers/dom/client/utils/DOMChildrenOperations.js index 303cd94b3050..f213c9e7830a 100644 --- a/src/renderers/dom/client/utils/DOMChildrenOperations.js +++ b/src/renderers/dom/client/utils/DOMChildrenOperations.js @@ -135,11 +135,19 @@ var dangerouslyReplaceNodeWithMarkup = Danger.dangerouslyReplaceNodeWithMarkup; if (__DEV__) { dangerouslyReplaceNodeWithMarkup = function(oldChild, markup, prevInstance) { Danger.dangerouslyReplaceNodeWithMarkup(oldChild, markup); - ReactInstrumentation.debugTool.onNativeOperation( - prevInstance._debugID, - 'replace with', - markup.toString() - ); + if (prevInstance._debugID !== 0) { + ReactInstrumentation.debugTool.onNativeOperation( + prevInstance._debugID, + 'replace with', + markup.toString() + ); + } else { + ReactInstrumentation.debugTool.onNativeOperation( + ReactDOMComponentTree.getInstanceFromNode(markup.node)._debugID, + 'mount', + markup.toString() + ); + } }; } From d80723b237dfb37356c9eed241c45c1fd7938adb Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 12 May 2016 00:45:05 +0100 Subject: [PATCH 4/6] Check for missing debugID in ReactDebugTool TopLevelWrapper and empty components have ID of 0. These are implementation details and we don't want to leak them to devtools. --- src/isomorphic/ReactDebugTool.js | 17 +++++++++++++++ ...ReactNativeOperationHistoryDevtool-test.js | 21 ++++++++++++------- src/renderers/dom/client/ReactMount.js | 13 +++++++----- .../__tests__/ReactDOMIDOperations-test.js | 2 +- .../dom/client/utils/DOMChildrenOperations.js | 13 +++++++----- .../__tests__/DOMPropertyOperations-test.js | 14 ++++++++----- .../reconciler/instantiateReactComponent.js | 12 ++++++----- src/test/ReactTestUtils.js | 4 ++++ 8 files changed, 68 insertions(+), 28 deletions(-) diff --git a/src/isomorphic/ReactDebugTool.js b/src/isomorphic/ReactDebugTool.js index 8fbc0341f804..005378b76d76 100644 --- a/src/isomorphic/ReactDebugTool.js +++ b/src/isomorphic/ReactDebugTool.js @@ -90,6 +90,10 @@ function resetMeasurements() { } } +function checkDebugID(debugID) { + warning(debugID, 'ReactDebugTool: debugID may not be empty.'); +} + var ReactDebugTool = { addDevtool(devtool) { eventHandlers.push(devtool); @@ -143,6 +147,7 @@ var ReactDebugTool = { emitEvent('onEndFlush'); }, onBeginLifeCycleTimer(debugID, timerType) { + checkDebugID(debugID); emitEvent('onBeginLifeCycleTimer', debugID, timerType); if (__DEV__) { if (isProfiling && currentFlushNesting > 0) { @@ -162,6 +167,7 @@ var ReactDebugTool = { } }, onEndLifeCycleTimer(debugID, timerType) { + checkDebugID(debugID); if (__DEV__) { if (isProfiling && currentFlushNesting > 0) { warning( @@ -186,9 +192,11 @@ var ReactDebugTool = { emitEvent('onEndLifeCycleTimer', debugID, timerType); }, onBeginReconcilerTimer(debugID, timerType) { + checkDebugID(debugID); emitEvent('onBeginReconcilerTimer', debugID, timerType); }, onEndReconcilerTimer(debugID, timerType) { + checkDebugID(debugID); emitEvent('onEndReconcilerTimer', debugID, timerType); }, onBeginProcessingChildContext() { @@ -198,33 +206,42 @@ var ReactDebugTool = { emitEvent('onEndProcessingChildContext'); }, onNativeOperation(debugID, type, payload) { + checkDebugID(debugID); emitEvent('onNativeOperation', debugID, type, payload); }, onSetState() { emitEvent('onSetState'); }, onSetDisplayName(debugID, displayName) { + checkDebugID(debugID); emitEvent('onSetDisplayName', debugID, displayName); }, onSetChildren(debugID, childDebugIDs) { + checkDebugID(debugID); emitEvent('onSetChildren', debugID, childDebugIDs); }, onSetOwner(debugID, ownerDebugID) { + checkDebugID(debugID); emitEvent('onSetOwner', debugID, ownerDebugID); }, onSetText(debugID, text) { + checkDebugID(debugID); emitEvent('onSetText', debugID, text); }, onMountRootComponent(debugID) { + checkDebugID(debugID); emitEvent('onMountRootComponent', debugID); }, onMountComponent(debugID) { + checkDebugID(debugID); emitEvent('onMountComponent', debugID); }, onUpdateComponent(debugID) { + checkDebugID(debugID); emitEvent('onUpdateComponent', debugID); }, onUnmountComponent(debugID) { + checkDebugID(debugID); emitEvent('onUnmountComponent', debugID); }, onTestEvent() { diff --git a/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js b/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js index 879f203a35e9..a5f1c5101e8b 100644 --- a/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js +++ b/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js @@ -72,13 +72,17 @@ describe('ReactNativeOperationHistoryDevtool', () => { var node = document.createElement('div'); ReactDOM.render(, node); var inst = ReactDOMComponentTree.getInstanceFromNode(node.firstChild); - assertHistoryMatches([{ - instanceID: inst._debugID, - type: 'mount', - payload: ReactDOMFeatureFlags.useCreateElement ? - '#comment' : - '', - }]); + + if (ReactDOMFeatureFlags.useCreateElement) { + // Empty DOM components should be invisible to devtools. + assertHistoryMatches([]); + } else { + assertHistoryMatches([{ + instanceID: inst._debugID, + type: 'mount', + payload: '', + }]); + } }); it('gets recorded when a native is mounted deeply instead of null', () => { @@ -95,6 +99,9 @@ describe('ReactNativeOperationHistoryDevtool', () => { element = ; ReactDOM.render(, node); var inst = ReactDOMComponentTree.getInstanceFromNode(node.firstChild); + + // Since empty components should be invisible to devtools, + // we record a "mount" event rather than a "replace with". assertHistoryMatches([{ instanceID: inst._debugID, type: 'mount', diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index eebc607c131f..6ec60a21fa91 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -701,11 +701,14 @@ var ReactMount = { } if (__DEV__) { - ReactInstrumentation.debugTool.onNativeOperation( - ReactDOMComponentTree.getInstanceFromNode(container.firstChild)._debugID, - 'mount', - markup.toString() - ); + var nativeNode = ReactDOMComponentTree.getInstanceFromNode(container.firstChild); + if (nativeNode._debugID !== 0) { + ReactInstrumentation.debugTool.onNativeOperation( + nativeNode._debugID, + 'mount', + markup.toString() + ); + } } }, }; diff --git a/src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js b/src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js index 1e5c05a6aaef..431a21e0f2b3 100644 --- a/src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js +++ b/src/renderers/dom/client/__tests__/ReactDOMIDOperations-test.js @@ -18,7 +18,7 @@ describe('ReactDOMIDOperations', function() { it('should update innerHTML and preserve whitespace', function() { var stubNode = document.createElement('div'); - var stubInstance = {}; + var stubInstance = {_debugID: 1}; ReactDOMComponentTree.precacheNode(stubInstance, stubNode); var html = '\n \t \n testContent \t \n \t'; diff --git a/src/renderers/dom/client/utils/DOMChildrenOperations.js b/src/renderers/dom/client/utils/DOMChildrenOperations.js index f213c9e7830a..4f0da4b7cce9 100644 --- a/src/renderers/dom/client/utils/DOMChildrenOperations.js +++ b/src/renderers/dom/client/utils/DOMChildrenOperations.js @@ -142,11 +142,14 @@ if (__DEV__) { markup.toString() ); } else { - ReactInstrumentation.debugTool.onNativeOperation( - ReactDOMComponentTree.getInstanceFromNode(markup.node)._debugID, - 'mount', - markup.toString() - ); + var nextInstance = ReactDOMComponentTree.getInstanceFromNode(markup.node); + if (nextInstance._debugID !== 0) { + ReactInstrumentation.debugTool.onNativeOperation( + nextInstance._debugID, + 'mount', + markup.toString() + ); + } } }; } diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js index 4f8f82cb1043..4200d3767889 100644 --- a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js @@ -174,10 +174,12 @@ describe('DOMPropertyOperations', function() { describe('setValueForProperty', function() { var stubNode; + var stubInstance; beforeEach(function() { stubNode = document.createElement('div'); - ReactDOMComponentTree.precacheNode({}, stubNode); + stubInstance = {_debugID: 1}; + ReactDOMComponentTree.precacheNode(stubInstance, stubNode); }); it('should set values as properties by default', function() { @@ -226,7 +228,7 @@ describe('DOMPropertyOperations', function() { it('should not remove empty attributes for special properties', function() { stubNode = document.createElement('input'); - ReactDOMComponentTree.precacheNode({}, stubNode); + ReactDOMComponentTree.precacheNode(stubInstance, stubNode); DOMPropertyOperations.setValueForProperty(stubNode, 'value', ''); // JSDOM does not behave correctly for attributes/properties @@ -348,10 +350,12 @@ describe('DOMPropertyOperations', function() { describe('deleteValueForProperty', function() { var stubNode; + var stubInstance; beforeEach(function() { stubNode = document.createElement('div'); - ReactDOMComponentTree.precacheNode({}, stubNode); + stubInstance = {_debugID: 1}; + ReactDOMComponentTree.precacheNode(stubInstance, stubNode); }); it('should remove attributes for normal properties', function() { @@ -367,7 +371,7 @@ describe('DOMPropertyOperations', function() { it('should not remove attributes for special properties', function() { stubNode = document.createElement('input'); - ReactDOMComponentTree.precacheNode({}, stubNode); + ReactDOMComponentTree.precacheNode(stubInstance, stubNode); stubNode.setAttribute('value', 'foo'); @@ -379,7 +383,7 @@ describe('DOMPropertyOperations', function() { it('should not leave all options selected when deleting multiple', function() { stubNode = document.createElement('select'); - ReactDOMComponentTree.precacheNode({}, stubNode); + ReactDOMComponentTree.precacheNode(stubInstance, stubNode); stubNode.multiple = true; stubNode.appendChild(document.createElement('option')); diff --git a/src/renderers/shared/stack/reconciler/instantiateReactComponent.js b/src/renderers/shared/stack/reconciler/instantiateReactComponent.js index 968c2991f73d..0632a3c4d5a2 100644 --- a/src/renderers/shared/stack/reconciler/instantiateReactComponent.js +++ b/src/renderers/shared/stack/reconciler/instantiateReactComponent.js @@ -139,11 +139,13 @@ function instantiateReactComponent(node) { var debugID = isEmpty ? 0 : 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); + if (debugID !== 0) { + var displayName = getDisplayName(instance); + ReactInstrumentation.debugTool.onSetDisplayName(debugID, displayName); + var owner = node && node._owner; + if (owner) { + ReactInstrumentation.debugTool.onSetOwner(debugID, owner._debugID); + } } } diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index 9937bf06e7c4..a9093a49a241 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -376,9 +376,12 @@ ReactShallowRenderer.prototype.getMountedInstance = function() { return this._instance ? this._instance._instance : null; }; +var nextDebugID = 0; + var NoopInternalComponent = function(element) { this._renderedOutput = element; this._currentElement = element; + this._debugID = nextDebugID++; }; NoopInternalComponent.prototype = { @@ -404,6 +407,7 @@ NoopInternalComponent.prototype = { }; var ShallowComponentWrapper = function(element) { + this._debugID = nextDebugID++; this.construct(element); }; Object.assign( From f0594d2792a699c35c8620ae81d010b2bb88f4d1 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 12 May 2016 00:49:35 +0100 Subject: [PATCH 5/6] Remove the protective clause that is now unnecessary Instead, we fixed the callsites to stop emitting events for empty components. --- src/isomorphic/ReactPerf.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/isomorphic/ReactPerf.js b/src/isomorphic/ReactPerf.js index f89de6764c1a..4c631a7f13e4 100644 --- a/src/isomorphic/ReactPerf.js +++ b/src/isomorphic/ReactPerf.js @@ -236,17 +236,11 @@ function getWasted(flushHistory = getFlushHistory()) { function getOperations(flushHistory = getFlushHistory()) { var stats = []; - flushHistory.forEach((flush, flushIndex) => { var {operations, treeSnapshot} = flush; - operations.forEach(operation => { var {instanceID, type, payload} = operation; - var {displayName, ownerID} = instanceID !== 0 ? - treeSnapshot[instanceID] : - // Empty components may appear in some operation logs. - {displayName: '#empty', ownerID: null}; - + var {displayName, ownerID} = treeSnapshot[instanceID]; var owner = treeSnapshot[ownerID]; var key = (owner ? owner.displayName + ' > ' : '') + displayName; From 9cebc2663888573b4b993a4a9096f7fac8c6a5a3 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 12 May 2016 02:32:10 +0100 Subject: [PATCH 6/6] Fix a failing test and count IDs from one --- .../ReactNativeOperationHistoryDevtool-test.js | 15 +++------------ src/test/ReactTestUtils.js | 2 +- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js b/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js index a5f1c5101e8b..73ee7511d1ec 100644 --- a/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js +++ b/src/isomorphic/devtools/__tests__/ReactNativeOperationHistoryDevtool-test.js @@ -65,24 +65,15 @@ describe('ReactNativeOperationHistoryDevtool', () => { }]); }); - it('gets recorded for composite roots that return null', () => { + it('gets ignored for composite roots that return null', () => { function Foo() { return null; } var node = document.createElement('div'); ReactDOM.render(, node); - var inst = ReactDOMComponentTree.getInstanceFromNode(node.firstChild); - if (ReactDOMFeatureFlags.useCreateElement) { - // Empty DOM components should be invisible to devtools. - assertHistoryMatches([]); - } else { - assertHistoryMatches([{ - instanceID: inst._debugID, - type: 'mount', - payload: '', - }]); - } + // Empty DOM components should be invisible to devtools. + assertHistoryMatches([]); }); it('gets recorded when a native is mounted deeply instead of null', () => { diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index a9093a49a241..af5b860f8569 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -376,7 +376,7 @@ ReactShallowRenderer.prototype.getMountedInstance = function() { return this._instance ? this._instance._instance : null; }; -var nextDebugID = 0; +var nextDebugID = 1; var NoopInternalComponent = function(element) { this._renderedOutput = element;