Skip to content

Commit

Permalink
Flush useEffect clean up functions in the passive effects phase (#1…
Browse files Browse the repository at this point in the history 
…7925)

* Flush useEffect clean up functions in the passive effects phase

This is a change in behavior that may cause broken product code, so it has been added behind a killswitch (deferPassiveEffectCleanupDuringUnmount)

* Avoid scheduling unnecessary callbacks for cleanup effects

Updated enqueuePendingPassiveEffectDestroyFn() to check rootDoesHavePassiveEffects before scheduling a new callback. This way we'll only schedule (at most) one.

* Updated newly added test for added clarity.

* Cleaned up hooks effect tags

We previously used separate Mount* and Unmount* tags to track hooks work for each phase (snapshot, mutation, layout, and passive). This was somewhat complicated to trace through and there were man tag types we never even used (e.g. UnmountLayout, MountMutation, UnmountSnapshot). In addition to this, it left passive and layout hooks looking the same after renders without changed dependencies, which meant we were unable to reliably defer passive effect destroy functions until after the commit phase.

This commit reduces the effect tag types to only include Layout and Passive and differentiates between work and no-work with an HasEffect flag.

* Disabled deferred passive effects flushing in OSS builds for now

* Split up unmount and mount effects list traversal
  • Loading branch information
Brian Vaughn committed Feb 3, 2020
1 parent 9944bf2 commit 7df32c4
Show file tree
Hide file tree
Showing 13 changed files with 248 additions and 87 deletions.
121 changes: 77 additions & 44 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,16 +110,13 @@ import {
captureCommitPhaseError,
resolveRetryThenable,
markCommitTimeOfFallback,
enqueuePendingPassiveEffectDestroyFn,
} from './ReactFiberWorkLoop';
import {
NoEffect as NoHookEffect,
UnmountSnapshot,
UnmountMutation,
MountMutation,
UnmountLayout,
MountLayout,
UnmountPassive,
MountPassive,
HasEffect as HookHasEffect,
Layout as HookLayout,
Passive as HookPassive,
} from './ReactHookEffectTags';
import {didWarnAboutReassigningProps} from './ReactFiberBeginWork';
import {runWithPriority, NormalPriority} from './SchedulerWithReactIntegration';
Expand Down Expand Up @@ -250,7 +248,6 @@ function commitBeforeMutationLifeCycles(
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork);
return;
}
case ClassComponent: {
Expand Down Expand Up @@ -328,26 +325,34 @@ function commitBeforeMutationLifeCycles(
);
}

function commitHookEffectList(
unmountTag: number,
mountTag: number,
finishedWork: Fiber,
) {
function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) {
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;
let effect = firstEffect;
do {
if ((effect.tag & unmountTag) !== NoHookEffect) {
if ((effect.tag & tag) === tag) {
// Unmount
const destroy = effect.destroy;
effect.destroy = undefined;
if (destroy !== undefined) {
destroy();
}
}
if ((effect.tag & mountTag) !== NoHookEffect) {
effect = effect.next;
} while (effect !== firstEffect);
}
}

function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;
let effect = firstEffect;
do {
if ((effect.tag & tag) === tag) {
// Mount
const create = effect.create;
effect.destroy = create();
Expand Down Expand Up @@ -398,8 +403,11 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void {
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectList(UnmountPassive, NoHookEffect, finishedWork);
commitHookEffectList(NoHookEffect, MountPassive, finishedWork);
// TODO (#17945) We should call all passive destroy functions (for all fibers)
// before calling any create functions. The current approach only serializes
// these for a single fiber.
commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork);
commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork);
break;
}
default:
Expand All @@ -419,7 +427,11 @@ function commitLifeCycles(
case ForwardRef:
case SimpleMemoComponent:
case Chunk: {
commitHookEffectList(UnmountLayout, MountLayout, finishedWork);
// At this point layout effects have already been destroyed (during mutation phase).
// This is done to prevent sibling component effects from interfering with each other,
// e.g. a destroy function in one component should never override a ref set
// by a create function in another component during the same commit.
commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork);
return;
}
case ClassComponent: {
Expand Down Expand Up @@ -756,32 +768,47 @@ 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 & HookPassive) !== 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,9 +1312,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case MemoComponent:
case SimpleMemoComponent:
case Chunk: {
// Note: We currently never use MountMutation, but useLayout uses
// UnmountMutation.
commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
// Layout effects are destroyed during the mutation phase so that all
// destroy functions for all fibers are called before any create functions.
// This prevents sibling component effects from interfering with each other,
// e.g. a destroy function in one component should never override a ref set
// by a create function in another component during the same commit.
commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
return;
}
case Profiler: {
Expand Down Expand Up @@ -1325,9 +1355,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case MemoComponent:
case SimpleMemoComponent:
case Chunk: {
// Note: We currently never use MountMutation, but useLayout uses
// UnmountMutation.
commitHookEffectList(UnmountMutation, MountMutation, finishedWork);
// Layout effects are destroyed during the mutation phase so that all
// destroy functions for all fibers are called before any create functions.
// This prevents sibling component effects from interfering with each other,
// e.g. a destroy function in one component should never override a ref set
// by a create function in another component during the same commit.
commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork);
return;
}
case ClassComponent: {
Expand Down
46 changes: 22 additions & 24 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,9 @@ import {
Passive as PassiveEffect,
} from 'shared/ReactSideEffectTags';
import {
NoEffect as NoHookEffect,
UnmountMutation,
MountLayout,
UnmountPassive,
MountPassive,
HasEffect as HookHasEffect,
Layout as HookLayout,
Passive as HookPassive,
} from './ReactHookEffectTags';
import {
scheduleWork,
Expand Down Expand Up @@ -923,7 +921,12 @@ function mountEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
const hook = mountWorkInProgressHook();
const nextDeps = deps === undefined ? null : deps;
currentlyRenderingFiber.effectTag |= fiberEffectTag;
hook.memoizedState = pushEffect(hookEffectTag, create, undefined, nextDeps);
hook.memoizedState = pushEffect(
HookHasEffect | hookEffectTag,
create,
undefined,
nextDeps,
);
}

function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
Expand All @@ -937,15 +940,20 @@ function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
if (nextDeps !== null) {
const prevDeps = prevEffect.deps;
if (areHookInputsEqual(nextDeps, prevDeps)) {
pushEffect(NoHookEffect, create, destroy, nextDeps);
pushEffect(hookEffectTag, create, destroy, nextDeps);
return;
}
}
}

currentlyRenderingFiber.effectTag |= fiberEffectTag;

hook.memoizedState = pushEffect(hookEffectTag, create, destroy, nextDeps);
hook.memoizedState = pushEffect(
HookHasEffect | hookEffectTag,
create,
destroy,
nextDeps,
);
}

function mountEffect(
Expand All @@ -960,7 +968,7 @@ function mountEffect(
}
return mountEffectImpl(
UpdateEffect | PassiveEffect,
UnmountPassive | MountPassive,
HookPassive,
create,
deps,
);
Expand All @@ -978,7 +986,7 @@ function updateEffect(
}
return updateEffectImpl(
UpdateEffect | PassiveEffect,
UnmountPassive | MountPassive,
HookPassive,
create,
deps,
);
Expand All @@ -988,24 +996,14 @@ function mountLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return mountEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
create,
deps,
);
return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
}

function updateLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return updateEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
create,
deps,
);
return updateEffectImpl(UpdateEffect, HookLayout, create, deps);
}

function imperativeHandleEffect<T>(
Expand Down Expand Up @@ -1059,7 +1057,7 @@ function mountImperativeHandle<T>(

return mountEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
HookLayout,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
);
Expand All @@ -1086,7 +1084,7 @@ function updateImperativeHandle<T>(

return updateEffectImpl(
UpdateEffect,
UnmountMutation | MountLayout,
HookLayout,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
);
Expand Down
31 changes: 31 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,21 @@ export function flushPassiveEffects() {
}
}

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

function flushPassiveEffectsImpl() {
if (rootWithPendingPassiveEffects === null) {
return false;
Expand All @@ -2193,6 +2210,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
16 changes: 8 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,11 @@

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 = /* */ 0b000;

// Represents whether effect should fire.
export const HasEffect = /* */ 0b001;

// Represents the phase in which the effect (not the clean-up) fires.
export const Layout = /* */ 0b010;
export const Passive = /* */ 0b100;
Loading

0 comments on commit 7df32c4

Please sign in to comment.