Change passive effect flushing to use arrays instead of lists

This change offers a small advantage over the way we did things previous: it continues invoking destroy functions even after a previous one errored.

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -110,7 +110,8 @@ import {
   captureCommitPhaseError,
   resolveRetryThenable,
   markCommitTimeOfFallback,
-  enqueuePendingPassiveEffectDestroyFn,
+  enqueuePendingPassiveHookEffectMount,
+  enqueuePendingPassiveHookEffectUnmount,
 } from './ReactFiberWorkLoop';
 import {
   NoEffect as NoHookEffect,
@@ -396,49 +397,39 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
   }
 }
 
-export function commitPassiveHookEffects(finishedWork: Fiber): void {
-  if ((finishedWork.effectTag & Passive) !== NoEffect) {
-    switch (finishedWork.tag) {
-      case FunctionComponent:
-      case ForwardRef:
-      case SimpleMemoComponent:
-      case Chunk: {
-        // TODO (#17945) We should call all passive destroy functions (for all fibers)
-        // before calling any create functions. The current approach only serializes
-        // these for a single fiber.
-        commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
-        commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
-        break;
-      }
-      default:
-        break;
+function schedulePassiveEffects(finishedWork: Fiber) {
+  if (deferPassiveEffectCleanupDuringUnmount) {
+    const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
+    let 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
+        ) {
+          enqueuePendingPassiveHookEffectUnmount(finishedWork, effect);
+          enqueuePendingPassiveHookEffectMount(finishedWork, effect);
+        }
+        effect = next;
+      } while (effect !== firstEffect);
     }
   }
 }
 
-export function commitPassiveHookUnmountEffects(finishedWork: Fiber): void {
+export function commitPassiveHookEffects(finishedWork: Fiber): void {
   if ((finishedWork.effectTag & Passive) !== NoEffect) {
     switch (finishedWork.tag) {
       case FunctionComponent:
       case ForwardRef:
       case SimpleMemoComponent:
       case Chunk: {
+        // TODO (#17945) We should call all passive destroy functions (for all fibers)
+        // before calling any create functions. The current approach only serializes
+        // these for a single fiber.
         commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
-        break;
-      }
-      default:
-        break;
-    }
-  }
-}
-
-export function commitPassiveHookMountEffects(finishedWork: Fiber): void {
-  if ((finishedWork.effectTag & Passive) !== NoEffect) {
-    switch (finishedWork.tag) {
-      case FunctionComponent:
-      case ForwardRef:
-      case SimpleMemoComponent:
-      case Chunk: {
         commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
         break;
       }
@@ -464,6 +455,10 @@ function commitLifeCycles(
       // e.g. a destroy function in one component should never override a ref set
       // by a create function in another component during the same commit.
       commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);
+
+      if (deferPassiveEffectCleanupDuringUnmount) {
+        schedulePassiveEffects(finishedWork);
+      }
       return;
     }
     case ClassComponent: {
@@ -806,7 +801,7 @@ function commitUnmount(
               const {destroy, tag} = effect;
               if (destroy !== undefined) {
                 if ((tag & HookPassive) !== NoHookEffect) {
-                  enqueuePendingPassiveEffectDestroyFn(destroy);
+                  enqueuePendingPassiveHookEffectUnmount(current, effect);
                 } else {
                   safelyCallDestroy(current, destroy);
                 }

packages/react-reconciler/src/ReactFiberHooks.js

@@ -144,7 +144,7 @@ export type Hook = {|
   next: Hook | null,
 |};
 
-type Effect = {|
+export type Effect = {|
   tag: HookEffectTag,
   create: () => (() => void) | void,
   destroy: (() => void) | void,

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
 import type {Interaction} from 'scheduler/src/Tracing';
 import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
 import type {SuspenseState} from './ReactFiberSuspenseComponent';
+import type {Effect as HookEffect} from './ReactFiberHooks';
 
 import {
   warnAboutDeprecatedLifecycles,
@@ -132,8 +133,6 @@ import {
   commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
   commitLifeCycles as commitLayoutEffectOnFiber,
   commitPassiveHookEffects,
-  commitPassiveHookUnmountEffects,
-  commitPassiveHookMountEffects,
   commitPlacement,
   commitWork,
   commitDeletion,
@@ -259,7 +258,8 @@ let rootDoesHavePassiveEffects: boolean = false;
 let rootWithPendingPassiveEffects: FiberRoot | null = null;
 let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
 let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
-let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
+let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
+let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];
 
 let rootsWithPendingDiscreteUpdates: Map<
   FiberRoot,
@@ -2170,11 +2170,28 @@ export function flushPassiveEffects() {
   }
 }
 
-export function enqueuePendingPassiveEffectDestroyFn(
-  destroy: () => void,
+export function enqueuePendingPassiveHookEffectMount(
+  fiber: Fiber,
+  effect: HookEffect,
+): void {
+  if (deferPassiveEffectCleanupDuringUnmount) {
+    pendingPassiveHookEffectsMount.push(effect, fiber);
+    if (!rootDoesHavePassiveEffects) {
+      rootDoesHavePassiveEffects = true;
+      scheduleCallback(NormalPriority, () => {
+        flushPassiveEffects();
+        return null;
+      });
+    }
+  }
+}
+
+export function enqueuePendingPassiveHookEffectUnmount(
+  fiber: Fiber,
+  effect: HookEffect,
 ): void {
   if (deferPassiveEffectCleanupDuringUnmount) {
-    pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
+    pendingPassiveHookEffectsUnmount.push(effect, fiber);
     if (!rootDoesHavePassiveEffects) {
       rootDoesHavePassiveEffects = true;
       scheduleCallback(NormalPriority, () => {
@@ -2185,6 +2202,11 @@ export function enqueuePendingPassiveEffectDestroyFn(
   }
 }
 
+function invokePassiveEffectCreate(effect: HookEffect): void {
+  const create = effect.create;
+  effect.destroy = create();
+}
+
 function flushPassiveEffectsImpl() {
   if (rootWithPendingPassiveEffects === null) {
     return false;
@@ -2203,18 +2225,6 @@ function flushPassiveEffectsImpl() {
   const prevInteractions = pushInteractions(root);
 
   if (deferPassiveEffectCleanupDuringUnmount) {
-    // Flush any pending passive effect destroy functions that belong to
-    // components that were unmounted during the most recent commit.
-    for (
-      let i = 0;
-      i < pendingUnmountedPassiveEffectDestroyFunctions.length;
-      i++
-    ) {
-      const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
-      invokeGuardedCallback(null, destroy, null);
-    }
-    pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
-
     // It's important that ALL pending passive effect destroy functions are called
     // before ANY passive effect create functions are called.
     // Otherwise effects in sibling components might interfere with each other.
@@ -2223,70 +2233,58 @@ function flushPassiveEffectsImpl() {
     // Layout effects have the same constraint.
 
     // First pass: Destroy stale passive effects.
-    //
-    // Note: This currently assumes there are no passive effects on the root fiber
-    // because the root is not part of its own effect list.
-    // This could change in the future.
-    let effect = root.current.firstEffect;
-    while (effect !== null) {
-      if (__DEV__) {
-        setCurrentDebugFiberInDEV(effect);
-        invokeGuardedCallback(
-          null,
-          commitPassiveHookUnmountEffects,
-          null,
-          effect,
-        );
-        if (hasCaughtError()) {
-          invariant(effect !== null, 'Should be working on an effect.');
-          const error = clearCaughtError();
-          captureCommitPhaseError(effect, error);
-        }
-        resetCurrentDebugFiberInDEV();
-      } else {
-        try {
-          commitPassiveHookUnmountEffects(effect);
-        } catch (error) {
-          invariant(effect !== null, 'Should be working on an effect.');
-          captureCommitPhaseError(effect, error);
+    let unmountEffects = pendingPassiveHookEffectsUnmount;
+    pendingPassiveHookEffectsUnmount = [];
+    for (let i = 0; i < unmountEffects.length; i += 2) {
+      const effect = ((unmountEffects[i]: any): HookEffect);
+      const fiber = ((unmountEffects[i + 1]: any): Fiber);
+      const destroy = effect.destroy;
+      effect.destroy = undefined;
+      if (typeof destroy === 'function') {
+        if (__DEV__) {
+          setCurrentDebugFiberInDEV(fiber);
+          invokeGuardedCallback(null, destroy, null);
+          if (hasCaughtError()) {
+            invariant(fiber !== null, 'Should be working on an effect.');
+            const error = clearCaughtError();
+            captureCommitPhaseError(fiber, error);
+          }
+          resetCurrentDebugFiberInDEV();
+        } else {
+          try {
+            destroy();
+          } catch (error) {
+            invariant(fiber !== null, 'Should be working on an effect.');
+            captureCommitPhaseError(fiber, error);
+          }
         }
       }
-      effect = effect.nextEffect;
     }
 
     // Second pass: Create new passive effects.
-    //
-    // Note: This currently assumes there are no passive effects on the root fiber
-    // because the root is not part of its own effect list.
-    // This could change in the future.
-    effect = root.current.firstEffect;
-    while (effect !== null) {
+    let mountEffects = pendingPassiveHookEffectsMount;
+    pendingPassiveHookEffectsMount = [];
+    for (let i = 0; i < mountEffects.length; i += 2) {
+      const effect = ((mountEffects[i]: any): HookEffect);
+      const fiber = ((mountEffects[i + 1]: any): Fiber);
       if (__DEV__) {
-        setCurrentDebugFiberInDEV(effect);
-        invokeGuardedCallback(
-          null,
-          commitPassiveHookMountEffects,
-          null,
-          effect,
-        );
+        setCurrentDebugFiberInDEV(fiber);
+        invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect);
         if (hasCaughtError()) {
-          invariant(effect !== null, 'Should be working on an effect.');
+          invariant(fiber !== null, 'Should be working on an effect.');
           const error = clearCaughtError();
-          captureCommitPhaseError(effect, error);
+          captureCommitPhaseError(fiber, error);
         }
         resetCurrentDebugFiberInDEV();
       } else {
         try {
-          commitPassiveHookMountEffects(effect);
+          const create = effect.create;
+          effect.destroy = create();
         } catch (error) {
-          invariant(effect !== null, 'Should be working on an effect.');
-          captureCommitPhaseError(effect, error);
+          invariant(fiber !== null, 'Should be working on an effect.');
+          captureCommitPhaseError(fiber, error);
         }
       }
-      const nextNextEffect = effect.nextEffect;
-      // Remove nextEffect pointer to assist GC
-      effect.nextEffect = null;
-      effect = nextNextEffect;
     }
   } else {
     // Note: This currently assumes there are no passive effects on the root fiber

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js

@@ -1753,6 +1753,7 @@ describe('ReactHooksWithNoopRenderer', () => {
         // not block the subsequent create functions from being run.
         expect(Scheduler).toHaveYielded([
           'Oops!',
+          'Unmount B [0]',
           'Mount A [1]',
           'Mount B [1]',
         ]);