Skip to content

Commit

Permalink
Flush all passive destroy fns before calling create fns
Browse files Browse the repository at this point in the history 
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).
  • Loading branch information
Brian Vaughn committed Feb 4, 2020
1 parent 812277d commit 0769c1f
Show file tree
Hide file tree
Showing 3 changed files with 210 additions and 31 deletions.
32 changes: 32 additions & 0 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Expand Up @@ -416,6 +416,38 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
}
}

export function commitPassiveHookUnmountEffects(finishedWork: Fiber): void {
if ((finishedWork.effectTag & Passive) !== NoEffect) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
break;
}
default:
break;
}
}
}

export function commitPassiveHookMountEffects(finishedWork: Fiber): void {
if ((finishedWork.effectTag & Passive) !== NoEffect) {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
break;
}
default:
break;
}
}
}

function commitLifeCycles(
finishedRoot: FiberRoot,
current: Fiber | null,
Expand Down
124 changes: 100 additions & 24 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Expand Up @@ -133,6 +133,8 @@ import {
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
commitLifeCycles as commitLayoutEffectOnFiber,
commitPassiveHookEffects,
commitPassiveHookUnmountEffects,
commitPassiveHookMountEffects,
commitPlacement,
commitWork,
commitDeletion,
Expand Down Expand Up @@ -2222,34 +2224,108 @@ function flushPassiveEffectsImpl() {
invokeGuardedCallback(null, destroy, null);
}
pendingUnmountedPassiveEffectDestroyFunctions.length = 0;
}

// Note: This currently assumes there are no passive effects on the root
// fiber, because the root is not part of its own effect list. This could
// change in the future.
let effect = root.current.firstEffect;
while (effect !== null) {
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
if (hasCaughtError()) {
invariant(effect !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
// It's important that ALL pending passive effect destroy functions are called
// before ANY passive effect create functions are called.
// Otherwise effects in sibling components might 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.
// Layout effects have the same constraint.

// First pass: Destroy stale passive effects.
//
// Note: This currently assumes there are no passive effects on the root fiber
// because the root is not part of its own effect list.
// This could change in the future.
let effect = root.current.firstEffect;
while (effect !== null) {
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(
null,
commitPassiveHookUnmountEffects,
null,
effect,
);
if (hasCaughtError()) {
invariant(effect !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookUnmountEffects(effect);
} catch (error) {
invariant(effect !== null, 'Should be working on an effect.');
captureCommitPhaseError(effect, error);
}
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookEffects(effect);
} catch (error) {
invariant(effect !== null, 'Should be working on an effect.');
captureCommitPhaseError(effect, error);
effect = effect.nextEffect;
}

// Second pass: Create new passive effects.
//
// Note: This currently assumes there are no passive effects on the root fiber
// because the root is not part of its own effect list.
// This could change in the future.
effect = root.current.firstEffect;
while (effect !== null) {
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(
null,
commitPassiveHookMountEffects,
null,
effect,
);
if (hasCaughtError()) {
invariant(effect !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookMountEffects(effect);
} catch (error) {
invariant(effect !== null, 'Should be working on an effect.');
captureCommitPhaseError(effect, error);
}
}
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
effect = nextNextEffect;
}
} else {
// Note: This currently assumes there are no passive effects on the root fiber
// because the root is not part of its own effect list.
// This could change in the future.
let effect = root.current.firstEffect;
while (effect !== null) {
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(null, commitPassiveHookEffects, null, effect);
if (hasCaughtError()) {
invariant(effect !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookEffects(effect);
} catch (error) {
invariant(effect !== null, 'Should be working on an effect.');
captureCommitPhaseError(effect, error);
}
}
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
effect = nextNextEffect;
}
const nextNextEffect = effect.nextEffect;
// Remove nextEffect pointer to assist GC
effect.nextEffect = null;
effect = nextNextEffect;
}

if (enableSchedulerTracing) {
Expand Down
85 changes: 78 additions & 7 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
View file
Expand Up @@ -1558,6 +1558,67 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});

it('unmounts all previous effects between siblings before creating any new ones', () => {
function Counter({count, label}) {
useEffect(() => {
Scheduler.unstable_yieldValue(`Mount ${label} [${count}]`);
return () => {
Scheduler.unstable_yieldValue(`Unmount ${label} [${count}]`);
};
});
return <Text text={`${label} ${count}`} />;
}
act(() => {
ReactNoop.render(
<React.Fragment>
<Counter label="A" count={0} />
<Counter label="B" count={0} />
</React.Fragment>,
() => Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['A 0', 'B 0', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('A 0'), span('B 0')]);
});

expect(Scheduler).toHaveYielded(['Mount A [0]', 'Mount B [0]']);

act(() => {
ReactNoop.render(
<React.Fragment>
<Counter label="A" count={1} />
<Counter label="B" count={1} />
</React.Fragment>,
() => Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['A 1', 'B 1', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('A 1'), span('B 1')]);
});
expect(Scheduler).toHaveYielded([
'Unmount A [0]',
'Unmount B [0]',
'Mount A [1]',
'Mount B [1]',
]);

act(() => {
ReactNoop.render(
<React.Fragment>
<Counter label="B" count={2} />
<Counter label="C" count={0} />
</React.Fragment>,
() => Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['B 2', 'C 0', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('B 2'), span('C 0')]);
});
expect(Scheduler).toHaveYielded([
'Unmount A [1]',
'Unmount B [1]',
'Mount B [2]',
'Mount C [0]',
]);
});

it('handles errors on mount', () => {
function Counter(props) {
useEffect(() => {
Expand Down Expand Up @@ -1656,8 +1717,6 @@ describe('ReactHooksWithNoopRenderer', () => {
return () => {
Scheduler.unstable_yieldValue('Oops!');
throw new Error('Oops!');
// eslint-disable-next-line no-unreachable
Scheduler.unstable_yieldValue(`Unmount A [${props.count}]`);
};
});
useEffect(() => {
Expand All @@ -1668,6 +1727,7 @@ describe('ReactHooksWithNoopRenderer', () => {
});
return <Text text={'Count: ' + props.count} />;
}

act(() => {
ReactNoop.render(<Counter count={0} />, () =>
Scheduler.unstable_yieldValue('Sync effect'),
Expand All @@ -1679,18 +1739,29 @@ describe('ReactHooksWithNoopRenderer', () => {
});

act(() => {
// This update will trigger an error
// This update will trigger an error during passive effect unmount
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough(['Count: 1', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
expect(Scheduler).toHaveYielded(['Oops!']);

// This tests enables a feature flag that flushes all passive destroys in a
// separate pass before flushing any passive creates.
// A result of this two-pass flush is that an error thrown from unmount does
// not block the subsequent create functions from being run.
expect(Scheduler).toHaveYielded([
'Oops!',
'Mount A [1]',
'Mount B [1]',
]);
});
// B unmounts even though an error was thrown in the previous effect
// B's destroy function runs later on unmount though, since it's passive
expect(Scheduler).toHaveYielded(['Unmount B [0]']);

// <Counter> gets unmounted because an error is thrown above.
// The remaining destroy functions are run later on unmount, since they're passive.
// In this case, one of them throws again (because of how the test is written).
expect(Scheduler).toHaveYielded(['Oops!', 'Unmount B [1]']);
expect(ReactNoop.getChildren()).toEqual([]);
});

Expand Down

0 comments on commit 0769c1f

Please sign in to comment.