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'),