Skip to content

Commit 41e0752

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 0769c1f commit 41e0752

File tree

4 files changed

+96
-103
lines changed

4 files changed

+96
-103
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 & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +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 {Hook} from './ReactFiberHooks';
17+
import type {Effect as HookEffect, Hook} from './ReactFiberHooks';
1818

1919
import {
2020
warnAboutDeprecatedLifecycles,
@@ -133,8 +133,6 @@ import {
133133
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
134134
commitLifeCycles as commitLayoutEffectOnFiber,
135135
commitPassiveHookEffects,
136-
commitPassiveHookUnmountEffects,
137-
commitPassiveHookMountEffects,
138136
commitPlacement,
139137
commitWork,
140138
commitDeletion,
@@ -260,7 +258,8 @@ let rootDoesHavePassiveEffects: boolean = false;
260258
let rootWithPendingPassiveEffects: FiberRoot | null = null;
261259
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
262260
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
263-
let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];
261+
let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
262+
let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];
264263

265264
let rootsWithPendingDiscreteUpdates: Map<
266265
FiberRoot,
@@ -2180,11 +2179,28 @@ export function flushPassiveEffects() {
21802179
}
21812180
}
21822181

2183-
export function enqueuePendingPassiveEffectDestroyFn(
2184-
destroy: () => void,
2182+
export function enqueuePendingPassiveHookEffectMount(
2183+
fiber: Fiber,
2184+
effect: HookEffect,
2185+
): void {
2186+
if (deferPassiveEffectCleanupDuringUnmount) {
2187+
pendingPassiveHookEffectsMount.push(effect, fiber);
2188+
if (!rootDoesHavePassiveEffects) {
2189+
rootDoesHavePassiveEffects = true;
2190+
scheduleCallback(NormalPriority, () => {
2191+
flushPassiveEffects();
2192+
return null;
2193+
});
2194+
}
2195+
}
2196+
}
2197+
2198+
export function enqueuePendingPassiveHookEffectUnmount(
2199+
fiber: Fiber,
2200+
effect: HookEffect,
21852201
): void {
21862202
if (deferPassiveEffectCleanupDuringUnmount) {
2187-
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
2203+
pendingPassiveHookEffectsUnmount.push(effect, fiber);
21882204
if (!rootDoesHavePassiveEffects) {
21892205
rootDoesHavePassiveEffects = true;
21902206
scheduleCallback(NormalPriority, () => {
@@ -2195,6 +2211,11 @@ export function enqueuePendingPassiveEffectDestroyFn(
21952211
}
21962212
}
21972213

2214+
function invokePassiveEffectCreate(effect: HookEffect): void {
2215+
const create = effect.create;
2216+
effect.destroy = create();
2217+
}
2218+
21982219
function flushPassiveEffectsImpl() {
21992220
if (rootWithPendingPassiveEffects === null) {
22002221
return false;
@@ -2213,18 +2234,6 @@ function flushPassiveEffectsImpl() {
22132234
const prevInteractions = pushInteractions(root);
22142235

22152236
if (deferPassiveEffectCleanupDuringUnmount) {
2216-
// Flush any pending passive effect destroy functions that belong to
2217-
// components that were unmounted during the most recent commit.
2218-
for (
2219-
let i = 0;
2220-
i < pendingUnmountedPassiveEffectDestroyFunctions.length;
2221-
i++
2222-
) {
2223-
const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
2224-
invokeGuardedCallback(null, destroy, null);
2225-
}
2226-
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
2227-
22282237
// It's important that ALL pending passive effect destroy functions are called
22292238
// before ANY passive effect create functions are called.
22302239
// Otherwise effects in sibling components might interfere with each other.
@@ -2233,70 +2242,58 @@ function flushPassiveEffectsImpl() {
22332242
// Layout effects have the same constraint.
22342243

22352244
// First pass: Destroy stale passive effects.
2236-
//
2237-
// Note: This currently assumes there are no passive effects on the root fiber
2238-
// because the root is not part of its own effect list.
2239-
// This could change in the future.
2240-
let effect = root.current.firstEffect;
2241-
while (effect !== null) {
2242-
if (__DEV__) {
2243-
setCurrentDebugFiberInDEV(effect);
2244-
invokeGuardedCallback(
2245-
null,
2246-
commitPassiveHookUnmountEffects,
2247-
null,
2248-
effect,
2249-
);
2250-
if (hasCaughtError()) {
2251-
invariant(effect !== null, 'Should be working on an effect.');
2252-
const error = clearCaughtError();
2253-
captureCommitPhaseError(effect, error);
2254-
}
2255-
resetCurrentDebugFiberInDEV();
2256-
} else {
2257-
try {
2258-
commitPassiveHookUnmountEffects(effect);
2259-
} catch (error) {
2260-
invariant(effect !== null, 'Should be working on an effect.');
2261-
captureCommitPhaseError(effect, error);
2245+
let unmountEffects = pendingPassiveHookEffectsUnmount;
2246+
pendingPassiveHookEffectsUnmount = [];
2247+
for (let i = 0; i < unmountEffects.length; i += 2) {
2248+
const effect = ((unmountEffects[i]: any): HookEffect);
2249+
const fiber = ((unmountEffects[i + 1]: any): Fiber);
2250+
const destroy = effect.destroy;
2251+
effect.destroy = undefined;
2252+
if (typeof destroy === 'function') {
2253+
if (__DEV__) {
2254+
setCurrentDebugFiberInDEV(fiber);
2255+
invokeGuardedCallback(null, destroy, null);
2256+
if (hasCaughtError()) {
2257+
invariant(fiber !== null, 'Should be working on an effect.');
2258+
const error = clearCaughtError();
2259+
captureCommitPhaseError(fiber, error);
2260+
}
2261+
resetCurrentDebugFiberInDEV();
2262+
} else {
2263+
try {
2264+
destroy();
2265+
} catch (error) {
2266+
invariant(fiber !== null, 'Should be working on an effect.');
2267+
captureCommitPhaseError(fiber, error);
2268+
}
22622269
}
22632270
}
2264-
effect = effect.nextEffect;
22652271
}
22662272

22672273
// Second pass: Create new passive effects.
2268-
//
2269-
// Note: This currently assumes there are no passive effects on the root fiber
2270-
// because the root is not part of its own effect list.
2271-
// This could change in the future.
2272-
effect = root.current.firstEffect;
2273-
while (effect !== null) {
2274+
let mountEffects = pendingPassiveHookEffectsMount;
2275+
pendingPassiveHookEffectsMount = [];
2276+
for (let i = 0; i < mountEffects.length; i += 2) {
2277+
const effect = ((mountEffects[i]: any): HookEffect);
2278+
const fiber = ((mountEffects[i + 1]: any): Fiber);
22742279
if (__DEV__) {
2275-
setCurrentDebugFiberInDEV(effect);
2276-
invokeGuardedCallback(
2277-
null,
2278-
commitPassiveHookMountEffects,
2279-
null,
2280-
effect,
2281-
);
2280+
setCurrentDebugFiberInDEV(fiber);
2281+
invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect);
22822282
if (hasCaughtError()) {
2283-
invariant(effect !== null, 'Should be working on an effect.');
2283+
invariant(fiber !== null, 'Should be working on an effect.');
22842284
const error = clearCaughtError();
2285-
captureCommitPhaseError(effect, error);
2285+
captureCommitPhaseError(fiber, error);
22862286
}
22872287
resetCurrentDebugFiberInDEV();
22882288
} else {
22892289
try {
2290-
commitPassiveHookMountEffects(effect);
2290+
const create = effect.create;
2291+
effect.destroy = create();
22912292
} catch (error) {
2292-
invariant(effect !== null, 'Should be working on an effect.');
2293-
captureCommitPhaseError(effect, error);
2293+
invariant(fiber !== null, 'Should be working on an effect.');
2294+
captureCommitPhaseError(fiber, error);
22942295
}
22952296
}
2296-
const nextNextEffect = effect.nextEffect;
2297-
// Remove nextEffect pointer to assist GC
2298-
effect.nextEffect = null;
2299-
effect = nextNextEffect;
23002297
}
23012298
} else {
23022299
// 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)