From af0c8313c5762cc098e736cf57d28c521c189176 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 10 Aug 2016 20:46:41 +0100 Subject: [PATCH 1/3] Remove unnecessary indirection from the tree hook --- .../hooks/ReactComponentTreeHook.js | 113 ++++++++++-------- 1 file changed, 61 insertions(+), 52 deletions(-) diff --git a/src/isomorphic/hooks/ReactComponentTreeHook.js b/src/isomorphic/hooks/ReactComponentTreeHook.js index 3e57973710d7b..74114c87b85e4 100644 --- a/src/isomorphic/hooks/ReactComponentTreeHook.js +++ b/src/isomorphic/hooks/ReactComponentTreeHook.js @@ -39,7 +39,7 @@ function remove(id) { delete itemByKey[key]; } -function update(id, updater) { +function getOrCreate(id) { var key = getKeyFromID(id); if (!itemByKey[key]) { itemByKey[key] = { @@ -53,7 +53,7 @@ function update(id, updater) { updateCount: 0, }; } - updater(itemByKey[key]); + return itemByKey[key]; } function purgeDeep(id) { @@ -95,75 +95,82 @@ function describeID(id) { var ReactComponentTreeHook = { onSetDisplayName(id, displayName) { - update(id, item => item.displayName = displayName); + var item = getOrCreate(id); + item.displayName = displayName; }, onSetChildren(id, nextChildIDs) { - update(id, item => { - item.childIDs = nextChildIDs; - - nextChildIDs.forEach(nextChildID => { - var nextChild = get(nextChildID); - invariant( - nextChild, - 'Expected hook events to fire for the child ' + - 'before its parent includes it in onSetChildren().' - ); - invariant( - nextChild.displayName != null, - 'Expected onSetDisplayName() to fire for the child ' + - 'before its parent includes it in onSetChildren().' - ); - invariant( - nextChild.childIDs != null || nextChild.text != null, - 'Expected onSetChildren() or onSetText() to fire for the child ' + - 'before its parent includes it in onSetChildren().' - ); - invariant( - nextChild.isMounted, - 'Expected onMountComponent() to fire for the child ' + - 'before its parent includes it in onSetChildren().' - ); - if (nextChild.parentID == null) { - nextChild.parentID = id; - // TODO: This shouldn't be necessary but mounting a new root during in - // componentWillMount currently causes not-yet-mounted components to - // be purged from our tree data so their parent ID is missing. - } - invariant( - nextChild.parentID === id, - 'Expected onSetParent() and onSetChildren() to be consistent (%s ' + - 'has parents %s and %s).', - nextChildID, - nextChild.parentID, - id - ); - }); - }); + var item = getOrCreate(id); + item.childIDs = nextChildIDs; + + for (var i = 0; i < nextChildIDs.length; i++) { + var nextChildID = nextChildIDs[i]; + var nextChild = get(nextChildID); + invariant( + nextChild, + 'Expected hook events to fire for the child ' + + 'before its parent includes it in onSetChildren().' + ); + invariant( + nextChild.displayName != null, + 'Expected onSetDisplayName() to fire for the child ' + + 'before its parent includes it in onSetChildren().' + ); + invariant( + nextChild.childIDs != null || nextChild.text != null, + 'Expected onSetChildren() or onSetText() to fire for the child ' + + 'before its parent includes it in onSetChildren().' + ); + invariant( + nextChild.isMounted, + 'Expected onMountComponent() to fire for the child ' + + 'before its parent includes it in onSetChildren().' + ); + if (nextChild.parentID == null) { + nextChild.parentID = id; + // TODO: This shouldn't be necessary but mounting a new root during in + // componentWillMount currently causes not-yet-mounted components to + // be purged from our tree data so their parent ID is missing. + } + invariant( + nextChild.parentID === id, + 'Expected onSetParent() and onSetChildren() to be consistent (%s ' + + 'has parents %s and %s).', + nextChildID, + nextChild.parentID, + id + ); + } }, onSetOwner(id, ownerID) { - update(id, item => item.ownerID = ownerID); + var item = getOrCreate(id); + item.ownerID = ownerID; }, onSetParent(id, parentID) { - update(id, item => item.parentID = parentID); + var item = getOrCreate(id); + item.parentID = parentID; }, onSetText(id, text) { - update(id, item => item.text = text); + var item = getOrCreate(id); + item.text = text; }, onBeforeMountComponent(id, element) { - update(id, item => item.element = element); + var item = getOrCreate(id); + item.element = element; }, onBeforeUpdateComponent(id, element) { - update(id, item => item.element = element); + var item = getOrCreate(id); + item.element = element; }, onMountComponent(id) { - update(id, item => item.isMounted = true); + var item = getOrCreate(id); + item.isMounted = true; }, onMountRootComponent(id) { @@ -171,11 +178,13 @@ var ReactComponentTreeHook = { }, onUpdateComponent(id) { - update(id, item => item.updateCount++); + var item = get(id); + item.updateCount++; }, onUnmountComponent(id) { - update(id, item => item.isMounted = false); + var item = get(id); + item.isMounted = false; unmountedIDs[id] = true; delete rootIDs[id]; }, From f448285e7909828a9788114cf58a7bc188f2335d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 10 Aug 2016 21:27:14 +0100 Subject: [PATCH 2/3] Replace onSetDisplayName, onSetOwner, onSetText with one event Less events is better. onSetDisplayName, onSetOwner, and onSetText only existed because we didn't initially track elements. --- .../hooks/ReactComponentTreeHook.js | 105 ++++++++++-------- src/renderers/dom/shared/ReactDOMComponent.js | 6 +- .../dom/shared/ReactDOMTextComponent.js | 9 -- .../native/ReactNativeTextComponent.js | 10 -- src/renderers/shared/ReactDebugTool.js | 12 +- .../reconciler/instantiateReactComponent.js | 22 +--- src/test/ReactTestUtils.js | 4 +- 7 files changed, 67 insertions(+), 101 deletions(-) diff --git a/src/isomorphic/hooks/ReactComponentTreeHook.js b/src/isomorphic/hooks/ReactComponentTreeHook.js index 74114c87b85e4..c3877d608a81d 100644 --- a/src/isomorphic/hooks/ReactComponentTreeHook.js +++ b/src/isomorphic/hooks/ReactComponentTreeHook.js @@ -39,21 +39,16 @@ function remove(id) { delete itemByKey[key]; } -function getOrCreate(id) { +function create(id, element) { var key = getKeyFromID(id); - if (!itemByKey[key]) { - itemByKey[key] = { - element: null, - parentID: null, - ownerID: null, - text: null, - childIDs: [], - displayName: 'Unknown', - isMounted: false, - updateCount: 0, - }; - } - return itemByKey[key]; + itemByKey[key] = { + element, + parentID: null, + text: null, + childIDs: [], + isMounted: false, + updateCount: 0, + }; } function purgeDeep(id) { @@ -76,6 +71,18 @@ function describeComponentFrame(name, source, ownerName) { ); } +function getDisplayName(element) { + if (element == null) { + return '#empty'; + } else if (typeof element === 'string' || typeof element === 'number') { + return '#text'; + } else if (typeof element.type === 'string') { + return element.type; + } else { + return element.type.displayName || element.type.name || 'Unknown'; + } +} + function describeID(id) { var name = ReactComponentTreeHook.getDisplayName(id); var element = ReactComponentTreeHook.getElement(id); @@ -94,13 +101,8 @@ function describeID(id) { } var ReactComponentTreeHook = { - onSetDisplayName(id, displayName) { - var item = getOrCreate(id); - item.displayName = displayName; - }, - onSetChildren(id, nextChildIDs) { - var item = getOrCreate(id); + var item = get(id); item.childIDs = nextChildIDs; for (var i = 0; i < nextChildIDs.length; i++) { @@ -112,13 +114,10 @@ var ReactComponentTreeHook = { 'before its parent includes it in onSetChildren().' ); invariant( - nextChild.displayName != null, - 'Expected onSetDisplayName() to fire for the child ' + - 'before its parent includes it in onSetChildren().' - ); - invariant( - nextChild.childIDs != null || nextChild.text != null, - 'Expected onSetChildren() or onSetText() to fire for the child ' + + nextChild.childIDs != null || + typeof nextChild.element !== 'object' || + nextChild.element == null, + 'Expected onSetChildren() to fire for a container child ' + 'before its parent includes it in onSetChildren().' ); invariant( @@ -143,33 +142,32 @@ var ReactComponentTreeHook = { } }, - onSetOwner(id, ownerID) { - var item = getOrCreate(id); - item.ownerID = ownerID; - }, - onSetParent(id, parentID) { - var item = getOrCreate(id); + var item = get(id); item.parentID = parentID; }, - onSetText(id, text) { - var item = getOrCreate(id); - item.text = text; + onInstantiateComponent(id, element) { + create(id, element); }, onBeforeMountComponent(id, element) { - var item = getOrCreate(id); + var item = get(id); item.element = element; }, onBeforeUpdateComponent(id, element) { - var item = getOrCreate(id); + var item = get(id); + if (!item || !item.isMounted) { + // We may end up here as a result of setState() in componentWillUnmount(). + // In this case, ignore the element. + return; + } item.element = element; }, onMountComponent(id) { - var item = getOrCreate(id); + var item = get(id); item.isMounted = true; }, @@ -179,6 +177,11 @@ var ReactComponentTreeHook = { onUpdateComponent(id) { var item = get(id); + if (!item || !item.isMounted) { + // We may end up here as a result of setState() in componentWillUnmount(). + // In this case, ignore the element. + return; + } item.updateCount++; }, @@ -243,8 +246,11 @@ var ReactComponentTreeHook = { }, getDisplayName(id) { - var item = get(id); - return item ? item.displayName : 'Unknown'; + var element = ReactComponentTreeHook.getElement(id); + if (!element) { + return null; + } + return getDisplayName(element); }, getElement(id) { @@ -253,8 +259,11 @@ var ReactComponentTreeHook = { }, getOwnerID(id) { - var item = get(id); - return item ? item.ownerID : null; + var element = ReactComponentTreeHook.getElement(id); + if (!element || !element._owner) { + return null; + } + return element._owner._debugID; }, getParentID(id) { @@ -270,8 +279,14 @@ var ReactComponentTreeHook = { }, getText(id) { - var item = get(id); - return item ? item.text : null; + var element = ReactComponentTreeHook.getElement(id); + if (typeof element === 'string') { + return element; + } else if (typeof element === 'number') { + return '' + element; + } else { + return null; + } }, getUpdateCount(id) { diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index eb467f5935950..62d4fa8956b9c 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -272,14 +272,12 @@ if (__DEV__) { this._contentDebugID = contentDebugID; var text = '' + content; - ReactInstrumentation.debugTool.onSetDisplayName(contentDebugID, '#text'); - ReactInstrumentation.debugTool.onSetParent(contentDebugID, debugID); - ReactInstrumentation.debugTool.onSetText(contentDebugID, text); - if (hasExistingContent) { ReactInstrumentation.debugTool.onBeforeUpdateComponent(contentDebugID, content); ReactInstrumentation.debugTool.onUpdateComponent(contentDebugID); } else { + ReactInstrumentation.debugTool.onInstantiateComponent(contentDebugID, content); + ReactInstrumentation.debugTool.onSetParent(contentDebugID, debugID); ReactInstrumentation.debugTool.onBeforeMountComponent(contentDebugID, content); ReactInstrumentation.debugTool.onMountComponent(contentDebugID); ReactInstrumentation.debugTool.onSetChildren(debugID, [contentDebugID]); diff --git a/src/renderers/dom/shared/ReactDOMTextComponent.js b/src/renderers/dom/shared/ReactDOMTextComponent.js index 95eacc9c2c06a..dfd93bd0ebf6a 100644 --- a/src/renderers/dom/shared/ReactDOMTextComponent.js +++ b/src/renderers/dom/shared/ReactDOMTextComponent.js @@ -67,8 +67,6 @@ Object.assign(ReactDOMTextComponent.prototype, { context ) { if (__DEV__) { - ReactInstrumentation.debugTool.onSetText(this._debugID, this._stringText); - var parentInfo; if (hostParent != null) { parentInfo = hostParent._ancestorInfo; @@ -142,13 +140,6 @@ Object.assign(ReactDOMTextComponent.prototype, { commentNodes[1], nextStringText ); - - if (__DEV__) { - ReactInstrumentation.debugTool.onSetText( - this._debugID, - nextStringText - ); - } } } }, diff --git a/src/renderers/native/ReactNativeTextComponent.js b/src/renderers/native/ReactNativeTextComponent.js index 2195923941c15..a56aa82eb35c6 100644 --- a/src/renderers/native/ReactNativeTextComponent.js +++ b/src/renderers/native/ReactNativeTextComponent.js @@ -29,10 +29,6 @@ var ReactNativeTextComponent = function(text) { Object.assign(ReactNativeTextComponent.prototype, { mountComponent: function(transaction, hostParent, hostContainerInfo, context) { - if (__DEV__) { - ReactInstrumentation.debugTool.onSetText(this._debugID, this._stringText); - } - // TODO: hostParent should have this context already. Stop abusing context. invariant( context.isInAParentText, @@ -70,12 +66,6 @@ Object.assign(ReactNativeTextComponent.prototype, { 'RCTRawText', {text: this._stringText} ); - if (__DEV__) { - ReactInstrumentation.debugTool.onSetText( - this._debugID, - nextStringText - ); - } } } }, diff --git a/src/renderers/shared/ReactDebugTool.js b/src/renderers/shared/ReactDebugTool.js index 9be6f486fe989..217647b6d6075 100644 --- a/src/renderers/shared/ReactDebugTool.js +++ b/src/renderers/shared/ReactDebugTool.js @@ -283,26 +283,18 @@ var ReactDebugTool = { onSetState() { emitEvent('onSetState'); }, - onSetDisplayName(debugID, displayName) { - checkDebugID(debugID); - emitEvent('onSetDisplayName', debugID, displayName); - }, onSetChildren(debugID, childDebugIDs) { checkDebugID(debugID); childDebugIDs.forEach(checkDebugID); emitEvent('onSetChildren', debugID, childDebugIDs); }, - onSetOwner(debugID, ownerDebugID) { - checkDebugID(debugID); - emitEvent('onSetOwner', debugID, ownerDebugID); - }, onSetParent(debugID, parentDebugID) { checkDebugID(debugID); emitEvent('onSetParent', debugID, parentDebugID); }, - onSetText(debugID, text) { + onInstantiateComponent(debugID, element) { checkDebugID(debugID); - emitEvent('onSetText', debugID, text); + emitEvent('onInstantiateComponent', debugID, element); }, onMountRootComponent(debugID) { checkDebugID(debugID); diff --git a/src/renderers/shared/stack/reconciler/instantiateReactComponent.js b/src/renderers/shared/stack/reconciler/instantiateReactComponent.js index 658983fa0dbf8..c30952b0eed79 100644 --- a/src/renderers/shared/stack/reconciler/instantiateReactComponent.js +++ b/src/renderers/shared/stack/reconciler/instantiateReactComponent.js @@ -41,21 +41,6 @@ function getDeclarationErrorAddendum(owner) { return ''; } -function getDisplayName(instance) { - var element = instance._currentElement; - if (element == null) { - return '#empty'; - } else if (typeof element === 'string' || typeof element === 'number') { - return '#text'; - } else if (typeof element.type === 'string') { - return element.type; - } else if (instance.getName) { - return instance.getName() || 'Unknown'; - } else { - return element.type.displayName || element.type.name || 'Unknown'; - } -} - /** * Check if the type reference is a known internal type. I.e. not a user * provided composite type. @@ -144,12 +129,7 @@ function instantiateReactComponent(node, shouldHaveDebugID) { 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); - } + ReactInstrumentation.debugTool.onInstantiateComponent(debugID, node); } else { instance._debugID = 0; } diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index d454b9150daa2..bfc8e837e4ada 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -384,6 +384,7 @@ var NoopInternalComponent = function(element) { this._renderedOutput = element; this._currentElement = element; this._debugID = nextDebugID++; + ReactInstrumentation.debugTool.onInstantiateComponent(this._debugID, element); }; NoopInternalComponent.prototype = { @@ -412,8 +413,7 @@ var ShallowComponentWrapper = function(element) { // TODO: Consolidate with instantiateReactComponent if (__DEV__) { this._debugID = nextDebugID++; - var displayName = element.type.displayName || element.type.name || 'Unknown'; - ReactInstrumentation.debugTool.onSetDisplayName(this._debugID, displayName); + ReactInstrumentation.debugTool.onInstantiateComponent(this._debugID, element); } this.construct(element); From 2489f8fc1a9ab9b8e9a7ecc3559e54a9f6562117 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 11 Aug 2016 00:14:10 +0100 Subject: [PATCH 3/3] Remove unused variables --- src/renderers/dom/shared/ReactDOMComponent.js | 2 -- src/renderers/dom/shared/ReactDOMTextComponent.js | 1 - src/renderers/native/ReactNativeTextComponent.js | 1 - 3 files changed, 4 deletions(-) diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index 62d4fa8956b9c..d39c159ede47b 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -270,8 +270,6 @@ if (__DEV__) { } this._contentDebugID = contentDebugID; - var text = '' + content; - if (hasExistingContent) { ReactInstrumentation.debugTool.onBeforeUpdateComponent(contentDebugID, content); ReactInstrumentation.debugTool.onUpdateComponent(contentDebugID); diff --git a/src/renderers/dom/shared/ReactDOMTextComponent.js b/src/renderers/dom/shared/ReactDOMTextComponent.js index dfd93bd0ebf6a..fa589e7886204 100644 --- a/src/renderers/dom/shared/ReactDOMTextComponent.js +++ b/src/renderers/dom/shared/ReactDOMTextComponent.js @@ -14,7 +14,6 @@ var DOMChildrenOperations = require('DOMChildrenOperations'); var DOMLazyTree = require('DOMLazyTree'); var ReactDOMComponentTree = require('ReactDOMComponentTree'); -var ReactInstrumentation = require('ReactInstrumentation'); var escapeTextContentForBrowser = require('escapeTextContentForBrowser'); var invariant = require('invariant'); diff --git a/src/renderers/native/ReactNativeTextComponent.js b/src/renderers/native/ReactNativeTextComponent.js index a56aa82eb35c6..67a9b865fa421 100644 --- a/src/renderers/native/ReactNativeTextComponent.js +++ b/src/renderers/native/ReactNativeTextComponent.js @@ -11,7 +11,6 @@ 'use strict'; -var ReactInstrumentation = require('ReactInstrumentation'); var ReactNativeComponentTree = require('ReactNativeComponentTree'); var ReactNativeTagHandles = require('ReactNativeTagHandles'); var UIManager = require('UIManager');