Flush useEffect clean up functions in the passive effects phase (#17925)

* Flush useEffect clean up functions in the passive effects phase
This is a change in behavior that may cause broken product code, so it has been added behind a killswitch (deferPassiveEffectCleanupDuringUnmount)
* Avoid scheduling unnecessary callbacks for cleanup effects
Updated enqueuePendingPassiveEffectDestroyFn() to check rootDoesHavePassiveEffects before scheduling a new callback. This way we'll only schedule (at most) one.
* Updated newly added test for added clarity.
* Cleaned up hooks effect tags
We previously used separate Mount* and Unmount* tags to track hooks work for each phase (snapshot, mutation, layout, and passive). This was somewhat complicated to trace through and there were man tag types we never even used (e.g. UnmountLayout, MountMutation, UnmountSnapshot). In addition to this, it left passive and layout hooks looking the same after renders without changed dependencies, which meant we were unable to reliably defer passive effect destroy functions until after the commit phase.
This commit reduces the effect tag types to only include Layout and Passive and differentiates between work and no-work with an HasEffect flag.
* Disabled deferred passive effects flushing in OSS builds for now
* Split up unmount and mount effects list traversal

Author: Brian Vaughn

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -26,6 +26,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
 
 import{unstable_wrapasSchedule_tracing_wrap}from'scheduler/tracing';
 import{
+deferPassiveEffectCleanupDuringUnmount,
 enableSchedulerTracing,
 enableProfilerTimer,
 enableSuspenseServerRenderer,
@@ -109,16 +110,13 @@ import {
 captureCommitPhaseError,
 resolveRetryThenable,
 markCommitTimeOfFallback,
+enqueuePendingPassiveEffectDestroyFn,
 }from'./ReactFiberWorkLoop';
 import{
 NoEffectasNoHookEffect,
-UnmountSnapshot,
-UnmountMutation,
-MountMutation,
-UnmountLayout,
-MountLayout,
-UnmountPassive,
-MountPassive,
+HasEffectasHookHasEffect,
+LayoutasHookLayout,
+PassiveasHookPassive,
 }from'./ReactHookEffectTags';
 import{didWarnAboutReassigningProps}from'./ReactFiberBeginWork';
 import{runWithPriority,NormalPriority}from'./SchedulerWithReactIntegration';
@@ -250,7 +248,6 @@ function commitBeforeMutationLifeCycles(
 caseForwardRef:
 caseSimpleMemoComponent:
 caseChunk: {
-commitHookEffectList(UnmountSnapshot,NoHookEffect,finishedWork);
 return;
 }
 caseClassComponent: {
@@ -328,26 +325,34 @@ function commitBeforeMutationLifeCycles(
 );
 }
 
-functioncommitHookEffectList(
-unmountTag: number,
-mountTag: number,
-finishedWork: Fiber,
-){
+functioncommitHookEffectListUnmount(tag: number,finishedWork: Fiber){
 constupdateQueue: FunctionComponentUpdateQueue|null=(finishedWork.updateQueue: any);
 letlastEffect=updateQueue!==null ? updateQueue.lastEffect : null;
 if(lastEffect!==null){
 constfirstEffect=lastEffect.next;
 leteffect=firstEffect;
 do{
-if((effect.tag&unmountTag)!==NoHookEffect){
+if((effect.tag&tag)===tag){
 // Unmount
 constdestroy=effect.destroy;
 effect.destroy=undefined;
 if(destroy!==undefined){
 destroy();
 }
 }
-if((effect.tag&mountTag)!==NoHookEffect){
+effect=effect.next;
+}while(effect!==firstEffect);
+}
+}
+
+functioncommitHookEffectListMount(tag: number,finishedWork: Fiber){
+constupdateQueue: FunctionComponentUpdateQueue|null=(finishedWork.updateQueue: any);
+letlastEffect=updateQueue!==null ? updateQueue.lastEffect : null;
+if(lastEffect!==null){
+constfirstEffect=lastEffect.next;
+leteffect=firstEffect;
+do{
+if((effect.tag&tag)===tag){
 // Mount
 constcreate=effect.create;
 effect.destroy=create();
@@ -398,8 +403,11 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
 caseForwardRef:
 caseSimpleMemoComponent:
 caseChunk: {
-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(
 caseForwardRef:
 caseSimpleMemoComponent:
 caseChunk: {
-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;
 }
 caseClassComponent: {
@@ -756,32 +768,47 @@ function commitUnmount(
 if(lastEffect!==null){
 constfirstEffect=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.
-constpriorityLevel=
-renderPriorityLevel>NormalPriority
- ? NormalPriority
- : renderPriorityLevel;
-runWithPriority(priorityLevel,()=>{
+if(deferPassiveEffectCleanupDuringUnmount){
 leteffect=firstEffect;
 do{
-constdestroy=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.
+constpriorityLevel=
+renderPriorityLevel>NormalPriority
+ ? NormalPriority
+ : renderPriorityLevel;
+runWithPriority(priorityLevel,()=>{
+leteffect=firstEffect;
+do{
+constdestroy=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 {
 caseMemoComponent:
 caseSimpleMemoComponent:
 caseChunk: {
-// 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;
 }
 caseProfiler: {
@@ -1325,9 +1355,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
 caseMemoComponent:
 caseSimpleMemoComponent:
 caseChunk: {
-// 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;
 }
 caseClassComponent: {

packages/react-reconciler/src/ReactFiberHooks.js

@@ -28,11 +28,9 @@ import {
 PassiveasPassiveEffect,
 }from'shared/ReactSideEffectTags';
 import{
-NoEffectasNoHookEffect,
-UnmountMutation,
-MountLayout,
-UnmountPassive,
-MountPassive,
+HasEffectasHookHasEffect,
+LayoutasHookLayout,
+PassiveasHookPassive,
 }from'./ReactHookEffectTags';
 import{
 scheduleWork,
@@ -923,7 +921,12 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
 consthook=mountWorkInProgressHook();
 constnextDeps=deps===undefined ? null : deps;
 currentlyRenderingFiber.effectTag|=fiberEffectTag;
-hook.memoizedState=pushEffect(hookEffectTag,create,undefined,nextDeps);
+hook.memoizedState=pushEffect(
+HookHasEffect|hookEffectTag,
+create,
+undefined,
+nextDeps,
+);
 }
 
 functionupdateEffectImpl(fiberEffectTag,hookEffectTag,create,deps): void{
@@ -937,15 +940,20 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
 if(nextDeps!==null){
 constprevDeps=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,
+);
 }
 
 functionmountEffect(
@@ -960,7 +968,7 @@ function mountEffect(
 }
 returnmountEffectImpl(
 UpdateEffect|PassiveEffect,
-UnmountPassive|MountPassive,
+HookPassive,
 create,
 deps,
 );
@@ -978,7 +986,7 @@ function updateEffect(
 }
 returnupdateEffectImpl(
 UpdateEffect|PassiveEffect,
-UnmountPassive|MountPassive,
+HookPassive,
 create,
 deps,
 );
@@ -988,24 +996,14 @@ function mountLayoutEffect(
 create: ()=>(()=>void)|void,
 deps: Array<mixed>|void|null,
 ): void{
-returnmountEffectImpl(
-UpdateEffect,
-UnmountMutation|MountLayout,
-create,
-deps,
-);
+returnmountEffectImpl(UpdateEffect,HookLayout,create,deps);
 }
 
 functionupdateLayoutEffect(
 create: ()=>(()=>void)|void,
 deps: Array<mixed>|void|null,
 ): void{
-returnupdateEffectImpl(
-UpdateEffect,
-UnmountMutation|MountLayout,
-create,
-deps,
-);
+returnupdateEffectImpl(UpdateEffect,HookLayout,create,deps);
 }
 
 functionimperativeHandleEffect<T>(
@@ -1059,7 +1057,7 @@ function mountImperativeHandle<T>(
 
 returnmountEffectImpl(
 UpdateEffect,
-UnmountMutation|MountLayout,
+HookLayout,
 imperativeHandleEffect.bind(null,create,ref),
 effectDeps,
 );
@@ -1086,7 +1084,7 @@ function updateImperativeHandle<T>(
 
 returnupdateEffectImpl(
 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;
 letrootWithPendingPassiveEffects: 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() {
 }
 }
 
+exportfunctionenqueuePendingPassiveEffectDestroyFn(
+destroy: ()=>void,
+): void{
+if(deferPassiveEffectCleanupDuringUnmount){
+pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
+if(!rootDoesHavePassiveEffects){
+rootDoesHavePassiveEffects=true;
+scheduleCallback(NormalPriority,()=>{
+flushPassiveEffects();
+returnnull;
+});
+}
+}
+}
+
 functionflushPassiveEffectsImpl(){
 if(rootWithPendingPassiveEffects===null){
 returnfalse;
@@ -2193,6 +2210,20 @@ function flushPassiveEffectsImpl() {
 executionContext|=CommitContext;
 constprevInteractions=pushInteractions(root);
 
+if(deferPassiveEffectCleanupDuringUnmount){
+// Flush any pending passive effect destroy functions that belong to
+// components that were unmounted during the most recent commit.
+for(
+leti=0;
+i<pendingUnmountedPassiveEffectDestroyFunctions.length;
+i++
+){
+constdestroy=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 @@
 
 exporttypeHookEffectTag=number;
 
-exportconstNoEffect=/* */0b00000000;
-exportconstUnmountSnapshot=/* */0b00000010;
-exportconstUnmountMutation=/* */0b00000100;
-exportconstMountMutation=/* */0b00001000;
-exportconstUnmountLayout=/* */0b00010000;
-exportconstMountLayout=/* */0b00100000;
-exportconstMountPassive=/* */0b01000000;
-exportconstUnmountPassive=/* */0b10000000;
+exportconstNoEffect=/* */0b000;
+
+// Represents whether effect should fire.
+exportconstHasEffect=/* */0b001;
+
+// Represents the phase in which the effect (not the clean-up) fires.
+exportconstLayout=/* */0b010;
+exportconstPassive=/* */0b100;