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).
  • Loading branch information
Brian Vaughn committed Jan 31, 2020
1 parent 1c6be34 commit f8aad4e
Show file tree
Hide file tree
Showing 3 changed files with 206 additions and 29 deletions.
32 changes: 32 additions & 0 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,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: {
commitHookEffectList(HookPassive, NoHookEffect, 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: {
commitHookEffectList(NoHookEffect, HookPassive, finishedWork);
break;
}
default:
break;
}
}
}

function commitLifeCycles(
finishedRoot: FiberRoot,
current: Fiber | null,
Expand Down
132 changes: 108 additions & 24 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
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,116 @@ 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;
let effectWithErrorDuringUnmount = null;
while (effect !== null) {
if (__DEV__) {
setCurrentDebugFiberInDEV(effect);
invokeGuardedCallback(
null,
commitPassiveHookUnmountEffects,
null,
effect,
);
if (hasCaughtError()) {
effectWithErrorDuringUnmount = effect;
invariant(effect !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(effect, error);
}
resetCurrentDebugFiberInDEV();
} else {
try {
commitPassiveHookUnmountEffects(effect);
} catch (error) {
effectWithErrorDuringUnmount = effect;
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) {
// Don't run create effects for a Fiber that errored during destroy.
// This check is in place to match previous behavior.
// TODO: Rethink whether we want to carry this behavior forward.
if (effectWithErrorDuringUnmount !== effect) {
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
71 changes: 66 additions & 5 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
View file
Original file line number Diff line number Diff line change
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,7 +1739,7 @@ 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'),
);
Expand All @@ -1688,8 +1748,9 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
expect(Scheduler).toHaveYielded(['Oops!']);
});
// 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

// <Counter> gets unmounted because an error is thrown above.
// The remaining destroy function gets runs later on unmount, since it's passive
expect(Scheduler).toHaveYielded(['Unmount B [0]']);
expect(ReactNoop.getChildren()).toEqual([]);
});
Expand Down

0 comments on commit f8aad4e

Please sign in to comment.