From e1f8379f5c256e67db58d3aeaf5153af963adc70 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 29 Jan 2020 11:28:28 -0800 Subject: [PATCH 1/6] 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 | 73 ++++++++---- .../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, 213 insertions(+), 45 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 77b074a17522..1a099060693b 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,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; @@ -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; } @@ -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; } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 02191ffe5d90..abb6cfb104e9 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 a4664613ac22..5eecd0b149e1 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 d54df30cf4e7..2955f9be85e5 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 ca4d3090215e..0f7ee49263cb 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 d4997de38f70..361d8c146709 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 ead32bb435b4..78829b18fdda 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 7ae4ab0e4113..35bd12c3e93d 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -50,6 +50,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = 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 241dad2cf7e3..d26c95f11c10 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -45,6 +45,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = 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 a8bb6bf0aff5..1287361c790b 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -45,6 +45,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = 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 ae94c8f7f5e7..204b44e22a87 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -45,6 +45,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = 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 60448b0097b0..bec0c74ec68a 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -43,6 +43,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = 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 cd3e59e5fe58..3b59401fcdc2 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 From cf44b09c0ca3f30ea345e52077592007e990211d Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 30 Jan 2020 14:49:06 -0800 Subject: [PATCH 2/6] 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. --- packages/react-reconciler/src/ReactFiberWorkLoop.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 5eecd0b149e1..4c067de698ec 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -2183,11 +2183,13 @@ export function enqueuePendingPassiveEffectDestroyFn( ): void { if (deferPassiveEffectCleanupDuringUnmount) { pendingUnmountedPassiveEffectDestroyFunctions.push(destroy); - rootDoesHavePassiveEffects = true; - scheduleCallback(NormalPriority, () => { - flushPassiveEffects(); - return null; - }); + if (!rootDoesHavePassiveEffects) { + rootDoesHavePassiveEffects = true; + scheduleCallback(NormalPriority, () => { + flushPassiveEffects(); + return null; + }); + } } } From 5f3a3b950ed8e5dea8d9ed1f03d948651925bc0d Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 30 Jan 2020 14:53:11 -0800 Subject: [PATCH 3/6] Updated newly added test for added clarity. --- ...eactHooksWithNoopRenderer-test.internal.js | 57 +++++++++---------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 0f7ee49263cb..aa01dbd840b8 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -974,70 +974,69 @@ describe('ReactHooksWithNoopRenderer', () => { ); it('defers passive effect destroy functions during unmount', () => { - function Child({count}) { + function Child({bar, foo}) { React.useEffect(() => { - Scheduler.unstable_yieldValue('passive create (no dependencies)'); + Scheduler.unstable_yieldValue('passive bar create'); return () => { - Scheduler.unstable_yieldValue('passive destroy (no dependencies)'); + Scheduler.unstable_yieldValue('passive bar destroy'); }; - }, []); + }, [bar]); React.useLayoutEffect(() => { - Scheduler.unstable_yieldValue('layout create (no dependencies)'); + Scheduler.unstable_yieldValue('layout bar create'); return () => { - Scheduler.unstable_yieldValue('layout destroy (no dependencies)'); + Scheduler.unstable_yieldValue('layout bar destroy'); }; - }, []); + }, [bar]); React.useEffect(() => { - Scheduler.unstable_yieldValue('passive create'); + Scheduler.unstable_yieldValue('passive foo create'); return () => { - Scheduler.unstable_yieldValue('passive destroy'); + Scheduler.unstable_yieldValue('passive foo destroy'); }; - }, [count]); + }, [foo]); React.useLayoutEffect(() => { - Scheduler.unstable_yieldValue('layout create'); + Scheduler.unstable_yieldValue('layout foo create'); return () => { - Scheduler.unstable_yieldValue('layout destroy'); + Scheduler.unstable_yieldValue('layout foo destroy'); }; - }, [count]); + }, [foo]); Scheduler.unstable_yieldValue('render'); return null; } act(() => { - ReactNoop.render(, () => + ReactNoop.render(, () => Scheduler.unstable_yieldValue('Sync effect'), ); expect(Scheduler).toFlushAndYieldThrough([ 'render', - 'layout create (no dependencies)', - 'layout create', + 'layout bar create', + 'layout foo create', 'Sync effect', ]); // Effects are deferred until after the commit expect(Scheduler).toFlushAndYield([ - 'passive create (no dependencies)', - 'passive create', + 'passive bar create', + 'passive foo create', ]); }); - // HACK - // This update is kind of gross since it exists to test an internal implementation detail: + // This update is 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(, () => + ReactNoop.render(, () => Scheduler.unstable_yieldValue('Sync effect'), ); expect(Scheduler).toFlushAndYieldThrough([ 'render', - 'layout destroy', - 'layout create', + 'layout foo destroy', + 'layout foo create', 'Sync effect', ]); // Effects are deferred until after the commit expect(Scheduler).toFlushAndYield([ - 'passive destroy', - 'passive create', + 'passive foo destroy', + 'passive foo create', ]); }); @@ -1047,14 +1046,14 @@ describe('ReactHooksWithNoopRenderer', () => { Scheduler.unstable_yieldValue('Sync effect'), ); expect(Scheduler).toFlushAndYieldThrough([ - 'layout destroy (no dependencies)', - 'layout destroy', + 'layout bar destroy', + 'layout foo destroy', 'Sync effect', ]); // Effects are deferred until after the commit expect(Scheduler).toFlushAndYield([ - 'passive destroy (no dependencies)', - 'passive destroy', + 'passive bar destroy', + 'passive foo destroy', ]); }); }); From e8c7da10a4d11bad26fa45376e740434f60674ab Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 31 Jan 2020 11:20:08 -0800 Subject: [PATCH 4/6] 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. --- .../src/ReactFiberCommitWork.js | 62 ++++++++++--------- .../react-reconciler/src/ReactFiberHooks.js | 60 +++++++----------- .../src/ReactHookEffectTags.js | 17 +++-- ...eactHooksWithNoopRenderer-test.internal.js | 1 - 4 files changed, 65 insertions(+), 75 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 1a099060693b..ec23dcd995b8 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -114,14 +114,9 @@ import { } from './ReactFiberWorkLoop'; import { NoEffect as NoHookEffect, - NoEffectPassiveUnmountFiber, - 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'; @@ -253,7 +248,6 @@ function commitBeforeMutationLifeCycles( case ForwardRef: case SimpleMemoComponent: case Chunk: { - commitHookEffectList(UnmountSnapshot, NoHookEffect, finishedWork); return; } case ClassComponent: { @@ -342,7 +336,10 @@ function commitHookEffectList( const firstEffect = lastEffect.next; let effect = firstEffect; do { - if ((effect.tag & unmountTag) !== NoHookEffect) { + if ( + (effect.tag & HookHasEffect) !== NoHookEffect && + (effect.tag & unmountTag) !== NoHookEffect + ) { // Unmount const destroy = effect.destroy; effect.destroy = undefined; @@ -350,7 +347,10 @@ function commitHookEffectList( destroy(); } } - if ((effect.tag & mountTag) !== NoHookEffect) { + if ( + (effect.tag & HookHasEffect) !== NoHookEffect && + (effect.tag & mountTag) !== NoHookEffect + ) { // Mount const create = effect.create; effect.destroy = create(); @@ -401,8 +401,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. + commitHookEffectList(HookPassive, NoHookEffect, finishedWork); + commitHookEffectList(NoHookEffect, HookPassive, finishedWork); break; } default: @@ -422,7 +425,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. + commitHookEffectList(NoHookEffect, HookLayout, finishedWork); return; } case ClassComponent: { @@ -764,10 +771,7 @@ function commitUnmount( do { const {destroy, tag} = effect; if (destroy !== undefined) { - if ( - (tag & UnmountPassive) !== NoHookEffect || - (tag & NoEffectPassiveUnmountFiber) !== NoHookEffect - ) { + if ((tag & HookPassive) !== NoHookEffect) { enqueuePendingPassiveEffectDestroyFn(destroy); } else { safelyCallDestroy(current, destroy); @@ -1306,11 +1310,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { case MemoComponent: case SimpleMemoComponent: case Chunk: { - // 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); + // 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. + commitHookEffectList(HookLayout, NoHookEffect, finishedWork); return; } case Profiler: { @@ -1348,11 +1353,12 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { case MemoComponent: case SimpleMemoComponent: case Chunk: { - // 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); + // 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. + commitHookEffectList(HookLayout, NoHookEffect, finishedWork); return; } case ClassComponent: { diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index abb6cfb104e9..fdb0875f8bb5 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -28,12 +28,9 @@ import { Passive as PassiveEffect, } from 'shared/ReactSideEffectTags'; import { - NoEffect as NoHookEffect, - NoEffectPassiveUnmountFiber, - UnmountMutation, - MountLayout, - UnmountPassive, - MountPassive, + HasEffect as HookHasEffect, + Layout as HookLayout, + Passive as HookPassive, } from './ReactHookEffectTags'; import { scheduleWork, @@ -924,16 +921,15 @@ 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, - noWorkUnmountFiberHookEffectTag, - create, - deps, -): void { +function updateEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void { const hook = updateWorkInProgressHook(); const nextDeps = deps === undefined ? null : deps; let destroy = undefined; @@ -944,7 +940,7 @@ function updateEffectImpl( if (nextDeps !== null) { const prevDeps = prevEffect.deps; if (areHookInputsEqual(nextDeps, prevDeps)) { - pushEffect(noWorkUnmountFiberHookEffectTag, create, destroy, nextDeps); + pushEffect(hookEffectTag, create, destroy, nextDeps); return; } } @@ -952,7 +948,12 @@ function updateEffectImpl( currentlyRenderingFiber.effectTag |= fiberEffectTag; - hook.memoizedState = pushEffect(hookEffectTag, create, destroy, nextDeps); + hook.memoizedState = pushEffect( + HookHasEffect | hookEffectTag, + create, + destroy, + nextDeps, + ); } function mountEffect( @@ -967,7 +968,7 @@ function mountEffect( } return mountEffectImpl( UpdateEffect | PassiveEffect, - UnmountPassive | MountPassive, + HookPassive, create, deps, ); @@ -985,8 +986,7 @@ function updateEffect( } return updateEffectImpl( UpdateEffect | PassiveEffect, - UnmountPassive | MountPassive, - NoEffectPassiveUnmountFiber, + HookPassive, create, deps, ); @@ -996,27 +996,14 @@ function mountLayoutEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { - return mountEffectImpl( - UpdateEffect, - UnmountMutation | MountLayout, - create, - deps, - ); + return mountEffectImpl(UpdateEffect, HookLayout, create, deps); } 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, - ); + return updateEffectImpl(UpdateEffect, HookLayout, create, deps); } function imperativeHandleEffect( @@ -1070,7 +1057,7 @@ function mountImperativeHandle( return mountEffectImpl( UpdateEffect, - UnmountMutation | MountLayout, + HookLayout, imperativeHandleEffect.bind(null, create, ref), effectDeps, ); @@ -1097,8 +1084,7 @@ function updateImperativeHandle( return updateEffectImpl( UpdateEffect, - UnmountMutation | MountLayout, - NoHookEffect, + HookLayout, imperativeHandleEffect.bind(null, create, ref), effectDeps, ); diff --git a/packages/react-reconciler/src/ReactHookEffectTags.js b/packages/react-reconciler/src/ReactHookEffectTags.js index 2955f9be85e5..709b5b891e8f 100644 --- a/packages/react-reconciler/src/ReactHookEffectTags.js +++ b/packages/react-reconciler/src/ReactHookEffectTags.js @@ -9,12 +9,11 @@ export type HookEffectTag = number; -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; +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; diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index aa01dbd840b8..c98dbb117127 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1022,7 +1022,6 @@ describe('ReactHooksWithNoopRenderer', () => { // This update is 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'), From d93a76fed76d4de562185ae9e0bbd71749394780 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 3 Feb 2020 10:48:08 -0800 Subject: [PATCH 5/6] Disabled deferred passive effects flushing in OSS builds for now --- packages/shared/ReactFeatureFlags.js | 2 +- packages/shared/forks/ReactFeatureFlags.native-fb.js | 2 +- packages/shared/forks/ReactFeatureFlags.native-oss.js | 2 +- packages/shared/forks/ReactFeatureFlags.persistent.js | 2 +- packages/shared/forks/ReactFeatureFlags.test-renderer.js | 2 +- packages/shared/forks/ReactFeatureFlags.test-renderer.www.js | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 78829b18fdda..4610a042b8ba 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -95,7 +95,7 @@ export const enableNativeTargetAsInstance = false; // 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; +export const deferPassiveEffectCleanupDuringUnmount = false; // -------------------------- // Future APIs to be deprecated diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 35bd12c3e93d..324c229ff993 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -50,7 +50,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; -export const deferPassiveEffectCleanupDuringUnmount = true; +export const deferPassiveEffectCleanupDuringUnmount = false; // 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 d26c95f11c10..8359c153f984 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -45,7 +45,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; -export const deferPassiveEffectCleanupDuringUnmount = true; +export const deferPassiveEffectCleanupDuringUnmount = false; // 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 1287361c790b..c20f6e842a5e 100644 --- a/packages/shared/forks/ReactFeatureFlags.persistent.js +++ b/packages/shared/forks/ReactFeatureFlags.persistent.js @@ -45,7 +45,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; -export const deferPassiveEffectCleanupDuringUnmount = true; +export const deferPassiveEffectCleanupDuringUnmount = false; // 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 204b44e22a87..65ca4e3e78da 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -45,7 +45,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; -export const deferPassiveEffectCleanupDuringUnmount = true; +export const deferPassiveEffectCleanupDuringUnmount = false; // 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 bec0c74ec68a..cf4fbda7c7c2 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -43,7 +43,7 @@ export const disableTextareaChildren = false; export const disableUnstableRenderSubtreeIntoContainer = false; export const warnUnstableRenderSubtreeIntoContainer = false; export const disableUnstableCreatePortal = false; -export const deferPassiveEffectCleanupDuringUnmount = true; +export const deferPassiveEffectCleanupDuringUnmount = false; // Only used in www builds. export function addUserTimingListener() { From 606af56d868f90162baffc78cc66adcb9fe3038c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 3 Feb 2020 11:48:19 -0800 Subject: [PATCH 6/6] Split up unmount and mount effects list traversal --- .../src/ReactFiberCommitWork.js | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index ec23dcd995b8..f4282ac4fa5b 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -325,21 +325,14 @@ 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 & HookHasEffect) !== NoHookEffect && - (effect.tag & unmountTag) !== NoHookEffect - ) { + if ((effect.tag & tag) === tag) { // Unmount const destroy = effect.destroy; effect.destroy = undefined; @@ -347,10 +340,19 @@ function commitHookEffectList( destroy(); } } - if ( - (effect.tag & HookHasEffect) !== NoHookEffect && - (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(); @@ -404,8 +406,8 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void { // 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. - commitHookEffectList(HookPassive, NoHookEffect, finishedWork); - commitHookEffectList(NoHookEffect, HookPassive, finishedWork); + commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork); + commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork); break; } default: @@ -429,7 +431,7 @@ function commitLifeCycles( // 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. - commitHookEffectList(NoHookEffect, HookLayout, finishedWork); + commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); return; } case ClassComponent: { @@ -1315,7 +1317,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { // 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. - commitHookEffectList(HookLayout, NoHookEffect, finishedWork); + commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork); return; } case Profiler: { @@ -1358,7 +1360,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { // 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. - commitHookEffectList(HookLayout, NoHookEffect, finishedWork); + commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork); return; } case ClassComponent: {