From 1c6be34b86af348bec52396374f68b5a10bf7eaf Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 31 Jan 2020 11:20:08 -0800 Subject: [PATCH] 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'),