Skip to content

Commit 674bfc4

Browse files
author
Brian Vaughn
committed
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.
1 parent 993e110 commit 674bfc4

File tree

4 files changed

+96
-102
lines changed

4 files changed

+96
-102
lines changed

packages/react-reconciler/src/ReactFiberCommitWork.js

Lines changed: 29 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,8 @@ import {
110110
captureCommitPhaseError,
111111
resolveRetryThenable,
112112
markCommitTimeOfFallback,
113-
enqueuePendingPassiveEffectDestroyFn,
113+
enqueuePendingPassiveHookEffectMount,
114+
enqueuePendingPassiveHookEffectUnmount,
114115
} from './ReactFiberWorkLoop';
115116
import {
116117
NoEffect as NoHookEffect,
@@ -396,49 +397,39 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
396397
}
397398
}
398399

399-
export function commitPassiveHookEffects(finishedWork: Fiber): void {
400-
if ((finishedWork.effectTag & Passive) !== NoEffect) {
401-
switch (finishedWork.tag) {
402-
case FunctionComponent:
403-
case ForwardRef:
404-
case SimpleMemoComponent:
405-
case Chunk: {
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);
411-
break;
412-
}
413-
default:
414-
break;
400+
function schedulePassiveEffects(finishedWork: Fiber) {
401+
if (deferPassiveEffectCleanupDuringUnmount) {
402+
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
403+
let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
404+
if (lastEffect !== null) {
405+
const firstEffect = lastEffect.next;
406+
let effect = firstEffect;
407+
do {
408+
const {next, tag} = effect;
409+
if (
410+
(tag & HookPassive) !== NoHookEffect &&
411+
(tag & HookHasEffect) !== NoHookEffect
412+
) {
413+
enqueuePendingPassiveHookEffectUnmount(finishedWork, effect);
414+
enqueuePendingPassiveHookEffectMount(finishedWork, effect);
415+
}
416+
effect = next;
417+
} while (effect !== firstEffect);
415418
}
416419
}
417420
}
418421

419-
export function commitPassiveHookUnmountEffects(finishedWork: Fiber): void {
422+
export function commitPassiveHookEffects(finishedWork: Fiber): void {
420423
if ((finishedWork.effectTag & Passive) !== NoEffect) {
421424
switch (finishedWork.tag) {
422425
case FunctionComponent:
423426
case ForwardRef:
424427
case SimpleMemoComponent:
425428
case Chunk: {
429+
// TODO (#17945) We should call all passive destroy functions (for all fibers)
430+
// before calling any create functions. The current approach only serializes
431+
// these for a single fiber.
426432
commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
427-
break;
428-
}
429-
default:
430-
break;
431-
}
432-
}
433-
}
434-
435-
export function commitPassiveHookMountEffects(finishedWork: Fiber): void {
436-
if ((finishedWork.effectTag & Passive) !== NoEffect) {
437-
switch (finishedWork.tag) {
438-
case FunctionComponent:
439-
case ForwardRef:
440-
case SimpleMemoComponent:
441-
case Chunk: {
442433
commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
443434
break;
444435
}
@@ -464,6 +455,10 @@ function commitLifeCycles(
464455
// e.g. a destroy function in one component should never override a ref set
465456
// by a create function in another component during the same commit.
466457
commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);
458+
459+
if (deferPassiveEffectCleanupDuringUnmount) {
460+
schedulePassiveEffects(finishedWork);
461+
}
467462
return;
468463
}
469464
case ClassComponent: {
@@ -806,7 +801,7 @@ function commitUnmount(
806801
const {destroy, tag} = effect;
807802
if (destroy !== undefined) {
808803
if ((tag & HookPassive) !== NoHookEffect) {
809-
enqueuePendingPassiveEffectDestroyFn(destroy);
804+
enqueuePendingPassiveHookEffectUnmount(current, effect);
810805
} else {
811806
safelyCallDestroy(current, destroy);
812807
}

packages/react-reconciler/src/ReactFiberHooks.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ export type Hook = {|
144144
next: Hook | null,
145145
|};
146146

147-
type Effect = {|
147+
export type Effect = {|
148148
tag: HookEffectTag,
149149
create: () => (() => void) | void,
150150
destroy: (() => void) | void,

packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines changed: 65 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
1414
import type {Interaction} from 'scheduler/src/Tracing';
1515
import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
1616
import type {SuspenseState} from './ReactFiberSuspenseComponent';
17+
import type {Effect as HookEffect} from './ReactFiberHooks';
1718

1819
import {
1920
warnAboutDeprecatedLifecycles,
@@ -132,8 +133,6 @@ import {
132133
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
133134
commitLifeCycles as commitLayoutEffectOnFiber,
134135
commitPassiveHookEffects,
135-
commitPassiveHookUnmountEffects,
136-
commitPassiveHookMountEffects,
137136
commitPlacement,
138137
commitWork,
139138
commitDeletion,
@@ -259,7 +258,8 @@ let rootDoesHavePassiveEffects: boolean = false;
259258
let rootWithPendingPassiveEffects: FiberRoot | null = null;
260259
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
261260
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
262-
let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
261+
let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
262+
let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];
263263

264264
let rootsWithPendingDiscreteUpdates: Map<
265265
FiberRoot,
@@ -2170,11 +2170,28 @@ export function flushPassiveEffects() {
21702170
}
21712171
}
21722172

2173-
export function enqueuePendingPassiveEffectDestroyFn(
2174-
destroy: () => void,
2173+
export function enqueuePendingPassiveHookEffectMount(
2174+
fiber: Fiber,
2175+
effect: HookEffect,
2176+
): void {
2177+
if (deferPassiveEffectCleanupDuringUnmount) {
2178+
pendingPassiveHookEffectsMount.push(effect, fiber);
2179+
if (!rootDoesHavePassiveEffects) {
2180+
rootDoesHavePassiveEffects = true;
2181+
scheduleCallback(NormalPriority, () => {
2182+
flushPassiveEffects();
2183+
return null;
2184+
});
2185+
}
2186+
}
2187+
}
2188+
2189+
export function enqueuePendingPassiveHookEffectUnmount(
2190+
fiber: Fiber,
2191+
effect: HookEffect,
21752192
): void {
21762193
if (deferPassiveEffectCleanupDuringUnmount) {
2177-
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
2194+
pendingPassiveHookEffectsUnmount.push(effect, fiber);
21782195
if (!rootDoesHavePassiveEffects) {
21792196
rootDoesHavePassiveEffects = true;
21802197
scheduleCallback(NormalPriority, () => {
@@ -2185,6 +2202,11 @@ export function enqueuePendingPassiveEffectDestroyFn(
21852202
}
21862203
}
21872204

2205+
function invokePassiveEffectCreate(effect: HookEffect): void {
2206+
const create = effect.create;
2207+
effect.destroy = create();
2208+
}
2209+
21882210
function flushPassiveEffectsImpl() {
21892211
if (rootWithPendingPassiveEffects === null) {
21902212
return false;
@@ -2203,18 +2225,6 @@ function flushPassiveEffectsImpl() {
22032225
const prevInteractions = pushInteractions(root);
22042226

22052227
if (deferPassiveEffectCleanupDuringUnmount) {
2206-
// Flush any pending passive effect destroy functions that belong to
2207-
// components that were unmounted during the most recent commit.
2208-
for (
2209-
let i = 0;
2210-
i < pendingUnmountedPassiveEffectDestroyFunctions.length;
2211-
i++
2212-
) {
2213-
const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
2214-
invokeGuardedCallback(null, destroy, null);
2215-
}
2216-
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
2217-
22182228
// It's important that ALL pending passive effect destroy functions are called
22192229
// before ANY passive effect create functions are called.
22202230
// Otherwise effects in sibling components might interfere with each other.
@@ -2223,70 +2233,58 @@ function flushPassiveEffectsImpl() {
22232233
// Layout effects have the same constraint.
22242234

22252235
// First pass: Destroy stale passive effects.
2226-
//
2227-
// Note: This currently assumes there are no passive effects on the root fiber
2228-
// because the root is not part of its own effect list.
2229-
// This could change in the future.
2230-
let effect = root.current.firstEffect;
2231-
while (effect !== null) {
2232-
if (__DEV__) {
2233-
setCurrentDebugFiberInDEV(effect);
2234-
invokeGuardedCallback(
2235-
null,
2236-
commitPassiveHookUnmountEffects,
2237-
null,
2238-
effect,
2239-
);
2240-
if (hasCaughtError()) {
2241-
invariant(effect !== null, 'Should be working on an effect.');
2242-
const error = clearCaughtError();
2243-
captureCommitPhaseError(effect, error);
2244-
}
2245-
resetCurrentDebugFiberInDEV();
2246-
} else {
2247-
try {
2248-
commitPassiveHookUnmountEffects(effect);
2249-
} catch (error) {
2250-
invariant(effect !== null, 'Should be working on an effect.');
2251-
captureCommitPhaseError(effect, error);
2236+
let unmountEffects = pendingPassiveHookEffectsUnmount;
2237+
pendingPassiveHookEffectsUnmount = [];
2238+
for (let i = 0; i < unmountEffects.length; i += 2) {
2239+
const effect = ((unmountEffects[i]: any): HookEffect);
2240+
const fiber = ((unmountEffects[i + 1]: any): Fiber);
2241+
const destroy = effect.destroy;
2242+
effect.destroy = undefined;
2243+
if (typeof destroy === 'function') {
2244+
if (__DEV__) {
2245+
setCurrentDebugFiberInDEV(fiber);
2246+
invokeGuardedCallback(null, destroy, null);
2247+
if (hasCaughtError()) {
2248+
invariant(fiber !== null, 'Should be working on an effect.');
2249+
const error = clearCaughtError();
2250+
captureCommitPhaseError(fiber, error);
2251+
}
2252+
resetCurrentDebugFiberInDEV();
2253+
} else {
2254+
try {
2255+
destroy();
2256+
} catch (error) {
2257+
invariant(fiber !== null, 'Should be working on an effect.');
2258+
captureCommitPhaseError(fiber, error);
2259+
}
22522260
}
22532261
}
2254-
effect = effect.nextEffect;
22552262
}
22562263

22572264
// Second pass: Create new passive effects.
2258-
//
2259-
// Note: This currently assumes there are no passive effects on the root fiber
2260-
// because the root is not part of its own effect list.
2261-
// This could change in the future.
2262-
effect = root.current.firstEffect;
2263-
while (effect !== null) {
2265+
let mountEffects = pendingPassiveHookEffectsMount;
2266+
pendingPassiveHookEffectsMount = [];
2267+
for (let i = 0; i < mountEffects.length; i += 2) {
2268+
const effect = ((mountEffects[i]: any): HookEffect);
2269+
const fiber = ((mountEffects[i + 1]: any): Fiber);
22642270
if (__DEV__) {
2265-
setCurrentDebugFiberInDEV(effect);
2266-
invokeGuardedCallback(
2267-
null,
2268-
commitPassiveHookMountEffects,
2269-
null,
2270-
effect,
2271-
);
2271+
setCurrentDebugFiberInDEV(fiber);
2272+
invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect);
22722273
if (hasCaughtError()) {
2273-
invariant(effect !== null, 'Should be working on an effect.');
2274+
invariant(fiber !== null, 'Should be working on an effect.');
22742275
const error = clearCaughtError();
2275-
captureCommitPhaseError(effect, error);
2276+
captureCommitPhaseError(fiber, error);
22762277
}
22772278
resetCurrentDebugFiberInDEV();
22782279
} else {
22792280
try {
2280-
commitPassiveHookMountEffects(effect);
2281+
const create = effect.create;
2282+
effect.destroy = create();
22812283
} catch (error) {
2282-
invariant(effect !== null, 'Should be working on an effect.');
2283-
captureCommitPhaseError(effect, error);
2284+
invariant(fiber !== null, 'Should be working on an effect.');
2285+
captureCommitPhaseError(fiber, error);
22842286
}
22852287
}
2286-
const nextNextEffect = effect.nextEffect;
2287-
// Remove nextEffect pointer to assist GC
2288-
effect.nextEffect = null;
2289-
effect = nextNextEffect;
22902288
}
22912289
} else {
22922290
// Note: This currently assumes there are no passive effects on the root fiber

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1753,6 +1753,7 @@ describe('ReactHooksWithNoopRenderer', () => {
17531753
// not block the subsequent create functions from being run.
17541754
expect(Scheduler).toHaveYielded([
17551755
'Oops!',
1756+
'Unmount B [0]',
17561757
'Mount A [1]',
17571758
'Mount B [1]',
17581759
]);

0 commit comments

Comments
 (0)