Consolidate commit phase hook functions (#19864)

acdlite · web-flow · commit 7355bf575ad0 · 2020-09-22T11:16:27.000-07:00
There were a few pairs of commit phase functions that were almost
identical except for one detail. I've refactored them a bit to
consolidate their implementations:

- Lifted error handling logic when mounting a fiber's passive hook
effects to surround the entire list, instead of surrounding each effect.
- Lifted profiler duration tracking to surround the entire list.

In both cases, this matches the corresponding code for the layout phase.

The naming is still a bit of a mess but I'm not too concerned because
my next step is to refactor each commit sub-phase (layout, mutation)
so that we can store values on the JS stack. So the existing function
boundaries are about to change, anyway.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -20,10 +20,7 @@ import type {FiberRoot} from './ReactInternalTypes';
 import type {Lanes} from './ReactFiberLane';
 import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
 import type {UpdateQueue} from './ReactUpdateQueue.new';
-import type {
-  Effect as HookEffect,
-  FunctionComponentUpdateQueue,
-} from './ReactFiberHooks.new';
+import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new';
 import type {Wakeable} from 'shared/ReactTypes';
 import type {ReactPriorityLevel} from './ReactInternalTypes';
 import type {OffscreenState} from './ReactFiberOffscreenComponent';
@@ -343,42 +340,6 @@ function commitHookEffectListUnmount(
   }
 }
 
-// TODO: Remove this duplication.
-function commitHookEffectListUnmount2(
-  // Tags to check for when deciding whether to unmount. e.g. to skip over layout effects
-  hookFlags: HookFlags,
-  fiber: Fiber,
-  nearestMountedAncestor: Fiber | null,
-): void {
-  const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
-  const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
-  if (lastEffect !== null) {
-    const firstEffect = lastEffect.next;
-    let effect = firstEffect;
-    do {
-      const {next, tag} = effect;
-      if ((tag & hookFlags) === hookFlags) {
-        const destroy = effect.destroy;
-        if (destroy !== undefined) {
-          effect.destroy = undefined;
-          if (
-            enableProfilerTimer &&
-            enableProfilerCommitHooks &&
-            fiber.mode & ProfileMode
-          ) {
-            startPassiveEffectTimer();
-            safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
-            recordPassiveEffectDuration(fiber);
-          } else {
-            safelyCallDestroy(fiber, nearestMountedAncestor, destroy);
-          }
-        }
-      }
-      effect = next;
-    } while (effect !== firstEffect);
-  }
-}
-
 function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) {
   const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
   const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
@@ -429,83 +390,6 @@ function commitHookEffectListMount(tag: HookFlags, finishedWork: Fiber) {
   }
 }
 
-function invokePassiveEffectCreate(effect: HookEffect): void {
-  const create = effect.create;
-  effect.destroy = create();
-}
-
-// TODO: Remove this duplication.
-function commitHookEffectListMount2(fiber: Fiber): void {
-  const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
-  const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
-  if (lastEffect !== null) {
-    const firstEffect = lastEffect.next;
-    let effect = firstEffect;
-    do {
-      const {next, tag} = effect;
-
-      if (
-        (tag & HookPassive) !== NoHookEffect &&
-        (tag & HookHasEffect) !== NoHookEffect
-      ) {
-        if (__DEV__) {
-          if (
-            enableProfilerTimer &&
-            enableProfilerCommitHooks &&
-            fiber.mode & ProfileMode
-          ) {
-            startPassiveEffectTimer();
-            invokeGuardedCallback(
-              null,
-              invokePassiveEffectCreate,
-              null,
-              effect,
-            );
-            recordPassiveEffectDuration(fiber);
-          } else {
-            invokeGuardedCallback(
-              null,
-              invokePassiveEffectCreate,
-              null,
-              effect,
-            );
-          }
-          if (hasCaughtError()) {
-            invariant(fiber !== null, 'Should be working on an effect.');
-            const error = clearCaughtError();
-            captureCommitPhaseError(fiber, fiber.return, error);
-          }
-        } else {
-          try {
-            const create = effect.create;
-            if (
-              enableProfilerTimer &&
-              enableProfilerCommitHooks &&
-              fiber.mode & ProfileMode
-            ) {
-              try {
-                startPassiveEffectTimer();
-                effect.destroy = create();
-              } finally {
-                recordPassiveEffectDuration(fiber);
-              }
-            } else {
-              effect.destroy = create();
-            }
-            // TODO: This is missing the warning that exists in commitHookEffectListMount.
-            // The warning refers to useEffect but only applies to useLayoutEffect.
-          } catch (error) {
-            invariant(fiber !== null, 'Should be working on an effect.');
-            captureCommitPhaseError(fiber, fiber.return, error);
-          }
-        }
-      }
-
-      effect = next;
-    } while (effect !== firstEffect);
-  }
-}
-
 function commitProfilerPassiveEffect(
   finishedRoot: FiberRoot,
   finishedWork: Fiber,
@@ -1894,17 +1778,31 @@ function commitResetTextContent(current: Fiber): void {
   resetTextContent(current.stateNode);
 }
 
-function commitPassiveWork(finishedWork: Fiber): void {
+function commitPassiveUnmountInsideDeletedTree(finishedWork: Fiber): void {
   switch (finishedWork.tag) {
     case FunctionComponent:
     case ForwardRef:
     case SimpleMemoComponent:
     case Block: {
-      commitHookEffectListUnmount2(
-        HookPassive | HookHasEffect,
-        finishedWork,
-        finishedWork.return,
-      );
+      if (
+        enableProfilerTimer &&
+        enableProfilerCommitHooks &&
+        finishedWork.mode & ProfileMode
+      ) {
+        startPassiveEffectTimer();
+        commitHookEffectListUnmount(
+          HookPassive | HookHasEffect,
+          finishedWork,
+          finishedWork.return,
+        );
+        recordPassiveEffectDuration(finishedWork);
+      } else {
+        commitHookEffectListUnmount(
+          HookPassive | HookHasEffect,
+          finishedWork,
+          finishedWork.return,
+        );
+      }
       break;
     }
   }
@@ -1918,16 +1816,32 @@ function commitPassiveUnmount(
     case FunctionComponent:
     case ForwardRef:
     case SimpleMemoComponent:
-    case Block:
-      commitHookEffectListUnmount2(
-        HookPassive,
-        current,
-        nearestMountedAncestor,
-      );
+    case Block: {
+      if (
+        enableProfilerTimer &&
+        enableProfilerCommitHooks &&
+        current.mode & ProfileMode
+      ) {
+        startPassiveEffectTimer();
+        commitHookEffectListUnmount(
+          HookPassive,
+          current,
+          nearestMountedAncestor,
+        );
+        recordPassiveEffectDuration(current);
+      } else {
+        commitHookEffectListUnmount(
+          HookPassive,
+          current,
+          nearestMountedAncestor,
+        );
+      }
+      break;
+    }
   }
 }
 
-function commitPassiveLifeCycles(
+function commitPassiveMount(
   finishedRoot: FiberRoot,
   finishedWork: Fiber,
 ): void {
@@ -1936,7 +1850,20 @@ function commitPassiveLifeCycles(
     case ForwardRef:
     case SimpleMemoComponent:
     case Block: {
-      commitHookEffectListMount2(finishedWork);
+      if (
+        enableProfilerTimer &&
+        enableProfilerCommitHooks &&
+        finishedWork.mode & ProfileMode
+      ) {
+        startPassiveEffectTimer();
+        try {
+          commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
+        } finally {
+          recordPassiveEffectDuration(finishedWork);
+        }
+      } else {
+        commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
+      }
       break;
     }
     case Profiler: {
@@ -1956,6 +1883,6 @@ export {
   commitAttachRef,
   commitDetachRef,
   commitPassiveUnmount,
-  commitPassiveWork,
-  commitPassiveLifeCycles,
+  commitPassiveUnmountInsideDeletedTree,
+  commitPassiveMount,
 };
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -191,9 +191,9 @@ import {
   commitPlacement,
   commitWork,
   commitDeletion,
-  commitPassiveUnmount,
-  commitPassiveWork,
-  commitPassiveLifeCycles as commitPassiveEffectOnFiber,
+  commitPassiveUnmount as commitPassiveUnmountOnFiber,
+  commitPassiveUnmountInsideDeletedTree as commitPassiveUnmountInsideDeletedTreeOnFiber,
+  commitPassiveMount as commitPassiveMountOnFiber,
   commitDetachRef,
   commitAttachRef,
   commitResetTextContent,
@@ -2458,9 +2458,27 @@ function flushPassiveMountEffects(root, firstChild: Fiber): void {
     }
 
     if ((fiber.flags & Passive) !== NoFlags) {
-      setCurrentDebugFiberInDEV(fiber);
-      commitPassiveEffectOnFiber(root, fiber);
-      resetCurrentDebugFiberInDEV();
+      if (__DEV__) {
+        setCurrentDebugFiberInDEV(fiber);
+        invokeGuardedCallback(
+          null,
+          commitPassiveMountOnFiber,
+          null,
+          root,
+          fiber,
+        );
+        if (hasCaughtError()) {
+          const error = clearCaughtError();
+          captureCommitPhaseError(fiber, fiber.return, error);
+        }
+        resetCurrentDebugFiberInDEV();
+      } else {
+        try {
+          commitPassiveMountOnFiber(root, fiber);
+        } catch (error) {
+          captureCommitPhaseError(fiber, fiber.return, error);
+        }
+      }
     }
 
     fiber = fiber.sibling;
@@ -2496,7 +2514,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void {
     const primaryFlags = fiber.flags & Passive;
     if (primaryFlags !== NoFlags) {
       setCurrentDebugFiberInDEV(fiber);
-      commitPassiveWork(fiber);
+      commitPassiveUnmountInsideDeletedTreeOnFiber(fiber);
       resetCurrentDebugFiberInDEV();
     }
 
@@ -2525,7 +2543,7 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree(
 
   if ((fiberToDelete.flags & PassiveStatic) !== NoFlags) {
     setCurrentDebugFiberInDEV(fiberToDelete);
-    commitPassiveUnmount(fiberToDelete, nearestMountedAncestor);
+    commitPassiveUnmountOnFiber(fiberToDelete, nearestMountedAncestor);
     resetCurrentDebugFiberInDEV();
   }
 }
