Don't use closures in DevTools injection (facebook#18278)

* Don't use closures in DevTools injection Nested closures are tricky. They're not super efficient and when they share scope between multiple closures they're hard for a compiler to optimize. It's also unclear how many versions will be created. By hoisting things out an just make it simple calls the compiler can do a much better job. * Store injected hook to work around fast refresh
sophiebits · Apr 9, 2020 · b014e2d · b014e2d
1 parent 5474a83
commit b014e2d
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 83 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberDevToolsHook.js b/packages/react-reconciler/src/ReactFiberDevToolsHook.js
@@ -20,9 +20,8 @@ import {DidCapture} from './ReactSideEffectTags';
 
 declare var __REACT_DEVTOOLS_GLOBAL_HOOK__: Object | void;
 
-let onScheduleFiberRoot = null;
-let onCommitFiberRoot = null;
-let onCommitFiberUnmount = null;
+let rendererID = null;
+let injectedHook = null;
 let hasLoggedError = false;
 
 export const isDevToolsPresent =
@@ -52,66 +51,9 @@ export function injectInternals(internals: Object): boolean {
     return true;
   }
   try {
-    const rendererID = hook.inject(internals);
+    rendererID = hook.inject(internals);
     // We have successfully injected, so now it is safe to set up hooks.
-    if (__DEV__) {
-      // Only used by Fast Refresh
-      if (typeof hook.onScheduleFiberRoot === 'function') {
-        onScheduleFiberRoot = (root, children) => {
-          try {
-            hook.onScheduleFiberRoot(rendererID, root, children);
-          } catch (err) {
-            if (__DEV__ && !hasLoggedError) {
-              hasLoggedError = true;
-              console.error(
-                'React instrumentation encountered an error: %s',
-                err,
-              );
-            }
-          }
-        };
-      }
-    }
-    onCommitFiberRoot = (root, expirationTime) => {
-      try {
-        const didError = (root.current.effectTag & DidCapture) === DidCapture;
-        if (enableProfilerTimer) {
-          const currentTime = getCurrentTime();
-          const priorityLevel = inferPriorityFromExpirationTime(
-            currentTime,
-            expirationTime,
-          );
-          hook.onCommitFiberRoot(rendererID, root, priorityLevel, didError);
-        } else {
-          hook.onCommitFiberRoot(rendererID, root, undefined, didError);
-        }
-      } catch (err) {
-        if (__DEV__) {
-          if (!hasLoggedError) {
-            hasLoggedError = true;
-            console.error(
-              'React instrumentation encountered an error: %s',
-              err,
-            );
-          }
-        }
-      }
-    };
-    onCommitFiberUnmount = fiber => {
-      try {
-        hook.onCommitFiberUnmount(rendererID, fiber);
-      } catch (err) {
-        if (__DEV__) {
-          if (!hasLoggedError) {
-            hasLoggedError = true;
-            console.error(
-              'React instrumentation encountered an error: %s',
-              err,
-            );
-          }
-        }
-      }
-    };
+    injectedHook = hook;
   } catch (err) {
     // Catch all errors because it is unsafe to throw during initialization.
     if (__DEV__) {
@@ -123,19 +65,64 @@ export function injectInternals(internals: Object): boolean {
 }
 
 export function onScheduleRoot(root: FiberRoot, children: ReactNodeList) {
-  if (typeof onScheduleFiberRoot === 'function') {
-    onScheduleFiberRoot(root, children);
+  if (__DEV__) {
+    if (
+      injectedHook &&
+      typeof injectedHook.onScheduleFiberRoot === 'function'
+    ) {
+      try {
+        injectedHook.onScheduleFiberRoot(rendererID, root, children);
+      } catch (err) {
+        if (__DEV__ && !hasLoggedError) {
+          hasLoggedError = true;
+          console.error('React instrumentation encountered an error: %s', err);
+        }
+      }
+    }
   }
 }
 
 export function onCommitRoot(root: FiberRoot, expirationTime: ExpirationTime) {
-  if (typeof onCommitFiberRoot === 'function') {
-    onCommitFiberRoot(root, expirationTime);
+  if (injectedHook && typeof injectedHook.onCommitFiberRoot === 'function') {
+    try {
+      const didError = (root.current.effectTag & DidCapture) === DidCapture;
+      if (enableProfilerTimer) {
+        const currentTime = getCurrentTime();
+        const priorityLevel = inferPriorityFromExpirationTime(
+          currentTime,
+          expirationTime,
+        );
+        injectedHook.onCommitFiberRoot(
+          rendererID,
+          root,
+          priorityLevel,
+          didError,
+        );
+      } else {
+        injectedHook.onCommitFiberRoot(rendererID, root, undefined, didError);
+      }
+    } catch (err) {
+      if (__DEV__) {
+        if (!hasLoggedError) {
+          hasLoggedError = true;
+          console.error('React instrumentation encountered an error: %s', err);
+        }
+      }
+    }
   }
 }
 
 export function onCommitUnmount(fiber: Fiber) {
-  if (typeof onCommitFiberUnmount === 'function') {
-    onCommitFiberUnmount(fiber);
+  if (injectedHook && typeof injectedHook.onCommitFiberUnmount === 'function') {
+    try {
+      injectedHook.onCommitFiberUnmount(rendererID, fiber);
+    } catch (err) {
+      if (__DEV__) {
+        if (!hasLoggedError) {
+          hasLoggedError = true;
+          console.error('React instrumentation encountered an error: %s', err);
+        }
+      }
+    }
   }
 }
diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js
@@ -506,6 +506,24 @@ if (__DEV__) {
   };
 }
 
+function findHostInstanceByFiber(fiber: Fiber): Instance | TextInstance | null {
+  const hostFiber = findCurrentHostFiber(fiber);
+  if (hostFiber === null) {
+    return null;
+  }
+  return hostFiber.stateNode;
+}
+
+function emptyFindFiberByHostInstance(
+  instance: Instance | TextInstance,
+): Fiber | null {
+  return null;
+}
+
+function getCurrentFiberForDevTools() {
+  return ReactCurrentFiberCurrent;
+}
+
 export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean {
   const {findFiberByHostInstance} = devToolsConfig;
   const {ReactCurrentDispatcher} = ReactSharedInternals;
@@ -520,27 +538,16 @@ export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean {
     setSuspenseHandler,
     scheduleUpdate,
     currentDispatcherRef: ReactCurrentDispatcher,
-    findHostInstanceByFiber(fiber: Fiber): Instance | TextInstance | null {
-      const hostFiber = findCurrentHostFiber(fiber);
-      if (hostFiber === null) {
-        return null;
-      }
-      return hostFiber.stateNode;
-    },
-    findFiberByHostInstance(instance: Instance | TextInstance): Fiber | null {
-      if (!findFiberByHostInstance) {
-        // Might not be implemented by the renderer.
-        return null;
-      }
-      return findFiberByHostInstance(instance);
-    },
+    findHostInstanceByFiber,
+    findFiberByHostInstance:
+      findFiberByHostInstance || emptyFindFiberByHostInstance,
     // React Refresh
     findHostInstancesForRefresh: __DEV__ ? findHostInstancesForRefresh : null,
     scheduleRefresh: __DEV__ ? scheduleRefresh : null,
     scheduleRoot: __DEV__ ? scheduleRoot : null,
     setRefreshHandler: __DEV__ ? setRefreshHandler : null,
     // Enables DevTools to append owner stacks to error messages in DEV mode.
-    getCurrentFiber: __DEV__ ? () => ReactCurrentFiberCurrent : null,
+    getCurrentFiber: __DEV__ ? getCurrentFiberForDevTools : null,
   });
 }