From bf7b3c34a5c93e0de6e2eff28c697be934a731f9 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jan 2020 11:24:54 -0800 Subject: [PATCH] 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) --- .../src/ReactFiberCommitWork.js | 77 +++++++++---- .../react-reconciler/src/ReactFiberHooks.js | 16 ++- .../src/ReactFiberWorkLoop.js | 29 +++++ .../src/ReactHookEffectTags.js | 17 +-- ...eactHooksWithNoopRenderer-test.internal.js | 106 ++++++++++++++++-- ...tSuspenseWithNoopRenderer-test.internal.js | 5 +- packages/shared/ReactFeatureFlags.js | 6 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.persistent.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 13 files changed, 217 insertions(+), 45 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 77b074a175222..c8a53094b38b1 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -26,6 +26,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing'; import { + deferPassiveEffectCleanupDuringUnmount, enableSchedulerTracing, enableProfilerTimer, enableSuspenseServerRenderer, @@ -109,9 +110,11 @@ import { captureCommitPhaseError, resolveRetryThenable, markCommitTimeOfFallback, + enqueuePendingPassiveEffectDestroyFn, } from './ReactFiberWorkLoop'; import { NoEffect as NoHookEffect, + NoEffectPassiveUnmountFiber, UnmountSnapshot, UnmountMutation, MountMutation, @@ -756,32 +759,54 @@ 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 (deferPassiveEffectCleanupDuringUnmount) { + if ( + (tag & UnmountPassive) !== NoHookEffect || + (tag & NoEffectPassiveUnmountFiber) !== NoHookEffect + ) { + enqueuePendingPassiveEffectDestroyFn(destroy); + } else { + safelyCallDestroy(current, 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; @@ -1285,8 +1310,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; } @@ -1325,8 +1352,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; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 02191ffe5d906..abb6cfb104e91 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -29,6 +29,7 @@ import { } from 'shared/ReactSideEffectTags'; import { NoEffect as NoHookEffect, + NoEffectPassiveUnmountFiber, UnmountMutation, MountLayout, UnmountPassive, @@ -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; @@ -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; } } @@ -979,6 +986,7 @@ function updateEffect( return updateEffectImpl( UpdateEffect | PassiveEffect, UnmountPassive | MountPassive, + NoEffectPassiveUnmountFiber, create, deps, ); @@ -1000,9 +1008,12 @@ function updateLayoutEffect( create: () => (() => void) | void, deps: Array | 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, ); @@ -1087,6 +1098,7 @@ function updateImperativeHandle( return updateEffectImpl( UpdateEffect, UnmountMutation | MountLayout, + NoHookEffect, imperativeHandleEffect.bind(null, create, ref), effectDeps, ); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index a4664613ac224..5eecd0b149e15 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -18,6 +18,7 @@ import type {Hook} from './ReactFiberHooks'; import { warnAboutDeprecatedLifecycles, + deferPassiveEffectCleanupDuringUnmount, enableUserTimingAPI, enableSuspenseServerRenderer, replayFailedUnitOfWorkWithInvokeGuardedCallback, @@ -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, @@ -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; @@ -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. diff --git a/packages/react-reconciler/src/ReactHookEffectTags.js b/packages/react-reconciler/src/ReactHookEffectTags.js index d54df30cf4e73..2955f9be85e50 100644 --- a/packages/react-reconciler/src/ReactHookEffectTags.js +++ b/packages/react-reconciler/src/ReactHookEffectTags.js @@ -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; diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index ca4d3090215eb..0f7ee49263cb4 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -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'); @@ -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(, () => + 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(, () => + 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)'); @@ -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', () => { @@ -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', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index d4997de38f705..361d8c1467090 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -21,6 +21,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; ReactFeatureFlags.flushSuspenseFallbacksInTests = false; + ReactFeatureFlags.deferPassiveEffectCleanupDuringUnmount = true; React = require('react'); Fragment = React.Fragment; ReactNoop = require('react-noop-renderer'); @@ -1617,8 +1618,8 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield([ 'B', 'Destroy Layout Effect [Loading...]', - 'Destroy Effect [Loading...]', 'Layout Effect [B]', + 'Destroy Effect [Loading...]', 'Effect [B]', ]); @@ -1654,9 +1655,9 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield([ 'B2', 'Destroy Layout Effect [Loading...]', - 'Destroy Effect [Loading...]', 'Destroy Layout Effect [B]', 'Layout Effect [B2]', + 'Destroy Effect [Loading...]', 'Destroy Effect [B]', 'Effect [B2]', ]); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 6705b683cf4c3..66f6105dbce8c 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -91,6 +91,12 @@ export const enableTrustedTypesIntegration = false; // Flag to turn event.target and event.currentTarget in ReactNative from a reactTag to a component instance export const enableNativeTargetAsInstance = false; +// Controls behavior of deferred effect destroy functions during unmount. +// Previously these functions were run during commit (along with layout effects). +// Ideally we should delay these until after commit for performance reasons. +// This flag provides a killswitch if that proves to break existing code somehow. +export const deferPassiveEffectCleanupDuringUnmount = true; + // -------------------------- // Future APIs to be deprecated // -------------------------- diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 0fac92224c125..c97963de0946f 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -49,6 +49,7 @@ export const disableCreateFactory = false; export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; +export const deferPassiveEffectCleanupDuringUnmount = true; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 1ab71e94964cb..93624aa4b4cf3 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -44,6 +44,7 @@ export const disableCreateFactory = false; export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; +export const deferPassiveEffectCleanupDuringUnmount = true; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js index 995cb081cdb21..1e7a4aa1c3e98 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -44,6 +44,7 @@ export const disableCreateFactory = false; export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; +export const deferPassiveEffectCleanupDuringUnmount = true; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 4f8251d196b9d..81fd4066c571e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -44,6 +44,7 @@ export const disableCreateFactory = false; export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; +export const deferPassiveEffectCleanupDuringUnmount = true; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 514d532333f8c..260931c0d6d08 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -42,6 +42,7 @@ export const disableCreateFactory = false; export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; +export const deferPassiveEffectCleanupDuringUnmount = true; // Only used in www builds. export function addUserTimingListener() { diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index cf14d167f297c..7402c4a4e18ec 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -15,6 +15,7 @@ export const { debugRenderPhaseSideEffectsForStrictMode, disableInputAttributeSyncing, enableTrustedTypesIntegration, + deferPassiveEffectCleanupDuringUnmount, } = require('ReactFeatureFlags'); // In www, we have experimental support for gathering data