Skip to content

Commit 7df32c4

Browse files
author
Brian Vaughn
authored
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
1 parent 9944bf2 commit 7df32c4

13 files changed

+248
-87
lines changed

‎packages/react-reconciler/src/ReactFiberCommitWork.js

Lines changed: 77 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
2626

2727
import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';
2828
import {
29+
deferPassiveEffectCleanupDuringUnmount,
2930
enableSchedulerTracing,
3031
enableProfilerTimer,
3132
enableSuspenseServerRenderer,
@@ -109,16 +110,13 @@ import {
109110
captureCommitPhaseError,
110111
resolveRetryThenable,
111112
markCommitTimeOfFallback,
113+
enqueuePendingPassiveEffectDestroyFn,
112114
} from './ReactFiberWorkLoop';
113115
import {
114116
NoEffect as NoHookEffect,
115-
UnmountSnapshot,
116-
UnmountMutation,
117-
MountMutation,
118-
UnmountLayout,
119-
MountLayout,
120-
UnmountPassive,
121-
MountPassive,
117+
HasEffect as HookHasEffect,
118+
Layout as HookLayout,
119+
Passive as HookPassive,
122120
} from './ReactHookEffectTags';
123121
import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
124122
import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';
@@ -250,7 +248,6 @@ function commitBeforeMutationLifeCycles(
250248
case ForwardRef:
251249
case SimpleMemoComponent:
252250
case Chunk: {
253-
commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork);
254251
return;
255252
}
256253
case ClassComponent: {
@@ -328,26 +325,34 @@ function commitBeforeMutationLifeCycles(
328325
);
329326
}
330327

331-
function commitHookEffectList(
332-
unmountTag: number,
333-
mountTag: number,
334-
finishedWork: Fiber,
335-
) {
328+
function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) {
336329
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
337330
let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
338331
if (lastEffect !== null) {
339332
const firstEffect = lastEffect.next;
340333
let effect = firstEffect;
341334
do {
342-
if ((effect.tag & unmountTag) !== NoHookEffect) {
335+
if ((effect.tag & tag) === tag) {
343336
// Unmount
344337
const destroy = effect.destroy;
345338
effect.destroy = undefined;
346339
if (destroy !== undefined) {
347340
destroy();
348341
}
349342
}
350-
if ((effect.tag & mountTag) !== NoHookEffect) {
343+
effect = effect.next;
344+
} while (effect !== firstEffect);
345+
}
346+
}
347+
348+
function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
349+
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
350+
let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
351+
if (lastEffect !== null) {
352+
const firstEffect = lastEffect.next;
353+
let effect = firstEffect;
354+
do {
355+
if ((effect.tag & tag) === tag) {
351356
// Mount
352357
const create = effect.create;
353358
effect.destroy = create();
@@ -398,8 +403,11 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
398403
case ForwardRef:
399404
case SimpleMemoComponent:
400405
case Chunk: {
401-
commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
402-
commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
406+
// TODO (#17945) We should call all passive destroy functions (for all fibers)
407+
// before calling any create functions. The current approach only serializes
408+
// these for a single fiber.
409+
commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
410+
commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
403411
break;
404412
}
405413
default:
@@ -419,7 +427,11 @@ function commitLifeCycles(
419427
case ForwardRef:
420428
case SimpleMemoComponent:
421429
case Chunk: {
422-
commitHookEffectList(UnmountLayout, MountLayout, finishedWork);
430+
// At this point layout effects have already been destroyed (during mutation phase).
431+
// This is done to prevent sibling component effects from interfering with each other,
432+
// e.g. a destroy function in one component should never override a ref set
433+
// by a create function in another component during the same commit.
434+
commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);
423435
return;
424436
}
425437
case ClassComponent: {
@@ -756,32 +768,47 @@ function commitUnmount(
756768
if (lastEffect !== null) {
757769
const firstEffect = lastEffect.next;
758770

759-
// When the owner fiber is deleted, the destroy function of a passive
760-
// effect hook is called during the synchronous commit phase. This is
761-
// a concession to implementation complexity. Calling it in the
762-
// passive effect phase (like they usually are, when dependencies
763-
// change during an update) would require either traversing the
764-
// children of the deleted fiber again, or including unmount effects
765-
// as part of the fiber effect list.
766-
//
767-
// Because this is during the sync commit phase, we need to change
768-
// the priority.
769-
//
770-
// TODO: Reconsider this implementation trade off.
771-
const priorityLevel =
772-
renderPriorityLevel > NormalPriority
773-
? NormalPriority
774-
: renderPriorityLevel;
775-
runWithPriority(priorityLevel, () => {
771+
if (deferPassiveEffectCleanupDuringUnmount) {
776772
let effect = firstEffect;
777773
do {
778-
const destroy = effect.destroy;
774+
const {destroy, tag} = effect;
779775
if (destroy !== undefined) {
780-
safelyCallDestroy(current, destroy);
776+
if ((tag & HookPassive) !== NoHookEffect) {
777+
enqueuePendingPassiveEffectDestroyFn(destroy);
778+
} else {
779+
safelyCallDestroy(current, destroy);
780+
}
781781
}
782782
effect = effect.next;
783783
} while (effect !== firstEffect);
784-
});
784+
} else {
785+
// When the owner fiber is deleted, the destroy function of a passive
786+
// effect hook is called during the synchronous commit phase. This is
787+
// a concession to implementation complexity. Calling it in the
788+
// passive effect phase (like they usually are, when dependencies
789+
// change during an update) would require either traversing the
790+
// children of the deleted fiber again, or including unmount effects
791+
// as part of the fiber effect list.
792+
//
793+
// Because this is during the sync commit phase, we need to change
794+
// the priority.
795+
//
796+
// TODO: Reconsider this implementation trade off.
797+
const priorityLevel =
798+
renderPriorityLevel > NormalPriority
799+
? NormalPriority
800+
: renderPriorityLevel;
801+
runWithPriority(priorityLevel, () => {
802+
let effect = firstEffect;
803+
do {
804+
const destroy = effect.destroy;
805+
if (destroy !== undefined) {
806+
safelyCallDestroy(current, destroy);
807+
}
808+
effect = effect.next;
809+
} while (effect !== firstEffect);
810+
});
811+
}
785812
}
786813
}
787814
return;
@@ -1285,9 +1312,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
12851312
case MemoComponent:
12861313
case SimpleMemoComponent:
12871314
case Chunk: {
1288-
// Note: We currently never use MountMutation, but useLayout uses
1289-
// UnmountMutation.
1290-
commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
1315+
// Layout effects are destroyed during the mutation phase so that all
1316+
// destroy functions for all fibers are called before any create functions.
1317+
// This prevents sibling component effects from interfering with each other,
1318+
// e.g. a destroy function in one component should never override a ref set
1319+
// by a create function in another component during the same commit.
1320+
commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
12911321
return;
12921322
}
12931323
case Profiler: {
@@ -1325,9 +1355,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
13251355
case MemoComponent:
13261356
case SimpleMemoComponent:
13271357
case Chunk: {
1328-
// Note: We currently never use MountMutation, but useLayout uses
1329-
// UnmountMutation.
1330-
commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
1358+
// Layout effects are destroyed during the mutation phase so that all
1359+
// destroy functions for all fibers are called before any create functions.
1360+
// This prevents sibling component effects from interfering with each other,
1361+
// e.g. a destroy function in one component should never override a ref set
1362+
// by a create function in another component during the same commit.
1363+
commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
13311364
return;
13321365
}
13331366
case ClassComponent: {

‎packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,9 @@ import {
2828
Passive as PassiveEffect,
2929
} from 'shared/ReactSideEffectTags';
3030
import {
31-
NoEffect as NoHookEffect,
32-
UnmountMutation,
33-
MountLayout,
34-
UnmountPassive,
35-
MountPassive,
31+
HasEffect as HookHasEffect,
32+
Layout as HookLayout,
33+
Passive as HookPassive,
3634
} from './ReactHookEffectTags';
3735
import {
3836
scheduleWork,
@@ -923,7 +921,12 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
923921
const hook = mountWorkInProgressHook();
924922
const nextDeps = deps === undefined ? null : deps;
925923
currentlyRenderingFiber.effectTag |= fiberEffectTag;
926-
hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
924+
hook.memoizedState = pushEffect(
925+
HookHasEffect | hookEffectTag,
926+
create,
927+
undefined,
928+
nextDeps,
929+
);
927930
}
928931

929932
function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
@@ -937,15 +940,20 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
937940
if (nextDeps !== null) {
938941
const prevDeps = prevEffect.deps;
939942
if (areHookInputsEqual(nextDeps, prevDeps)) {
940-
pushEffect(NoHookEffect, create, destroy, nextDeps);
943+
pushEffect(hookEffectTag, create, destroy, nextDeps);
941944
return;
942945
}
943946
}
944947
}
945948

946949
currentlyRenderingFiber.effectTag |= fiberEffectTag;
947950

948-
hook.memoizedState = pushEffect(hookEffectTag, create, destroy, nextDeps);
951+
hook.memoizedState = pushEffect(
952+
HookHasEffect | hookEffectTag,
953+
create,
954+
destroy,
955+
nextDeps,
956+
);
949957
}
950958

951959
function mountEffect(
@@ -960,7 +968,7 @@ function mountEffect(
960968
}
961969
return mountEffectImpl(
962970
UpdateEffect | PassiveEffect,
963-
UnmountPassive | MountPassive,
971+
HookPassive,
964972
create,
965973
deps,
966974
);
@@ -978,7 +986,7 @@ function updateEffect(
978986
}
979987
return updateEffectImpl(
980988
UpdateEffect | PassiveEffect,
981-
UnmountPassive | MountPassive,
989+
HookPassive,
982990
create,
983991
deps,
984992
);
@@ -988,24 +996,14 @@ function mountLayoutEffect(
988996
create: () => (() => void) | void,
989997
deps: Array<mixed> | void | null,
990998
): void {
991-
return mountEffectImpl(
992-
UpdateEffect,
993-
UnmountMutation | MountLayout,
994-
create,
995-
deps,
996-
);
999+
return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
9971000
}
9981001

9991002
function updateLayoutEffect(
10001003
create: () => (() => void) | void,
10011004
deps: Array<mixed> | void | null,
10021005
): void {
1003-
return updateEffectImpl(
1004-
UpdateEffect,
1005-
UnmountMutation | MountLayout,
1006-
create,
1007-
deps,
1008-
);
1006+
return updateEffectImpl(UpdateEffect, HookLayout, create, deps);
10091007
}
10101008

10111009
function imperativeHandleEffect<T>(
@@ -1059,7 +1057,7 @@ function mountImperativeHandle<T>(
10591057

10601058
return mountEffectImpl(
10611059
UpdateEffect,
1062-
UnmountMutation | MountLayout,
1060+
HookLayout,
10631061
imperativeHandleEffect.bind(null, create, ref),
10641062
effectDeps,
10651063
);
@@ -1086,7 +1084,7 @@ function updateImperativeHandle<T>(
10861084

10871085
return updateEffectImpl(
10881086
UpdateEffect,
1089-
UnmountMutation | MountLayout,
1087+
HookLayout,
10901088
imperativeHandleEffect.bind(null, create, ref),
10911089
effectDeps,
10921090
);

‎packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import type {Hook} from './ReactFiberHooks';
1818

1919
import {
2020
warnAboutDeprecatedLifecycles,
21+
deferPassiveEffectCleanupDuringUnmount,
2122
enableUserTimingAPI,
2223
enableSuspenseServerRenderer,
2324
replayFailedUnitOfWorkWithInvokeGuardedCallback,
@@ -257,6 +258,7 @@ let rootDoesHavePassiveEffects: boolean = false;
257258
let rootWithPendingPassiveEffects: FiberRoot | null = null;
258259
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
259260
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
261+
let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
260262

261263
let rootsWithPendingDiscreteUpdates: Map<
262264
FiberRoot,
@@ -2176,6 +2178,21 @@ export function flushPassiveEffects() {
21762178
}
21772179
}
21782180

2181+
export function enqueuePendingPassiveEffectDestroyFn(
2182+
destroy: () => void,
2183+
): void {
2184+
if (deferPassiveEffectCleanupDuringUnmount) {
2185+
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
2186+
if (!rootDoesHavePassiveEffects) {
2187+
rootDoesHavePassiveEffects = true;
2188+
scheduleCallback(NormalPriority, () => {
2189+
flushPassiveEffects();
2190+
return null;
2191+
});
2192+
}
2193+
}
2194+
}
2195+
21792196
function flushPassiveEffectsImpl() {
21802197
if (rootWithPendingPassiveEffects === null) {
21812198
return false;
@@ -2193,6 +2210,20 @@ function flushPassiveEffectsImpl() {
21932210
executionContext |= CommitContext;
21942211
const prevInteractions = pushInteractions(root);
21952212

2213+
if (deferPassiveEffectCleanupDuringUnmount) {
2214+
// Flush any pending passive effect destroy functions that belong to
2215+
// components that were unmounted during the most recent commit.
2216+
for (
2217+
let i = 0;
2218+
i < pendingUnmountedPassiveEffectDestroyFunctions.length;
2219+
i++
2220+
) {
2221+
const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
2222+
invokeGuardedCallback(null, destroy, null);
2223+
}
2224+
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
2225+
}
2226+
21962227
// Note: This currently assumes there are no passive effects on the root
21972228
// fiber, because the root is not part of its own effect list. This could
21982229
// change in the future.

‎packages/react-reconciler/src/ReactHookEffectTags.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@
99

1010
export type HookEffectTag = number;
1111

12-
export const NoEffect = /* */ 0b00000000;
13-
export const UnmountSnapshot = /* */ 0b00000010;
14-
export const UnmountMutation = /* */ 0b00000100;
15-
export const MountMutation = /* */ 0b00001000;
16-
export const UnmountLayout = /* */ 0b00010000;
17-
export const MountLayout = /* */ 0b00100000;
18-
export const MountPassive = /* */ 0b01000000;
19-
export const UnmountPassive = /* */ 0b10000000;
12+
export const NoEffect = /* */ 0b000;
13+
14+
// Represents whether effect should fire.
15+
export const HasEffect = /* */ 0b001;
16+
17+
// Represents the phase in which the effect (not the clean-up) fires.
18+
export const Layout = /* */ 0b010;
19+
export const Passive = /* */ 0b100;

0 commit comments

Comments
 (0)