Skip to content

Commit 993e110

Browse files
author
Brian Vaughn
committed
Flush all passive destroy fns before calling create fns
Previously we only flushed destroy functions for a single fiber. The reason this is important is that interleaving destroy/create effects between sibling components might cause components to interfere with each other (e.g. a destroy function in one component may unintentionally override a ref value set by a create function in another component). This PR builds on top of the recently added deferPassiveEffectCleanupDuringUnmount kill switch to separate passive effects flushing into two separate phases (similar to layout effects).
1 parent b644b95 commit 993e110

File tree

3 files changed

+210
-31
lines changed

3 files changed

+210
-31
lines changed

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -416,6 +416,38 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
416416
}
417417
}
418418

419+
export function commitPassiveHookUnmountEffects(finishedWork: Fiber): void {
420+
if ((finishedWork.effectTag & Passive) !== NoEffect) {
421+
switch (finishedWork.tag) {
422+
case FunctionComponent:
423+
case ForwardRef:
424+
case SimpleMemoComponent:
425+
case Chunk: {
426+
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: {
442+
commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
443+
break;
444+
}
445+
default:
446+
break;
447+
}
448+
}
449+
}
450+
419451
function commitLifeCycles(
420452
finishedRoot: FiberRoot,
421453
current: Fiber | null,

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

Lines changed: 100 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,8 @@ import {
132132
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
133133
commitLifeCycles as commitLayoutEffectOnFiber,
134134
commitPassiveHookEffects,
135+
commitPassiveHookUnmountEffects,
136+
commitPassiveHookMountEffects,
135137
commitPlacement,
136138
commitWork,
137139
commitDeletion,
@@ -2212,34 +2214,108 @@ function flushPassiveEffectsImpl() {
22122214
invokeGuardedCallback(null, destroy, null);
22132215
}
22142216
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
2215-
}
22162217

2217-
// Note: This currently assumes there are no passive effects on the root
2218-
// fiber, because the root is not part of its own effect list. This could
2219-
// change in the future.
2220-
let effect = root.current.firstEffect;
2221-
while (effect !== null) {
2222-
if (__DEV__) {
2223-
setCurrentDebugFiberInDEV(effect);
2224-
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
2225-
if (hasCaughtError()) {
2226-
invariant(effect !== null, 'Should be working on an effect.');
2227-
const error = clearCaughtError();
2228-
captureCommitPhaseError(effect, error);
2218+
// It's important that ALL pending passive effect destroy functions are called
2219+
// before ANY passive effect create functions are called.
2220+
// Otherwise effects in sibling components might interfere with each other.
2221+
// e.g. a destroy function in one component may unintentionally override a ref
2222+
// value set by a create function in another component.
2223+
// Layout effects have the same constraint.
2224+
2225+
// 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);
2252+
}
22292253
}
2230-
resetCurrentDebugFiberInDEV();
2231-
} else {
2232-
try {
2233-
commitPassiveHookEffects(effect);
2234-
} catch (error) {
2235-
invariant(effect !== null, 'Should be working on an effect.');
2236-
captureCommitPhaseError(effect, error);
2254+
effect = effect.nextEffect;
2255+
}
2256+
2257+
// 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) {
2264+
if (__DEV__) {
2265+
setCurrentDebugFiberInDEV(effect);
2266+
invokeGuardedCallback(
2267+
null,
2268+
commitPassiveHookMountEffects,
2269+
null,
2270+
effect,
2271+
);
2272+
if (hasCaughtError()) {
2273+
invariant(effect !== null, 'Should be working on an effect.');
2274+
const error = clearCaughtError();
2275+
captureCommitPhaseError(effect, error);
2276+
}
2277+
resetCurrentDebugFiberInDEV();
2278+
} else {
2279+
try {
2280+
commitPassiveHookMountEffects(effect);
2281+
} catch (error) {
2282+
invariant(effect !== null, 'Should be working on an effect.');
2283+
captureCommitPhaseError(effect, error);
2284+
}
2285+
}
2286+
const nextNextEffect = effect.nextEffect;
2287+
// Remove nextEffect pointer to assist GC
2288+
effect.nextEffect = null;
2289+
effect = nextNextEffect;
2290+
}
2291+
} else {
2292+
// Note: This currently assumes there are no passive effects on the root fiber
2293+
// because the root is not part of its own effect list.
2294+
// This could change in the future.
2295+
let effect = root.current.firstEffect;
2296+
while (effect !== null) {
2297+
if (__DEV__) {
2298+
setCurrentDebugFiberInDEV(effect);
2299+
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
2300+
if (hasCaughtError()) {
2301+
invariant(effect !== null, 'Should be working on an effect.');
2302+
const error = clearCaughtError();
2303+
captureCommitPhaseError(effect, error);
2304+
}
2305+
resetCurrentDebugFiberInDEV();
2306+
} else {
2307+
try {
2308+
commitPassiveHookEffects(effect);
2309+
} catch (error) {
2310+
invariant(effect !== null, 'Should be working on an effect.');
2311+
captureCommitPhaseError(effect, error);
2312+
}
22372313
}
2314+
const nextNextEffect = effect.nextEffect;
2315+
// Remove nextEffect pointer to assist GC
2316+
effect.nextEffect = null;
2317+
effect = nextNextEffect;
22382318
}
2239-
const nextNextEffect = effect.nextEffect;
2240-
// Remove nextEffect pointer to assist GC
2241-
effect.nextEffect = null;
2242-
effect = nextNextEffect;
22432319
}
22442320

22452321
if (enableSchedulerTracing) {

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

Lines changed: 78 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1558,6 +1558,67 @@ describe('ReactHooksWithNoopRenderer', () => {
15581558
]);
15591559
});
15601560

1561+
it('unmounts all previous effects between siblings before creating any new ones', () => {
1562+
function Counter({count, label}) {
1563+
useEffect(() => {
1564+
Scheduler.unstable_yieldValue(`Mount ${label} [${count}]`);
1565+
return () => {
1566+
Scheduler.unstable_yieldValue(`Unmount ${label} [${count}]`);
1567+
};
1568+
});
1569+
return <Text text={`${label} ${count}`} />;
1570+
}
1571+
act(() => {
1572+
ReactNoop.render(
1573+
<React.Fragment>
1574+
<Counter label="A" count={0} />
1575+
<Counter label="B" count={0} />
1576+
</React.Fragment>,
1577+
() => Scheduler.unstable_yieldValue('Sync effect'),
1578+
);
1579+
expect(Scheduler).toFlushAndYieldThrough(['A 0', 'B 0', 'Sync effect']);
1580+
expect(ReactNoop.getChildren()).toEqual([span('A 0'), span('B 0')]);
1581+
});
1582+
1583+
expect(Scheduler).toHaveYielded(['Mount A [0]', 'Mount B [0]']);
1584+
1585+
act(() => {
1586+
ReactNoop.render(
1587+
<React.Fragment>
1588+
<Counter label="A" count={1} />
1589+
<Counter label="B" count={1} />
1590+
</React.Fragment>,
1591+
() => Scheduler.unstable_yieldValue('Sync effect'),
1592+
);
1593+
expect(Scheduler).toFlushAndYieldThrough(['A 1', 'B 1', 'Sync effect']);
1594+
expect(ReactNoop.getChildren()).toEqual([span('A 1'), span('B 1')]);
1595+
});
1596+
expect(Scheduler).toHaveYielded([
1597+
'Unmount A [0]',
1598+
'Unmount B [0]',
1599+
'Mount A [1]',
1600+
'Mount B [1]',
1601+
]);
1602+
1603+
act(() => {
1604+
ReactNoop.render(
1605+
<React.Fragment>
1606+
<Counter label="B" count={2} />
1607+
<Counter label="C" count={0} />
1608+
</React.Fragment>,
1609+
() => Scheduler.unstable_yieldValue('Sync effect'),
1610+
);
1611+
expect(Scheduler).toFlushAndYieldThrough(['B 2', 'C 0', 'Sync effect']);
1612+
expect(ReactNoop.getChildren()).toEqual([span('B 2'), span('C 0')]);
1613+
});
1614+
expect(Scheduler).toHaveYielded([
1615+
'Unmount A [1]',
1616+
'Unmount B [1]',
1617+
'Mount B [2]',
1618+
'Mount C [0]',
1619+
]);
1620+
});
1621+
15611622
it('handles errors on mount', () => {
15621623
function Counter(props) {
15631624
useEffect(() => {
@@ -1656,8 +1717,6 @@ describe('ReactHooksWithNoopRenderer', () => {
16561717
return () => {
16571718
Scheduler.unstable_yieldValue('Oops!');
16581719
throw new Error('Oops!');
1659-
// eslint-disable-next-line no-unreachable
1660-
Scheduler.unstable_yieldValue(`Unmount A [${props.count}]`);
16611720
};
16621721
});
16631722
useEffect(() => {
@@ -1668,6 +1727,7 @@ describe('ReactHooksWithNoopRenderer', () => {
16681727
});
16691728
return <Text text={'Count: ' + props.count} />;
16701729
}
1730+
16711731
act(() => {
16721732
ReactNoop.render(<Counter count={0} />, () =>
16731733
Scheduler.unstable_yieldValue('Sync effect'),
@@ -1679,18 +1739,29 @@ describe('ReactHooksWithNoopRenderer', () => {
16791739
});
16801740

16811741
act(() => {
1682-
// This update will trigger an error
1742+
// This update will trigger an error during passive effect unmount
16831743
ReactNoop.render(<Counter count={1} />, () =>
16841744
Scheduler.unstable_yieldValue('Sync effect'),
16851745
);
16861746
expect(Scheduler).toFlushAndYieldThrough(['Count: 1', 'Sync effect']);
16871747
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
16881748
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
1689-
expect(Scheduler).toHaveYielded(['Oops!']);
1749+
1750+
// This tests enables a feature flag that flushes all passive destroys in a
1751+
// separate pass before flushing any passive creates.
1752+
// A result of this two-pass flush is that an error thrown from unmount does
1753+
// not block the subsequent create functions from being run.
1754+
expect(Scheduler).toHaveYielded([
1755+
'Oops!',
1756+
'Mount A [1]',
1757+
'Mount B [1]',
1758+
]);
16901759
});
1691-
// B unmounts even though an error was thrown in the previous effect
1692-
// B's destroy function runs later on unmount though, since it's passive
1693-
expect(Scheduler).toHaveYielded(['Unmount B [0]']);
1760+
1761+
// <Counter> gets unmounted because an error is thrown above.
1762+
// The remaining destroy functions are run later on unmount, since they're passive.
1763+
// In this case, one of them throws again (because of how the test is written).
1764+
expect(Scheduler).toHaveYielded(['Oops!', 'Unmount B [1]']);
16941765
expect(ReactNoop.getChildren()).toEqual([]);
16951766
});
16961767

0 commit comments

Comments
 (0)