Flush useEffect clean up functions in the passive effects phase

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -26,6 +26,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
 
 import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';
 import {
+  deferPassiveEffectCleanupDuringUnmount,
   enableSchedulerTracing,
   enableProfilerTimer,
   enableSuspenseServerRenderer,
@@ -109,16 +110,13 @@ import {
   captureCommitPhaseError,
   resolveRetryThenable,
   markCommitTimeOfFallback,
+  enqueuePendingPassiveEffectDestroyFn,
 } from './ReactFiberWorkLoop';
 import {
   NoEffect as NoHookEffect,
-  UnmountSnapshot,
-  UnmountMutation,
-  MountMutation,
-  UnmountLayout,
-  MountLayout,
-  UnmountPassive,
-  MountPassive,
+  HasEffect as HookHasEffect,
+  Layout as HookLayout,
+  Passive as HookPassive,
 } from './ReactHookEffectTags';
 import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
 import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';
@@ -250,7 +248,6 @@ function commitBeforeMutationLifeCycles(
     case ForwardRef:
     case SimpleMemoComponent:
     case Chunk: {
-      commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork);
       return;
     }
     case ClassComponent: {
@@ -328,26 +325,34 @@ function commitBeforeMutationLifeCycles(
   );
 }
 
-function commitHookEffectList(
-  unmountTag: number,
-  mountTag: number,
-  finishedWork: Fiber,
-) {
+function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) {
   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 {
-      if ((effect.tag & unmountTag) !== NoHookEffect) {
+      if ((effect.tag & tag) === tag) {
         // Unmount
         const destroy = effect.destroy;
         effect.destroy = undefined;
         if (destroy !== undefined) {
           destroy();
         }
       }
-      if ((effect.tag & mountTag) !== NoHookEffect) {
+      effect = effect.next;
+    } while (effect !== firstEffect);
+  }
+}
+
+function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
+  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 {
+      if ((effect.tag & tag) === tag) {
         // Mount
         const create = effect.create;
         effect.destroy = create();
@@ -398,8 +403,11 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
       case ForwardRef:
       case SimpleMemoComponent:
       case Chunk: {
-        commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
-        commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
+        // 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:
@@ -419,7 +427,11 @@ function commitLifeCycles(
     case ForwardRef:
     case SimpleMemoComponent:
     case Chunk: {
-      commitHookEffectList(UnmountLayout, MountLayout, finishedWork);
+      // At this point layout effects have already been destroyed (during mutation phase).
+      // This is done to prevent sibling component effects from interfering with each other,
+      // 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);
       return;
     }
     case ClassComponent: {
@@ -756,32 +768,47 @@ function commitUnmount(
         if (lastEffect !== null) {
           const firstEffect = lastEffect.next;
 
-          // When the owner fiber is deleted, the destroy function of a passive
-          // effect hook is called during the synchronous commit phase. This is
-          // a concession to implementation complexity. Calling it in the
-          // passive effect phase (like they usually are, when dependencies
-          // change during an update) would require either traversing the
-          // children of the deleted fiber again, or including unmount effects
-          // as part of the fiber effect list.
-          //
-          // Because this is during the sync commit phase, we need to change
-          // the priority.
-          //
-          // TODO: Reconsider this implementation trade off.
-          const priorityLevel =
-            renderPriorityLevel > NormalPriority
-              ? NormalPriority
-              : renderPriorityLevel;
-          runWithPriority(priorityLevel, () => {
+          if (deferPassiveEffectCleanupDuringUnmount) {
             let effect = firstEffect;
             do {
-              const destroy = effect.destroy;
+              const {destroy, tag} = effect;
               if (destroy !== undefined) {
-                safelyCallDestroy(current, destroy);
+                if ((tag & HookPassive) !== NoHookEffect) {
+                  enqueuePendingPassiveEffectDestroyFn(destroy);
+                } else {
+                  safelyCallDestroy(current, destroy);
+                }
               }
               effect = effect.next;
             } while (effect !== firstEffect);
-          });
+          } else {
+            // When the owner fiber is deleted, the destroy function of a passive
+            // effect hook is called during the synchronous commit phase. This is
+            // a concession to implementation complexity. Calling it in the
+            // passive effect phase (like they usually are, when dependencies
+            // change during an update) would require either traversing the
+            // children of the deleted fiber again, or including unmount effects
+            // as part of the fiber effect list.
+            //
+            // Because this is during the sync commit phase, we need to change
+            // the priority.
+            //
+            // TODO: Reconsider this implementation trade off.
+            const priorityLevel =
+              renderPriorityLevel > NormalPriority
+                ? NormalPriority
+                : renderPriorityLevel;
+            runWithPriority(priorityLevel, () => {
+              let effect = firstEffect;
+              do {
+                const destroy = effect.destroy;
+                if (destroy !== undefined) {
+                  safelyCallDestroy(current, destroy);
+                }
+                effect = effect.next;
+              } while (effect !== firstEffect);
+            });
+          }
         }
       }
       return;
@@ -1285,9 +1312,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
       case MemoComponent:
       case SimpleMemoComponent:
       case Chunk: {
-        // Note: We currently never use MountMutation, but useLayout uses
-        // UnmountMutation.
-        commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
+        // Layout effects are destroyed during the mutation phase so that all
+        // destroy functions for all fibers are called before any create functions.
+        // This prevents sibling component effects from interfering with each other,
+        // 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.
+        commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
         return;
       }
       case Profiler: {
@@ -1325,9 +1355,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
     case MemoComponent:
     case SimpleMemoComponent:
     case Chunk: {
-      // Note: We currently never use MountMutation, but useLayout uses
-      // UnmountMutation.
-      commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
+      // Layout effects are destroyed during the mutation phase so that all
+      // destroy functions for all fibers are called before any create functions.
+      // This prevents sibling component effects from interfering with each other,
+      // 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.
+      commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
       return;
     }
     case ClassComponent: {

packages/react-reconciler/src/ReactFiberHooks.js

@@ -28,11 +28,9 @@ import {
   Passive as PassiveEffect,
 } from 'shared/ReactSideEffectTags';
 import {
-  NoEffect as NoHookEffect,
-  UnmountMutation,
-  MountLayout,
-  UnmountPassive,
-  MountPassive,
+  HasEffect as HookHasEffect,
+  Layout as HookLayout,
+  Passive as HookPassive,
 } from './ReactHookEffectTags';
 import {
   scheduleWork,
@@ -923,7 +921,12 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
   const hook = mountWorkInProgressHook();
   const nextDeps = deps === undefined ? null : deps;
   currentlyRenderingFiber.effectTag |= fiberEffectTag;
-  hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
+  hook.memoizedState = pushEffect(
+    HookHasEffect | hookEffectTag,
+    create,
+    undefined,
+    nextDeps,
+  );
 }
 
 function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
@@ -937,15 +940,20 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
     if (nextDeps !== null) {
       const prevDeps = prevEffect.deps;
       if (areHookInputsEqual(nextDeps, prevDeps)) {
-        pushEffect(NoHookEffect, create, destroy, nextDeps);
+        pushEffect(hookEffectTag, create, destroy, nextDeps);
         return;
       }
     }
   }
 
   currentlyRenderingFiber.effectTag |= fiberEffectTag;
 
-  hook.memoizedState = pushEffect(hookEffectTag, create, destroy, nextDeps);
+  hook.memoizedState = pushEffect(
+    HookHasEffect | hookEffectTag,
+    create,
+    destroy,
+    nextDeps,
+  );
 }
 
 function mountEffect(
@@ -960,7 +968,7 @@ function mountEffect(
   }
   return mountEffectImpl(
     UpdateEffect | PassiveEffect,
-    UnmountPassive | MountPassive,
+    HookPassive,
     create,
     deps,
   );
@@ -978,7 +986,7 @@ function updateEffect(
   }
   return updateEffectImpl(
     UpdateEffect | PassiveEffect,
-    UnmountPassive | MountPassive,
+    HookPassive,
     create,
     deps,
   );
@@ -988,24 +996,14 @@ function mountLayoutEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
-  return mountEffectImpl(
-    UpdateEffect,
-    UnmountMutation | MountLayout,
-    create,
-    deps,
-  );
+  return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
 }
 
 function updateLayoutEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
-  return updateEffectImpl(
-    UpdateEffect,
-    UnmountMutation | MountLayout,
-    create,
-    deps,
-  );
+  return updateEffectImpl(UpdateEffect, HookLayout, create, deps);
 }
 
 function imperativeHandleEffect<T>(
@@ -1059,7 +1057,7 @@ function mountImperativeHandle<T>(
 
   return mountEffectImpl(
     UpdateEffect,
-    UnmountMutation | MountLayout,
+    HookLayout,
     imperativeHandleEffect.bind(null, create, ref),
     effectDeps,
   );
@@ -1086,7 +1084,7 @@ function updateImperativeHandle<T>(
 
   return updateEffectImpl(
     UpdateEffect,
-    UnmountMutation | MountLayout,
+    HookLayout,
     imperativeHandleEffect.bind(null, create, ref),
     effectDeps,
   );

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -18,6 +18,7 @@ import type {Hook} from './ReactFiberHooks';
 
 import {
   warnAboutDeprecatedLifecycles,
+  deferPassiveEffectCleanupDuringUnmount,
   enableUserTimingAPI,
   enableSuspenseServerRenderer,
   replayFailedUnitOfWorkWithInvokeGuardedCallback,
@@ -257,6 +258,7 @@ let rootDoesHavePassiveEffects: boolean = false;
 let rootWithPendingPassiveEffects: FiberRoot | null = null;
 let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
 let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
+let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
 
 let rootsWithPendingDiscreteUpdates: Map<
   FiberRoot,
@@ -2176,6 +2178,21 @@ export function flushPassiveEffects() {
   }
 }
 
+export function enqueuePendingPassiveEffectDestroyFn(
+  destroy: () => void,
+): void {
+  if (deferPassiveEffectCleanupDuringUnmount) {
+    pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
+    if (!rootDoesHavePassiveEffects) {
+      rootDoesHavePassiveEffects = true;
+      scheduleCallback(NormalPriority, () => {
+        flushPassiveEffects();
+        return null;
+      });
+    }
+  }
+}
+
 function flushPassiveEffectsImpl() {
   if (rootWithPendingPassiveEffects === null) {
     return false;
@@ -2193,6 +2210,20 @@ function flushPassiveEffectsImpl() {
   executionContext |= CommitContext;
   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;
+  }
+
   // 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.

packages/react-reconciler/src/ReactHookEffectTags.js

@@ -9,11 +9,11 @@
 
 export type HookEffectTag = number;
 
-export const NoEffect = /*             */ 0b00000000;
-export const UnmountSnapshot = /*      */ 0b00000010;
-export const UnmountMutation = /*      */ 0b00000100;
-export const MountMutation = /*        */ 0b00001000;
-export const UnmountLayout = /*        */ 0b00010000;
-export const MountLayout = /*          */ 0b00100000;
-export const MountPassive = /*         */ 0b01000000;
-export const UnmountPassive = /*       */ 0b10000000;
+export const NoEffect = /*  */ 0b000;
+
+// Represents whether effect should fire.
+export const HasEffect = /* */ 0b001;
+
+// Represents the phase in which the effect (not the clean-up) fires.
+export const Layout = /*    */ 0b010;
+export const Passive = /*   */ 0b100;