Skip to content

Commit

Permalink
Flush useEffect clean up functions in the passive effects phase
Browse files Browse the repository at this point in the history 
This is a change in behavior that may cause broken product code, so it has been added behind a killswitch (deferPassiveEffectCleanupDuringUnmount)
  • Loading branch information
Brian Vaughn committed Jan 31, 2020
1 parent 38cd758 commit 9a38b12
Show file tree
Hide file tree
Showing 13 changed files with 213 additions and 45 deletions.
73 changes: 49 additions & 24 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';

import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';
import {
deferPassiveEffectCleanupDuringUnmount,
enableSchedulerTracing,
enableProfilerTimer,
enableSuspenseServerRenderer,
Expand Down Expand Up @@ -109,9 +110,11 @@ import {
captureCommitPhaseError,
resolveRetryThenable,
markCommitTimeOfFallback,
enqueuePendingPassiveEffectDestroyFn,
} from './ReactFiberWorkLoop';
import {
NoEffect as NoHookEffect,
NoEffectPassiveUnmountFiber,
UnmountSnapshot,
UnmountMutation,
MountMutation,
Expand Down Expand Up @@ -756,32 +759,50 @@ function commitUnmount(
if (lastEffect !== null) {
const firstEffect = lastEffect.next;

// When the owner fiber is deleted, the destroy function of a passive
// effect hook is called during the synchronous commit phase. This is
// a concession to implementation complexity. Calling it in the
// passive effect phase (like they usually are, when dependencies
// change during an update) would require either traversing the
// children of the deleted fiber again, or including unmount effects
// as part of the fiber effect list.
//
// Because this is during the sync commit phase, we need to change
// the priority.
//
// TODO: Reconsider this implementation trade off.
const priorityLevel =
renderPriorityLevel > NormalPriority
? NormalPriority
: renderPriorityLevel;
runWithPriority(priorityLevel, () => {
if (deferPassiveEffectCleanupDuringUnmount) {
let effect = firstEffect;
do {
const destroy = effect.destroy;
const {destroy, tag} = effect;
if (destroy !== undefined) {
safelyCallDestroy(current, destroy);
if (
(tag & UnmountPassive) !== NoHookEffect ||
(tag & NoEffectPassiveUnmountFiber) !== NoHookEffect
) {
enqueuePendingPassiveEffectDestroyFn(destroy);
} else {
safelyCallDestroy(current, destroy);
}
}
effect = effect.next;
} while (effect !== firstEffect);
});
} else {
// When the owner fiber is deleted, the destroy function of a passive
// effect hook is called during the synchronous commit phase. This is
// a concession to implementation complexity. Calling it in the
// passive effect phase (like they usually are, when dependencies
// change during an update) would require either traversing the
// children of the deleted fiber again, or including unmount effects
// as part of the fiber effect list.
//
// Because this is during the sync commit phase, we need to change
// the priority.
//
// TODO: Reconsider this implementation trade off.
const priorityLevel =
renderPriorityLevel > NormalPriority
? NormalPriority
: renderPriorityLevel;
runWithPriority(priorityLevel, () => {
let effect = firstEffect;
do {
const destroy = effect.destroy;
if (destroy !== undefined) {
safelyCallDestroy(current, destroy);
}
effect = effect.next;
} while (effect !== firstEffect);
});
}
}
}
return;
Expand Down Expand Up @@ -1285,8 +1306,10 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case MemoComponent:
case SimpleMemoComponent:
case Chunk: {
// Note: We currently never use MountMutation, but useLayout uses
// UnmountMutation.
// Note: We currently never use MountMutation, but useLayoutEffect uses UnmountMutation.
// This is to ensure ALL destroy fns are run before create fns,
// without requiring us to traverse the effects list an extra time during commit.
// This sequence prevents sibling destroy and create fns from interfering with each other.
commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
return;
}
Expand Down Expand Up @@ -1325,8 +1348,10 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case MemoComponent:
case SimpleMemoComponent:
case Chunk: {
// Note: We currently never use MountMutation, but useLayout uses
// UnmountMutation.
// Note: We currently never use MountMutation, but useLayoutEffect uses UnmountMutation.
// This is to ensure ALL destroy fns are run before create fns,
// without requiring us to traverse the effects list an extra time during commit.
// This sequence prevents sibling destroy and create fns from interfering with each other.
commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
return;
}
Expand Down
16 changes: 14 additions & 2 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
} from 'shared/ReactSideEffectTags';
import {
NoEffect as NoHookEffect,
NoEffectPassiveUnmountFiber,
UnmountMutation,
MountLayout,
UnmountPassive,
Expand Down Expand Up @@ -926,7 +927,13 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
}

function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
function updateEffectImpl(
fiberEffectTag,
hookEffectTag,
noWorkUnmountFiberHookEffectTag,
create,
deps,
): void {
const hook = updateWorkInProgressHook();
const nextDeps = deps === undefined ? null : deps;
let destroy = undefined;
Expand All @@ -937,7 +944,7 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
if (nextDeps !== null) {
const prevDeps = prevEffect.deps;
if (areHookInputsEqual(nextDeps, prevDeps)) {
pushEffect(NoHookEffect, create, destroy, nextDeps);
pushEffect(noWorkUnmountFiberHookEffectTag, create, destroy, nextDeps);
return;
}
}
Expand Down Expand Up @@ -979,6 +986,7 @@ function updateEffect(
return updateEffectImpl(
UpdateEffect | PassiveEffect,
UnmountPassive | MountPassive,
NoEffectPassiveUnmountFiber,
create,
deps,
);
Expand All @@ -1000,9 +1008,12 @@ function updateLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
// Layout effects use UnmountMutation to ensure all destroy fns are run before create fns.
// This optimization lets us avoid traversing the effects list an extra time during the layout phase.
return updateEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
NoHookEffect,
create,
deps,
);
Expand Down Expand Up @@ -1087,6 +1098,7 @@ function updateImperativeHandle<T>(
return updateEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
NoHookEffect,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
);
Expand Down
29 changes: 29 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type {Hook} from './ReactFiberHooks';

import {
warnAboutDeprecatedLifecycles,
deferPassiveEffectCleanupDuringUnmount,
enableUserTimingAPI,
enableSuspenseServerRenderer,
replayFailedUnitOfWorkWithInvokeGuardedCallback,
Expand Down Expand Up @@ -257,6 +258,7 @@ let rootDoesHavePassiveEffects: boolean = false;
let rootWithPendingPassiveEffects: FiberRoot | null = null;
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = [];

let rootsWithPendingDiscreteUpdates: Map<
FiberRoot,
Expand Down Expand Up @@ -2176,6 +2178,19 @@ export function flushPassiveEffects() {
}
}

export function enqueuePendingPassiveEffectDestroyFn(
destroy: () => void,
): void {
if (deferPassiveEffectCleanupDuringUnmount) {
pendingUnmountedPassiveEffectDestroyFunctions.push(destroy);
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
flushPassiveEffects();
return null;
});
}
}

function flushPassiveEffectsImpl() {
if (rootWithPendingPassiveEffects === null) {
return false;
Expand All @@ -2193,6 +2208,20 @@ function flushPassiveEffectsImpl() {
executionContext |= CommitContext;
const prevInteractions = pushInteractions(root);

if (deferPassiveEffectCleanupDuringUnmount) {
// Flush any pending passive effect destroy functions that belong to
// components that were unmounted during the most recent commit.
for (
let i = 0;
i < pendingUnmountedPassiveEffectDestroyFunctions.length;
i++
) {
const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i];
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.
Expand Down
17 changes: 9 additions & 8 deletions packages/react-reconciler/src/ReactHookEffectTags.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@

export type HookEffectTag = number;

export const NoEffect = /* */ 0b00000000;
export const UnmountSnapshot = /* */ 0b00000010;
export const UnmountMutation = /* */ 0b00000100;
export const MountMutation = /* */ 0b00001000;
export const UnmountLayout = /* */ 0b00010000;
export const MountLayout = /* */ 0b00100000;
export const MountPassive = /* */ 0b01000000;
export const UnmountPassive = /* */ 0b10000000;
export const NoEffect = /* */ 0b000000000;
export const UnmountSnapshot = /* */ 0b000000010;
export const UnmountMutation = /* */ 0b000000100;
export const MountMutation = /* */ 0b000001000;
export const UnmountLayout = /* */ 0b000010000;
export const MountLayout = /* */ 0b000100000;
export const MountPassive = /* */ 0b001000000;
export const UnmountPassive = /* */ 0b010000000;
export const NoEffectPassiveUnmountFiber = /**/ 0b100000000;
106 changes: 97 additions & 9 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ describe('ReactHooksWithNoopRenderer', () => {
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
ReactFeatureFlags.enableSchedulerTracing = true;
ReactFeatureFlags.flushSuspenseFallbacksInTests = false;
ReactFeatureFlags.deferPassiveEffectCleanupDuringUnmount = true;
React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');
Expand Down Expand Up @@ -972,6 +973,92 @@ describe('ReactHooksWithNoopRenderer', () => {
},
);

it('defers passive effect destroy functions during unmount', () => {
function Child({count}) {
React.useEffect(() => {
Scheduler.unstable_yieldValue('passive create (no dependencies)');
return () => {
Scheduler.unstable_yieldValue('passive destroy (no dependencies)');
};
}, []);
React.useLayoutEffect(() => {
Scheduler.unstable_yieldValue('layout create (no dependencies)');
return () => {
Scheduler.unstable_yieldValue('layout destroy (no dependencies)');
};
}, []);
React.useEffect(() => {
Scheduler.unstable_yieldValue('passive create');
return () => {
Scheduler.unstable_yieldValue('passive destroy');
};
}, [count]);
React.useLayoutEffect(() => {
Scheduler.unstable_yieldValue('layout create');
return () => {
Scheduler.unstable_yieldValue('layout destroy');
};
}, [count]);
Scheduler.unstable_yieldValue('render');
return null;
}

act(() => {
ReactNoop.render(<Child count={1} />, () =>
Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough([
'render',
'layout create (no dependencies)',
'layout create',
'Sync effect',
]);
// Effects are deferred until after the commit
expect(Scheduler).toFlushAndYield([
'passive create (no dependencies)',
'passive create',
]);
});

// HACK
// This update is kind of gross since it exists to test an internal implementation detail:
// Effects without updating dependencies lose their layout/passive tag during an update.
// A special type of no-update tag (NoEffectPassiveUnmountFiber) is used to track these for later.
act(() => {
ReactNoop.render(<Child count={2} />, () =>
Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough([
'render',
'layout destroy',
'layout create',
'Sync effect',
]);
// Effects are deferred until after the commit
expect(Scheduler).toFlushAndYield([
'passive destroy',
'passive create',
]);
});

// Unmount the component and verify that passive destroy functions are deferred until post-commit.
act(() => {
ReactNoop.render(null, () =>
Scheduler.unstable_yieldValue('Sync effect'),
);
expect(Scheduler).toFlushAndYieldThrough([
'layout destroy (no dependencies)',
'layout destroy',
'Sync effect',
]);
// Effects are deferred until after the commit
expect(Scheduler).toFlushAndYield([
'passive destroy (no dependencies)',
'passive destroy',
]);
});
});

it('updates have async priority', () => {
function Counter(props) {
const [count, updateCount] = useState('(empty)');
Expand Down Expand Up @@ -1554,12 +1641,14 @@ describe('ReactHooksWithNoopRenderer', () => {
'Unmount B [0]',
'Mount A [1]',
'Oops!',
// Clean up effect A. There's no effect B to clean-up, because it
// never mounted.
'Unmount A [1]',
]);
expect(ReactNoop.getChildren()).toEqual([]);
});
expect(Scheduler).toHaveYielded([
// Clean up effect A runs passively on unmount.
// There's no effect B to clean-up, because it never mounted.
'Unmount A [1]',
]);
});

it('handles errors on unmount', () => {
Expand Down Expand Up @@ -1599,13 +1688,12 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(Scheduler).toFlushAndYieldThrough(['Count: 1', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops');
expect(Scheduler).toHaveYielded([
'Oops!',
// B unmounts even though an error was thrown in the previous effect
'Unmount B [0]',
]);
expect(ReactNoop.getChildren()).toEqual([]);
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
expect(Scheduler).toHaveYielded(['Unmount B [0]']);
expect(ReactNoop.getChildren()).toEqual([]);
});

it('works with memo', () => {
Expand Down
Loading

0 comments on commit 9a38b12

Please sign in to comment.