From 3957853ae5095a85bacf6d414ee200e8236e9ed4 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 22 Jan 2021 05:58:11 -0800 Subject: [PATCH] Re-add "strict effects mode" for legacy roots only (#20639) This combines changes originally made in #19523, #20028, and #20415 but with slightly different semantics: "strict effects" mode is enabled only for the experimental root APIs (never for legacy render, regardless of usage). These semantics may change slightly in the future. --- .../src/ReactFiberClassComponent.new.js | 55 ++++++- .../src/ReactFiberClassComponent.old.js | 55 ++++++- .../src/ReactFiberCommitWork.new.js | 134 ++++++++++++++++++ .../src/ReactFiberCommitWork.old.js | 134 ++++++++++++++++++ .../src/ReactFiberHooks.new.js | 96 ++++++++++--- .../src/ReactFiberHooks.old.js | 102 ++++++++++--- .../src/ReactFiberWorkLoop.new.js | 72 ++++++++++ .../src/ReactFiberWorkLoop.old.js | 70 +++++++++ .../ReactDoubleInvokeEvents-test.internal.js | 89 +++++------- .../__tests__/ReactProfiler-test.internal.js | 2 - 10 files changed, 707 insertions(+), 102 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.new.js b/packages/react-reconciler/src/ReactFiberClassComponent.new.js index e807ae8222a2e..83004c3f2063a 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.new.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.new.js @@ -12,13 +12,14 @@ import type {Lanes} from './ReactFiberLane.new'; import type {UpdateQueue} from './ReactUpdateQueue.new'; import * as React from 'react'; -import {Update, Snapshot} from './ReactFiberFlags'; +import {MountLayoutDev, Update, Snapshot} from './ReactFiberFlags'; import { debugRenderPhaseSideEffectsForStrictMode, disableLegacyContext, enableDebugTracing, enableSchedulingProfiler, warnAboutDeprecatedLifecycles, + enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; import ReactStrictModeWarnings from './ReactStrictModeWarnings.new'; import {isMounted} from './ReactFiberTreeReflection'; @@ -29,7 +30,13 @@ import invariant from 'shared/invariant'; import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols'; import {resolveDefaultProps} from './ReactFiberLazyComponent.new'; -import {DebugTracingMode, StrictMode} from './ReactTypeOfMode'; +import { + BlockingMode, + ConcurrentMode, + DebugTracingMode, + NoMode, + StrictMode, +} from './ReactTypeOfMode'; import { enqueueUpdate, @@ -890,7 +897,16 @@ function mountClassInstance( } if (typeof instance.componentDidMount === 'function') { - workInProgress.flags |= Update; + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. + workInProgress.flags |= MountLayoutDev | Update; + } else { + workInProgress.flags |= Update; + } } } @@ -960,7 +976,16 @@ function resumeMountClassInstance( // If an update was already in progress, we should schedule an Update // effect even though we're bailing out, so that cWU/cDU are called. if (typeof instance.componentDidMount === 'function') { - workInProgress.flags |= Update; + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. + workInProgress.flags |= MountLayoutDev | Update; + } else { + workInProgress.flags |= Update; + } } return false; } @@ -1003,13 +1028,31 @@ function resumeMountClassInstance( } } if (typeof instance.componentDidMount === 'function') { - workInProgress.flags |= Update; + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. + workInProgress.flags |= MountLayoutDev | Update; + } else { + workInProgress.flags |= Update; + } } } else { // If an update was already in progress, we should schedule an Update // effect even though we're bailing out, so that cWU/cDU are called. if (typeof instance.componentDidMount === 'function') { - workInProgress.flags |= Update; + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. + workInProgress.flags |= MountLayoutDev | Update; + } else { + workInProgress.flags |= Update; + } } // If shouldComponentUpdate returned false, we should still update the diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.old.js b/packages/react-reconciler/src/ReactFiberClassComponent.old.js index f055bac71ca94..2b4496ca5c910 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.old.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.old.js @@ -12,13 +12,14 @@ import type {Lanes} from './ReactFiberLane.old'; import type {UpdateQueue} from './ReactUpdateQueue.old'; import * as React from 'react'; -import {Update, Snapshot} from './ReactFiberFlags'; +import {MountLayoutDev, Update, Snapshot} from './ReactFiberFlags'; import { debugRenderPhaseSideEffectsForStrictMode, disableLegacyContext, enableDebugTracing, enableSchedulingProfiler, warnAboutDeprecatedLifecycles, + enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; import ReactStrictModeWarnings from './ReactStrictModeWarnings.old'; import {isMounted} from './ReactFiberTreeReflection'; @@ -29,7 +30,13 @@ import invariant from 'shared/invariant'; import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols'; import {resolveDefaultProps} from './ReactFiberLazyComponent.old'; -import {DebugTracingMode, StrictMode} from './ReactTypeOfMode'; +import { + BlockingMode, + ConcurrentMode, + DebugTracingMode, + NoMode, + StrictMode, +} from './ReactTypeOfMode'; import { enqueueUpdate, @@ -890,7 +897,16 @@ function mountClassInstance( } if (typeof instance.componentDidMount === 'function') { - workInProgress.flags |= Update; + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. + workInProgress.flags |= MountLayoutDev | Update; + } else { + workInProgress.flags |= Update; + } } } @@ -960,7 +976,16 @@ function resumeMountClassInstance( // If an update was already in progress, we should schedule an Update // effect even though we're bailing out, so that cWU/cDU are called. if (typeof instance.componentDidMount === 'function') { - workInProgress.flags |= Update; + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. + workInProgress.flags |= MountLayoutDev | Update; + } else { + workInProgress.flags |= Update; + } } return false; } @@ -1003,13 +1028,31 @@ function resumeMountClassInstance( } } if (typeof instance.componentDidMount === 'function') { - workInProgress.flags |= Update; + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. + workInProgress.flags |= MountLayoutDev | Update; + } else { + workInProgress.flags |= Update; + } } } else { // If an update was already in progress, we should schedule an Update // effect even though we're bailing out, so that cWU/cDU are called. if (typeof instance.componentDidMount === 'function') { - workInProgress.flags |= Update; + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. + workInProgress.flags |= MountLayoutDev | Update; + } else { + workInProgress.flags |= Update; + } } // If shouldComponentUpdate returned false, we should still update the diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 371991abda353..2d50f255ee21f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -36,6 +36,7 @@ import { enableFundamentalAPI, enableSuspenseCallback, enableScopeAPI, + enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -2436,6 +2437,135 @@ function ensureCorrectReturnPointer(fiber, expectedReturnFiber) { fiber.return = expectedReturnFiber; } +function invokeLayoutEffectMountInDEV(fiber: Fiber): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + invokeGuardedCallback( + null, + commitHookEffectListMount, + null, + HookLayout | HookHasEffect, + fiber, + ); + if (hasCaughtError()) { + const mountError = clearCaughtError(); + captureCommitPhaseError(fiber, mountError); + } + break; + } + case ClassComponent: { + const instance = fiber.stateNode; + invokeGuardedCallback(null, instance.componentDidMount, instance); + if (hasCaughtError()) { + const mountError = clearCaughtError(); + captureCommitPhaseError(fiber, mountError); + } + break; + } + } + } +} + +function invokePassiveEffectMountInDEV(fiber: Fiber): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + invokeGuardedCallback( + null, + commitHookEffectListMount, + null, + HookPassive | HookHasEffect, + fiber, + ); + if (hasCaughtError()) { + const mountError = clearCaughtError(); + captureCommitPhaseError(fiber, mountError); + } + break; + } + } + } +} + +function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + invokeGuardedCallback( + null, + commitHookEffectListUnmount, + null, + HookLayout | HookHasEffect, + fiber, + fiber.return, + ); + if (hasCaughtError()) { + const unmountError = clearCaughtError(); + captureCommitPhaseError(fiber, unmountError); + } + break; + } + case ClassComponent: { + const instance = fiber.stateNode; + if (typeof instance.componentWillUnmount === 'function') { + invokeGuardedCallback( + null, + safelyCallComponentWillUnmount, + null, + fiber, + instance, + fiber.return, + ); + if (hasCaughtError()) { + const unmountError = clearCaughtError(); + captureCommitPhaseError(fiber, unmountError); + } + } + break; + } + } + } +} + +function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + invokeGuardedCallback( + null, + commitHookEffectListUnmount, + null, + HookPassive | HookHasEffect, + fiber, + fiber.return, + ); + if (hasCaughtError()) { + const unmountError = clearCaughtError(); + captureCommitPhaseError(fiber, unmountError); + } + break; + } + } + } +} + export { commitResetTextContent, commitPlacement, @@ -2443,4 +2573,8 @@ export { commitWork, commitAttachRef, commitDetachRef, + invokeLayoutEffectMountInDEV, + invokeLayoutEffectUnmountInDEV, + invokePassiveEffectMountInDEV, + invokePassiveEffectUnmountInDEV, }; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index fdc9868dbe9b5..bc443576e05dc 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -36,6 +36,7 @@ import { enableFundamentalAPI, enableSuspenseCallback, enableScopeAPI, + enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; import { FunctionComponent, @@ -2352,6 +2353,135 @@ function ensureCorrectReturnPointer(fiber, expectedReturnFiber) { fiber.return = expectedReturnFiber; } +function invokeLayoutEffectMountInDEV(fiber: Fiber): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + invokeGuardedCallback( + null, + commitHookEffectListMount, + null, + HookLayout | HookHasEffect, + fiber, + ); + if (hasCaughtError()) { + const mountError = clearCaughtError(); + captureCommitPhaseError(fiber, mountError); + } + break; + } + case ClassComponent: { + const instance = fiber.stateNode; + invokeGuardedCallback(null, instance.componentDidMount, instance); + if (hasCaughtError()) { + const mountError = clearCaughtError(); + captureCommitPhaseError(fiber, mountError); + } + break; + } + } + } +} + +function invokePassiveEffectMountInDEV(fiber: Fiber): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + invokeGuardedCallback( + null, + commitHookEffectListMount, + null, + HookPassive | HookHasEffect, + fiber, + ); + if (hasCaughtError()) { + const mountError = clearCaughtError(); + captureCommitPhaseError(fiber, mountError); + } + break; + } + } + } +} + +function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + invokeGuardedCallback( + null, + commitHookEffectListUnmount, + null, + HookLayout | HookHasEffect, + fiber, + fiber.return, + ); + if (hasCaughtError()) { + const unmountError = clearCaughtError(); + captureCommitPhaseError(fiber, unmountError); + } + break; + } + case ClassComponent: { + const instance = fiber.stateNode; + if (typeof instance.componentWillUnmount === 'function') { + invokeGuardedCallback( + null, + safelyCallComponentWillUnmount, + null, + fiber, + instance, + fiber.return, + ); + if (hasCaughtError()) { + const unmountError = clearCaughtError(); + captureCommitPhaseError(fiber, unmountError); + } + } + break; + } + } + } +} + +function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + switch (fiber.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + invokeGuardedCallback( + null, + commitHookEffectListUnmount, + null, + HookPassive | HookHasEffect, + fiber, + fiber.return, + ); + if (hasCaughtError()) { + const unmountError = clearCaughtError(); + captureCommitPhaseError(fiber, unmountError); + } + break; + } + } + } +} + export { commitBeforeMutationLifeCycles, commitResetTextContent, @@ -2360,4 +2490,8 @@ export { commitWork, commitAttachRef, commitDetachRef, + invokeLayoutEffectMountInDEV, + invokeLayoutEffectUnmountInDEV, + invokePassiveEffectMountInDEV, + invokePassiveEffectUnmountInDEV, }; diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index e21d10b9d706d..ff4b18672e0c3 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -29,9 +29,15 @@ import { enableCache, decoupleUpdatePriorityFromScheduler, enableUseRefAccessWarning, + enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; -import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode'; +import { + NoMode, + BlockingMode, + ConcurrentMode, + DebugTracingMode, +} from './ReactTypeOfMode'; import { NoLane, NoLanes, @@ -52,6 +58,8 @@ import { Update as UpdateEffect, Passive as PassiveEffect, PassiveStatic as PassiveStaticEffect, + MountLayoutDev as MountLayoutDevEffect, + MountPassiveDev as MountPassiveDevEffect, } from './ReactFiberFlags'; import { HasEffect as HookHasEffect, @@ -474,7 +482,20 @@ export function bailoutHooks( workInProgress.updateQueue = current.updateQueue; // TODO: Don't need to reset the flags here, because they're reset in the // complete phase (bubbleProperties). - workInProgress.flags &= ~(PassiveEffect | UpdateEffect); + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + workInProgress.flags &= ~( + MountPassiveDevEffect | + MountLayoutDevEffect | + PassiveEffect | + UpdateEffect + ); + } else { + workInProgress.flags &= ~(PassiveEffect | UpdateEffect); + } current.lanes = removeLanes(current.lanes, lanes); } @@ -1310,12 +1331,25 @@ function mountEffect( warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } } - return mountEffectImpl( - PassiveEffect | PassiveStaticEffect, - HookPassive, - create, - deps, - ); + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + return mountEffectImpl( + MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, + HookPassive, + create, + deps, + ); + } else { + return mountEffectImpl( + PassiveEffect | PassiveStaticEffect, + HookPassive, + create, + deps, + ); + } } function updateEffect( @@ -1335,7 +1369,20 @@ function mountLayoutEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { - return mountEffectImpl(UpdateEffect, HookLayout, create, deps); + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + return mountEffectImpl( + MountLayoutDevEffect | UpdateEffect, + HookLayout, + create, + deps, + ); + } else { + return mountEffectImpl(UpdateEffect, HookLayout, create, deps); + } } function updateLayoutEffect( @@ -1394,12 +1441,25 @@ function mountImperativeHandle( const effectDeps = deps !== null && deps !== undefined ? deps.concat([ref]) : null; - return mountEffectImpl( - UpdateEffect, - HookLayout, - imperativeHandleEffect.bind(null, create, ref), - effectDeps, - ); + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + return mountEffectImpl( + MountLayoutDevEffect | UpdateEffect, + HookLayout, + imperativeHandleEffect.bind(null, create, ref), + effectDeps, + ); + } else { + return mountEffectImpl( + UpdateEffect, + HookLayout, + imperativeHandleEffect.bind(null, create, ref), + effectDeps, + ); + } } function updateImperativeHandle( @@ -1680,7 +1740,11 @@ function mountOpaqueIdentifier(): OpaqueIDType | void { const setId = mountState(id)[1]; if ((currentlyRenderingFiber.mode & BlockingMode) === NoMode) { - currentlyRenderingFiber.flags |= PassiveEffect; + if (__DEV__ && enableDoubleInvokingEffects) { + currentlyRenderingFiber.flags |= MountPassiveDevEffect | PassiveEffect; + } else { + currentlyRenderingFiber.flags |= PassiveEffect; + } pushEffect( HookHasEffect | HookPassive, () => { diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 45ff2b50665cc..f8cd86cc7d730 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -29,9 +29,15 @@ import { enableCache, decoupleUpdatePriorityFromScheduler, enableUseRefAccessWarning, + enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; -import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode'; +import { + NoMode, + BlockingMode, + ConcurrentMode, + DebugTracingMode, +} from './ReactTypeOfMode'; import { NoLane, NoLanes, @@ -52,6 +58,8 @@ import { Update as UpdateEffect, Passive as PassiveEffect, PassiveStatic as PassiveStaticEffect, + MountLayoutDev as MountLayoutDevEffect, + MountPassiveDev as MountPassiveDevEffect, } from './ReactFiberFlags'; import { HasEffect as HookHasEffect, @@ -472,7 +480,22 @@ export function bailoutHooks( lanes: Lanes, ) { workInProgress.updateQueue = current.updateQueue; - workInProgress.flags &= ~(PassiveEffect | UpdateEffect); + // TODO: Don't need to reset the flags here, because they're reset in the + // complete phase (bubbleProperties). + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + workInProgress.flags &= ~( + MountLayoutDevEffect | + MountPassiveDevEffect | + PassiveEffect | + UpdateEffect + ); + } else { + workInProgress.flags &= ~(PassiveEffect | UpdateEffect); + } current.lanes = removeLanes(current.lanes, lanes); } @@ -1308,12 +1331,28 @@ function mountEffect( warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber); } } - return mountEffectImpl( - UpdateEffect | PassiveEffect | PassiveStaticEffect, - HookPassive, - create, - deps, - ); + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + return mountEffectImpl( + MountPassiveDevEffect | + UpdateEffect | + PassiveEffect | + PassiveStaticEffect, + HookPassive, + create, + deps, + ); + } else { + return mountEffectImpl( + UpdateEffect | PassiveEffect | PassiveStaticEffect, + HookPassive, + create, + deps, + ); + } } function updateEffect( @@ -1338,7 +1377,20 @@ function mountLayoutEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { - return mountEffectImpl(UpdateEffect, HookLayout, create, deps); + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + return mountEffectImpl( + MountLayoutDevEffect | UpdateEffect, + HookLayout, + create, + deps, + ); + } else { + return mountEffectImpl(UpdateEffect, HookLayout, create, deps); + } } function updateLayoutEffect( @@ -1397,12 +1449,25 @@ function mountImperativeHandle( const effectDeps = deps !== null && deps !== undefined ? deps.concat([ref]) : null; - return mountEffectImpl( - UpdateEffect, - HookLayout, - imperativeHandleEffect.bind(null, create, ref), - effectDeps, - ); + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + return mountEffectImpl( + MountLayoutDevEffect | UpdateEffect, + HookLayout, + imperativeHandleEffect.bind(null, create, ref), + effectDeps, + ); + } else { + return mountEffectImpl( + UpdateEffect, + HookLayout, + imperativeHandleEffect.bind(null, create, ref), + effectDeps, + ); + } } function updateImperativeHandle( @@ -1683,7 +1748,12 @@ function mountOpaqueIdentifier(): OpaqueIDType | void { const setId = mountState(id)[1]; if ((currentlyRenderingFiber.mode & BlockingMode) === NoMode) { - currentlyRenderingFiber.flags |= UpdateEffect | PassiveEffect; + if (__DEV__ && enableDoubleInvokingEffects) { + currentlyRenderingFiber.flags |= + MountPassiveDevEffect | UpdateEffect | PassiveEffect; + } else { + currentlyRenderingFiber.flags |= UpdateEffect | PassiveEffect; + } pushEffect( HookHasEffect | HookPassive, () => { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index cb4c4dd366673..4ceb164d94ba0 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -15,6 +15,7 @@ import type {Interaction} from 'scheduler/src/Tracing'; import type {SuspenseState} from './ReactFiberSuspenseComponent.new'; import type {StackCursor} from './ReactFiberStack.new'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new'; +import type {Flags} from './ReactFiberFlags'; import { warnAboutDeprecatedLifecycles, @@ -31,6 +32,7 @@ import { enableDebugTracing, enableSchedulingProfiler, disableSchedulerTimeoutInWorkLoop, + enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -126,6 +128,8 @@ import { MutationMask, LayoutMask, PassiveMask, + MountPassiveDev, + MountLayoutDev, } from './ReactFiberFlags'; import { NoLanePriority, @@ -182,6 +186,10 @@ import { commitPassiveEffectDurations, commitPassiveMountEffects, commitPassiveUnmountEffects, + invokeLayoutEffectMountInDEV, + invokePassiveEffectMountInDEV, + invokeLayoutEffectUnmountInDEV, + invokePassiveEffectUnmountInDEV, } from './ReactFiberCommitWork.new'; import {enqueueUpdate} from './ReactUpdateQueue.new'; import {resetContextDependencies} from './ReactFiberNewContext.new'; @@ -2041,6 +2049,12 @@ function commitRootImpl(root, renderPriorityLevel) { legacyErrorBoundariesThatAlreadyFailed = null; } + if (__DEV__ && enableDoubleInvokingEffects) { + if (!rootDidHavePassiveEffects) { + commitDoubleInvokeEffectsInDEV(root.current, false); + } + } + if (enableSchedulerTracing) { if (!rootDidHavePassiveEffects) { // If there are no passive effects, then we can complete the pending interactions. @@ -2222,6 +2236,10 @@ function flushPassiveEffectsImpl() { markPassiveEffectsStopped(); } + if (__DEV__ && enableDoubleInvokingEffects) { + commitDoubleInvokeEffectsInDEV(root.current, true); + } + executionContext = prevExecutionContext; flushSyncCallbackQueue(); @@ -2505,6 +2523,60 @@ function flushRenderPhaseStrictModeWarningsInDEV() { } } +function commitDoubleInvokeEffectsInDEV( + fiber: Fiber, + hasPassiveEffects: boolean, +) { + if (__DEV__ && enableDoubleInvokingEffects) { + // Never double-invoke effects for legacy roots. + if ((fiber.mode & (BlockingMode | ConcurrentMode)) === NoMode) { + return; + } + + setCurrentDebugFiberInDEV(fiber); + invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectUnmountInDEV); + if (hasPassiveEffects) { + invokeEffectsInDev( + fiber, + MountPassiveDev, + invokePassiveEffectUnmountInDEV, + ); + } + + invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectMountInDEV); + if (hasPassiveEffects) { + invokeEffectsInDev(fiber, MountPassiveDev, invokePassiveEffectMountInDEV); + } + resetCurrentDebugFiberInDEV(); + } +} + +// TODO (strict effects) Rewrite to be iterative +function invokeEffectsInDev( + firstChild: Fiber, + fiberFlags: Flags, + invokeEffectFn: (fiber: Fiber) => void, +): void { + if (__DEV__ && enableDoubleInvokingEffects) { + // We don't need to re-check for legacy roots here. + // This function will not be called within legacy roots. + let fiber = firstChild; + while (fiber !== null) { + if (fiber.child !== null) { + const primarySubtreeFlag = fiber.subtreeFlags & fiberFlags; + if (primarySubtreeFlag !== NoFlags) { + invokeEffectsInDev(fiber.child, fiberFlags, invokeEffectFn); + } + } + + if ((fiber.flags & fiberFlags) !== NoFlags) { + invokeEffectFn(fiber); + } + fiber = fiber.sibling; + } + } +} + let didWarnStateUpdateForNotYetMountedComponent: Set | null = null; function warnAboutUpdateOnNotYetMountedFiberInDEV(fiber) { if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 65c966d53110d..f292c7b36c1a6 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -16,6 +16,7 @@ import type {SuspenseState} from './ReactFiberSuspenseComponent.old'; import type {Effect as HookEffect} from './ReactFiberHooks.old'; import type {StackCursor} from './ReactFiberStack.old'; import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.old'; +import type {Flags} from './ReactFiberFlags'; import { warnAboutDeprecatedLifecycles, @@ -32,6 +33,7 @@ import { enableDebugTracing, enableSchedulingProfiler, disableSchedulerTimeoutInWorkLoop, + enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -131,6 +133,8 @@ import { HostEffectMask, Hydrating, StaticMask, + MountPassiveDev, + MountLayoutDev, } from './ReactFiberFlags'; import { NoLanePriority, @@ -189,6 +193,10 @@ import { commitPassiveMountEffects, commitPassiveUnmountEffects, detachFiberAfterEffects, + invokeLayoutEffectMountInDEV, + invokePassiveEffectMountInDEV, + invokeLayoutEffectUnmountInDEV, + invokePassiveEffectUnmountInDEV, } from './ReactFiberCommitWork.old'; import {enqueueUpdate} from './ReactUpdateQueue.old'; import {resetContextDependencies} from './ReactFiberNewContext.old'; @@ -2126,6 +2134,12 @@ function commitRootImpl(root, renderPriorityLevel) { legacyErrorBoundariesThatAlreadyFailed = null; } + if (__DEV__ && enableDoubleInvokingEffects) { + if (!rootDidHavePassiveEffects) { + commitDoubleInvokeEffectsInDEV(root.current, false); + } + } + if (enableSchedulerTracing) { if (!rootDidHavePassiveEffects) { // If there are no passive effects, then we can complete the pending interactions. @@ -2379,6 +2393,10 @@ function flushPassiveEffectsImpl() { markPassiveEffectsStopped(); } + if (__DEV__ && enableDoubleInvokingEffects) { + commitDoubleInvokeEffectsInDEV(root.current, true); + } + executionContext = prevExecutionContext; flushSyncCallbackQueue(); @@ -2662,6 +2680,58 @@ function flushRenderPhaseStrictModeWarningsInDEV() { } } +function commitDoubleInvokeEffectsInDEV( + fiber: Fiber, + hasPassiveEffects: boolean, +) { + if (__DEV__ && enableDoubleInvokingEffects) { + // Never double-invoke effects for legacy roots. + if ((fiber.mode & (BlockingMode | ConcurrentMode)) === NoMode) { + return; + } + + setCurrentDebugFiberInDEV(fiber); + invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectUnmountInDEV); + if (hasPassiveEffects) { + invokeEffectsInDev( + fiber, + MountPassiveDev, + invokePassiveEffectUnmountInDEV, + ); + } + + invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectMountInDEV); + if (hasPassiveEffects) { + invokeEffectsInDev(fiber, MountPassiveDev, invokePassiveEffectMountInDEV); + } + resetCurrentDebugFiberInDEV(); + } +} + +// TODO (strict effects) Rewrite to be iterative +function invokeEffectsInDev( + firstChild: Fiber, + fiberFlags: Flags, + invokeEffectFn: (fiber: Fiber) => void, +): void { + if (__DEV__ && enableDoubleInvokingEffects) { + let fiber = firstChild; + while (fiber !== null) { + if (fiber.child !== null) { + const primarySubtreeFlag = fiber.subtreeFlags & fiberFlags; + if (primarySubtreeFlag !== NoFlags) { + invokeEffectsInDev(fiber.child, fiberFlags, invokeEffectFn); + } + } + + if ((fiber.flags & fiberFlags) !== NoFlags) { + invokeEffectFn(fiber); + } + fiber = fiber.sibling; + } + } +} + let didWarnStateUpdateForNotYetMountedComponent: Set | null = null; function warnAboutUpdateOnNotYetMountedFiberInDEV(fiber) { if (__DEV__) { diff --git a/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js b/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js index 0a7ac0a50d9fd..b53b7b719c6ba 100644 --- a/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js @@ -15,8 +15,9 @@ let ReactNoop; let Scheduler; function shouldDoubleInvokingEffects() { - // Reverted temporarily. Need to re-land. - return false; + // For now, this feature only exists in the old fork (while the new fork is being bisected). + // Eventually we'll land it in both forks. + return __DEV__; } describe('ReactDoubleInvokeEvents', () => { @@ -30,7 +31,7 @@ describe('ReactDoubleInvokeEvents', () => { ReactFeatureFlags.enableDoubleInvokingEffects = shouldDoubleInvokingEffects(); }); - it('should not double invoke effects outside of StrictMode', () => { + it('should not double invoke effects in legacy mode', () => { function App({text}) { React.useEffect(() => { Scheduler.unstable_yieldValue('useEffect mount'); @@ -55,54 +56,30 @@ describe('ReactDoubleInvokeEvents', () => { ]); }); - it('should double invoke effects only within StrictMode subtrees', () => { - function ComponentWithEffects({label}) { - React.useEffect(() => { - Scheduler.unstable_yieldValue(`useEffect mount "${label}"`); - return () => - Scheduler.unstable_yieldValue(`useEffect unmount "${label}"`); - }); + it('should not double invoke class lifecycles in legacy mode', () => { + class App extends React.PureComponent { + componentDidMount() { + Scheduler.unstable_yieldValue('componentDidMount'); + } - React.useLayoutEffect(() => { - Scheduler.unstable_yieldValue(`useLayoutEffect mount "${label}"`); - return () => - Scheduler.unstable_yieldValue(`useLayoutEffect unmount "${label}"`); - }); + componentDidUpdate() { + Scheduler.unstable_yieldValue('componentDidUpdate'); + } - return label; + componentWillUnmount() { + Scheduler.unstable_yieldValue('componentWillUnmount'); + } + + render() { + return this.props.text; + } } ReactNoop.act(() => { - ReactNoop.renderLegacySyncRoot( - <> - - - - - , - ); + ReactNoop.renderLegacySyncRoot(); }); - if (shouldDoubleInvokingEffects()) { - expect(Scheduler).toHaveYielded([ - 'useLayoutEffect mount "loose"', - 'useLayoutEffect mount "strict"', - 'useEffect mount "loose"', - 'useEffect mount "strict"', - - 'useLayoutEffect unmount "strict"', - 'useEffect unmount "strict"', - 'useLayoutEffect mount "strict"', - 'useEffect mount "strict"', - ]); - } else { - expect(Scheduler).toHaveYielded([ - 'useLayoutEffect mount "loose"', - 'useLayoutEffect mount "strict"', - 'useEffect mount "loose"', - 'useEffect mount "strict"', - ]); - } + expect(Scheduler).toHaveYielded(['componentDidMount']); }); it('should flush double-invoked effects within the same frame as layout effects if there are no passive effects', () => { @@ -117,10 +94,10 @@ describe('ReactDoubleInvokeEvents', () => { } ReactNoop.act(() => { - ReactNoop.renderLegacySyncRoot( - + ReactNoop.render( + <> - , + , ); if (shouldDoubleInvokingEffects()) { @@ -137,11 +114,11 @@ describe('ReactDoubleInvokeEvents', () => { }); ReactNoop.act(() => { - ReactNoop.renderLegacySyncRoot( - + ReactNoop.render( + <> - , + , ); if (shouldDoubleInvokingEffects()) { @@ -185,10 +162,10 @@ describe('ReactDoubleInvokeEvents', () => { } ReactNoop.act(() => { - ReactNoop.renderLegacySyncRoot( - + ReactNoop.render( + <> - , + , ); if (shouldDoubleInvokingEffects()) { @@ -211,11 +188,11 @@ describe('ReactDoubleInvokeEvents', () => { }); ReactNoop.act(() => { - ReactNoop.renderLegacySyncRoot( - + ReactNoop.render( + <> - , + , ); if (shouldDoubleInvokingEffects()) { diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 8387319511b55..4a1853c46c28e 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -4874,8 +4874,6 @@ describe('Profiler', () => { }); if (__DEV__) { - // @gate enableUseJSStackToTrackPassiveDurations - // @gate enableDoubleInvokingEffects it('double invoking does not disconnect wrapped async work', () => { ReactFeatureFlags.enableDoubleInvokingEffects = true;