Remove unnecessary indirection and events from the hooks (#7464)

* Remove unnecessary indirection from the tree hook * Replace onSetDisplayName, onSetOwner, onSetText with one event Less events is better. onSetDisplayName, onSetOwner, and onSetText only existed because we didn't initially track elements. * Remove unused variables
facebook · Aug 10, 2016 · 1f31357 · 1f31357
1 parent afa27bc
commit 1f31357
Show file tree

Hide file tree

Showing 7 changed files with 108 additions and 137 deletions.
diff --git a/src/isomorphic/hooks/ReactComponentTreeHook.js b/src/isomorphic/hooks/ReactComponentTreeHook.js
@@ -39,21 +39,16 @@ function remove(id) {
   delete itemByKey[key];
 }
 
-function update(id, updater) {
+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,
-    };
-  }
-  updater(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,88 +101,93 @@ function describeID(id) {
 }
 
 var ReactComponentTreeHook = {
-  onSetDisplayName(id, displayName) {
-    update(id, item => 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
-        );
-      });
-    });
-  },
-
-  onSetOwner(id, ownerID) {
-    update(id, item => item.ownerID = ownerID);
+    var item = get(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.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(
+        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
+      );
+    }
   },
 
   onSetParent(id, parentID) {
-    update(id, item => item.parentID = parentID);
+    var item = get(id);
+    item.parentID = parentID;
   },
 
-  onSetText(id, text) {
-    update(id, item => item.text = text);
+  onInstantiateComponent(id, element) {
+    create(id, element);
   },
 
   onBeforeMountComponent(id, element) {
-    update(id, item => item.element = element);
+    var item = get(id);
+    item.element = element;
   },
 
   onBeforeUpdateComponent(id, element) {
-    update(id, item => item.element = element);
+    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) {
-    update(id, item => item.isMounted = true);
+    var item = get(id);
+    item.isMounted = true;
   },
 
   onMountRootComponent(id) {
     rootIDs[id] = true;
   },
 
   onUpdateComponent(id) {
-    update(id, item => item.updateCount++);
+    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++;
   },
 
   onUnmountComponent(id) {
-    update(id, item => item.isMounted = false);
+    var item = get(id);
+    item.isMounted = false;
     unmountedIDs[id] = true;
     delete rootIDs[id];
   },
@@ -234,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) {
@@ -244,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) {
@@ -261,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
@@ -270,16 +270,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
@@ -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');
@@ -67,8 +66,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 +139,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
@@ -11,7 +11,6 @@
 
 'use strict';
 
-var ReactInstrumentation = require('ReactInstrumentation');
 var ReactNativeComponentTree = require('ReactNativeComponentTree');
 var ReactNativeTagHandles = require('ReactNativeTagHandles');
 var UIManager = require('UIManager');
@@ -29,10 +28,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 +65,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
@@ -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
@@ -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;
     }