Skip to content

Commit f8aad4e

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).
1 parent 1c6be34 commit f8aad4e

File tree

3 files changed

+206
-29
lines changed

3 files changed

+206
-29
lines changed

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

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

417+
export function commitPassiveHookUnmountEffects(finishedWork: Fiber): void {
418+
if ((finishedWork.effectTag & Passive) !== NoEffect) {
419+
switch (finishedWork.tag) {
420+
case FunctionComponent:
421+
case ForwardRef:
422+
case SimpleMemoComponent:
423+
case Chunk: {
424+
commitHookEffectList(HookPassive, NoHookEffect, finishedWork);
425+
break;
426+
}
427+
default:
428+
break;
429+
}
430+
}
431+
}
432+
433+
export function commitPassiveHookMountEffects(finishedWork: Fiber): void {
434+
if ((finishedWork.effectTag & Passive) !== NoEffect) {
435+
switch (finishedWork.tag) {
436+
case FunctionComponent:
437+
case ForwardRef:
438+
case SimpleMemoComponent:
439+
case Chunk: {
440+
commitHookEffectList(NoHookEffect, HookPassive, finishedWork);
441+
break;
442+
}
443+
default:
444+
break;
445+
}
446+
}
447+
}
448+
417449
function commitLifeCycles(
418450
finishedRoot: FiberRoot,
419451
current: Fiber | null,

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

Lines changed: 108 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,8 @@ import {
133133
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
134134
commitLifeCycles as commitLayoutEffectOnFiber,
135135
commitPassiveHookEffects,
136+
commitPassiveHookUnmountEffects,
137+
commitPassiveHookMountEffects,
136138
commitPlacement,
137139
commitWork,
138140
commitDeletion,
@@ -2222,34 +2224,116 @@ function flushPassiveEffectsImpl() {
22222224
invokeGuardedCallback(null, destroy, null);
22232225
}
22242226
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
2225-
}
22262227

2227-
// Note: This currently assumes there are no passive effects on the root
2228-
// fiber, because the root is not part of its own effect list. This could
2229-
// change in the future.
2230-
let effect = root.current.firstEffect;
2231-
while (effect !== null) {
2232-
if (__DEV__) {
2233-
setCurrentDebugFiberInDEV(effect);
2234-
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
2235-
if (hasCaughtError()) {
2236-
invariant(effect !== null, 'Should be working on an effect.');
2237-
const error = clearCaughtError();
2238-
captureCommitPhaseError(effect, error);
2228+
// It's important that ALL pending passive effect destroy functions are called
2229+
// before ANY passive effect create functions are called.
2230+
// Otherwise effects in sibling components might interfere with each other.
2231+
// e.g. a destroy function in one component may unintentionally override a ref
2232+
// value set by a create function in another component.
2233+
// Layout effects have the same constraint.
2234+
2235+
// 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+
let effectWithErrorDuringUnmount = null;
2242+
while (effect !== null) {
2243+
if (__DEV__) {
2244+
setCurrentDebugFiberInDEV(effect);
2245+
invokeGuardedCallback(
2246+
null,
2247+
commitPassiveHookUnmountEffects,
2248+
null,
2249+
effect,
2250+
);
2251+
if (hasCaughtError()) {
2252+
effectWithErrorDuringUnmount = effect;
2253+
invariant(effect !== null, 'Should be working on an effect.');
2254+
const error = clearCaughtError();
2255+
captureCommitPhaseError(effect, error);
2256+
}
2257+
resetCurrentDebugFiberInDEV();
2258+
} else {
2259+
try {
2260+
commitPassiveHookUnmountEffects(effect);
2261+
} catch (error) {
2262+
effectWithErrorDuringUnmount = effect;
2263+
invariant(effect !== null, 'Should be working on an effect.');
2264+
captureCommitPhaseError(effect, error);
2265+
}
22392266
}
2240-
resetCurrentDebugFiberInDEV();
2241-
} else {
2242-
try {
2243-
commitPassiveHookEffects(effect);
2244-
} catch (error) {
2245-
invariant(effect !== null, 'Should be working on an effect.');
2246-
captureCommitPhaseError(effect, error);
2267+
effect = effect.nextEffect;
2268+
}
2269+
2270+
// Second pass: Create new passive effects.
2271+
//
2272+
// Note: This currently assumes there are no passive effects on the root fiber
2273+
// because the root is not part of its own effect list.
2274+
// This could change in the future.
2275+
effect = root.current.firstEffect;
2276+
while (effect !== null) {
2277+
// Don't run create effects for a Fiber that errored during destroy.
2278+
// This check is in place to match previous behavior.
2279+
// TODO: Rethink whether we want to carry this behavior forward.
2280+
if (effectWithErrorDuringUnmount !== effect) {
2281+
if (__DEV__) {
2282+
setCurrentDebugFiberInDEV(effect);
2283+
invokeGuardedCallback(
2284+
null,
2285+
commitPassiveHookMountEffects,
2286+
null,
2287+
effect,
2288+
);
2289+
if (hasCaughtError()) {
2290+
invariant(effect !== null, 'Should be working on an effect.');
2291+
const error = clearCaughtError();
2292+
captureCommitPhaseError(effect, error);
2293+
}
2294+
resetCurrentDebugFiberInDEV();
2295+
} else {
2296+
try {
2297+
commitPassiveHookMountEffects(effect);
2298+
} catch (error) {
2299+
invariant(effect !== null, 'Should be working on an effect.');
2300+
captureCommitPhaseError(effect, error);
2301+
}
2302+
}
2303+
}
2304+
const nextNextEffect = effect.nextEffect;
2305+
// Remove nextEffect pointer to assist GC
2306+
effect.nextEffect = null;
2307+
effect = nextNextEffect;
2308+
}
2309+
} else {
2310+
// Note: This currently assumes there are no passive effects on the root fiber
2311+
// because the root is not part of its own effect list.
2312+
// This could change in the future.
2313+
let effect = root.current.firstEffect;
2314+
while (effect !== null) {
2315+
if (__DEV__) {
2316+
setCurrentDebugFiberInDEV(effect);
2317+
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
2318+
if (hasCaughtError()) {
2319+
invariant(effect !== null, 'Should be working on an effect.');
2320+
const error = clearCaughtError();
2321+
captureCommitPhaseError(effect, error);
2322+
}
2323+
resetCurrentDebugFiberInDEV();
2324+
} else {
2325+
try {
2326+
commitPassiveHookEffects(effect);
2327+
} catch (error) {
2328+
invariant(effect !== null, 'Should be working on an effect.');
2329+
captureCommitPhaseError(effect, error);
2330+
}
22472331
}
2332+
const nextNextEffect = effect.nextEffect;
2333+
// Remove nextEffect pointer to assist GC
2334+
effect.nextEffect = null;
2335+
effect = nextNextEffect;
22482336
}
2249-
const nextNextEffect = effect.nextEffect;
2250-
// Remove nextEffect pointer to assist GC
2251-
effect.nextEffect = null;
2252-
effect = nextNextEffect;
22532337
}
22542338

22552339
if (enableSchedulerTracing) {

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

Lines changed: 66 additions & 5 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,7 +1739,7 @@ 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
);
@@ -1688,8 +1748,9 @@ describe('ReactHooksWithNoopRenderer', () => {
16881748
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
16891749
expect(Scheduler).toHaveYielded(['Oops!']);
16901750
});
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
1751+
1752+
// <Counter> gets unmounted because an error is thrown above.
1753+
// The remaining destroy function gets runs later on unmount, since it's passive
16931754
expect(Scheduler).toHaveYielded(['Unmount B [0]']);
16941755
expect(ReactNoop.getChildren()).toEqual([]);
16951756
});

0 commit comments

Comments
 (0)