diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.new.js b/packages/react-reconciler/src/ReactFiberClassComponent.new.js index 4688c10998d17..663b27dfed419 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.new.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.new.js @@ -30,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, @@ -891,7 +897,12 @@ function mountClassInstance( } if (typeof instance.componentDidMount === 'function') { - if (__DEV__ && enableDoubleInvokingEffects) { + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { + // Never double-invoke effects for legacy roots. workInProgress.flags |= MountLayoutDev | Update; } else { workInProgress.flags |= Update; @@ -965,7 +976,11 @@ 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') { - if (__DEV__ && enableDoubleInvokingEffects) { + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { workInProgress.flags |= MountLayoutDev | Update; } else { workInProgress.flags |= Update; @@ -1012,7 +1027,11 @@ function resumeMountClassInstance( } } if (typeof instance.componentDidMount === 'function') { - if (__DEV__ && enableDoubleInvokingEffects) { + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { workInProgress.flags |= MountLayoutDev | Update; } else { workInProgress.flags |= Update; @@ -1022,7 +1041,11 @@ 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') { - if (__DEV__ && enableDoubleInvokingEffects) { + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { workInProgress.flags |= MountLayoutDev | Update; } else { workInProgress.flags |= Update; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 4e9acfae11356..c43c6f8b1363f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2024,6 +2024,8 @@ function commitPassiveMount( 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: @@ -2057,6 +2059,8 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { 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: @@ -2081,6 +2085,8 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void { 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: @@ -2113,6 +2119,8 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { 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: diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 67c148ddc0171..bdd0bed4c759d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -29,7 +29,12 @@ import { enableDoubleInvokingEffects, } from 'shared/ReactFeatureFlags'; -import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode'; +import { + NoMode, + BlockingMode, + ConcurrentMode, + DebugTracingMode, +} from './ReactTypeOfMode'; import { NoLane, NoLanes, @@ -485,7 +490,11 @@ export function bailoutHooks( lanes: Lanes, ) { workInProgress.updateQueue = current.updateQueue; - if (__DEV__ && enableDoubleInvokingEffects) { + if ( + __DEV__ && + enableDoubleInvokingEffects && + (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { workInProgress.flags &= ~( MountPassiveDevEffect | PassiveEffect | @@ -1253,7 +1262,11 @@ function mountEffect( } } - if (__DEV__ && enableDoubleInvokingEffects) { + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { return mountEffectImpl( MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, HookPassive, @@ -1287,7 +1300,11 @@ function mountLayoutEffect( create: () => (() => void) | void, deps: Array | void | null, ): void { - if (__DEV__ && enableDoubleInvokingEffects) { + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { return mountEffectImpl( MountLayoutDevEffect | UpdateEffect, HookLayout, @@ -1355,7 +1372,11 @@ function mountImperativeHandle( const effectDeps = deps !== null && deps !== undefined ? deps.concat([ref]) : null; - if (__DEV__ && enableDoubleInvokingEffects) { + if ( + __DEV__ && + enableDoubleInvokingEffects && + (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode + ) { return mountEffectImpl( MountLayoutDevEffect | UpdateEffect, HookLayout, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 4f40bde182310..a07d0f7031ed8 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2874,6 +2874,11 @@ function commitDoubleInvokeEffectsInDEV( 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) { @@ -2898,6 +2903,8 @@ function invokeEffectsInDev( 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) { diff --git a/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js b/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js index fddc3744245a3..e5136af4e2406 100644 --- a/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js @@ -11,17 +11,44 @@ let React; let ReactFeatureFlags; -let ReactNoop; +let ReactTestRenderer; let Scheduler; +let act; describe('ReactDoubleInvokeEvents', () => { beforeEach(() => { jest.resetModules(); React = require('react'); ReactFeatureFlags = require('shared/ReactFeatureFlags'); - ReactNoop = require('react-noop-renderer'); + ReactTestRenderer = require('react-test-renderer'); Scheduler = require('scheduler'); ReactFeatureFlags.enableDoubleInvokingEffects = __VARIANT__; + act = ReactTestRenderer.unstable_concurrentAct; + }); + + it('should not double invoke effects in legacy mode', () => { + function App({text}) { + React.useEffect(() => { + Scheduler.unstable_yieldValue('useEffect mount'); + return () => Scheduler.unstable_yieldValue('useEffect unmount'); + }); + + React.useLayoutEffect(() => { + Scheduler.unstable_yieldValue('useLayoutEffect mount'); + return () => Scheduler.unstable_yieldValue('useLayoutEffect unmount'); + }); + + return text; + } + + act(() => { + ReactTestRenderer.create(); + }); + + expect(Scheduler).toHaveYielded([ + 'useLayoutEffect mount', + 'useEffect mount', + ]); }); it('double invoking for effects works properly', () => { @@ -38,8 +65,12 @@ describe('ReactDoubleInvokeEvents', () => { return text; } - ReactNoop.act(() => { - ReactNoop.render(); + + let renderer; + act(() => { + renderer = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); }); if (__DEV__ && __VARIANT__) { @@ -58,8 +89,8 @@ describe('ReactDoubleInvokeEvents', () => { ]); } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + renderer.update(); }); expect(Scheduler).toHaveYielded([ @@ -69,8 +100,8 @@ describe('ReactDoubleInvokeEvents', () => { 'useEffect mount', ]); - ReactNoop.act(() => { - ReactNoop.render(null); + act(() => { + renderer.unmount(); }); expect(Scheduler).toHaveYielded([ @@ -94,8 +125,11 @@ describe('ReactDoubleInvokeEvents', () => { return text; } - ReactNoop.act(() => { - ReactNoop.render(); + let renderer; + act(() => { + renderer = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); }); if (__DEV__ && __VARIANT__) { @@ -114,8 +148,8 @@ describe('ReactDoubleInvokeEvents', () => { ]); } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + renderer.update(); }); expect(Scheduler).toHaveYielded([ @@ -125,8 +159,8 @@ describe('ReactDoubleInvokeEvents', () => { 'useEffect Two mount', ]); - ReactNoop.act(() => { - ReactNoop.render(null); + act(() => { + renderer.unmount(null); }); expect(Scheduler).toHaveYielded([ @@ -152,8 +186,11 @@ describe('ReactDoubleInvokeEvents', () => { return text; } - ReactNoop.act(() => { - ReactNoop.render(); + let renderer; + act(() => { + renderer = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); }); if (__DEV__ && __VARIANT__) { @@ -172,8 +209,8 @@ describe('ReactDoubleInvokeEvents', () => { ]); } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + renderer.update(); }); expect(Scheduler).toHaveYielded([ @@ -183,8 +220,8 @@ describe('ReactDoubleInvokeEvents', () => { 'useLayoutEffect Two mount', ]); - ReactNoop.act(() => { - ReactNoop.render(null); + act(() => { + renderer.unmount(); }); expect(Scheduler).toHaveYielded([ @@ -206,8 +243,11 @@ describe('ReactDoubleInvokeEvents', () => { return text; } - ReactNoop.act(() => { - ReactNoop.render(); + let renderer; + act(() => { + renderer = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); }); if (__DEV__ && __VARIANT__) { @@ -224,8 +264,8 @@ describe('ReactDoubleInvokeEvents', () => { ]); } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + renderer.update(); }); expect(Scheduler).toHaveYielded([ @@ -233,8 +273,8 @@ describe('ReactDoubleInvokeEvents', () => { 'useEffect mount', ]); - ReactNoop.act(() => { - ReactNoop.render(null); + act(() => { + renderer.unmount(); }); expect(Scheduler).toHaveYielded([]); @@ -264,8 +304,8 @@ describe('ReactDoubleInvokeEvents', () => { } } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + ReactTestRenderer.create(, {unstable_isConcurrent: true}); }); if (__DEV__ && __VARIANT__) { @@ -298,8 +338,11 @@ describe('ReactDoubleInvokeEvents', () => { } } - ReactNoop.act(() => { - ReactNoop.render(); + let renderer; + act(() => { + renderer = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); }); if (__DEV__ && __VARIANT__) { @@ -312,19 +355,45 @@ describe('ReactDoubleInvokeEvents', () => { expect(Scheduler).toHaveYielded(['componentDidMount']); } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + renderer.update(); }); expect(Scheduler).toHaveYielded(['componentDidUpdate']); - ReactNoop.act(() => { - ReactNoop.render(null); + act(() => { + renderer.unmount(); }); expect(Scheduler).toHaveYielded(['componentWillUnmount']); }); + it('should not double invoke class lifecycles in legacy mode', () => { + class App extends React.PureComponent { + componentDidMount() { + Scheduler.unstable_yieldValue('componentDidMount'); + } + + componentDidUpdate() { + Scheduler.unstable_yieldValue('componentDidUpdate'); + } + + componentWillUnmount() { + Scheduler.unstable_yieldValue('componentWillUnmount'); + } + + render() { + return this.props.text; + } + } + + act(() => { + ReactTestRenderer.create(); + }); + + expect(Scheduler).toHaveYielded(['componentDidMount']); + }); + it('double flushing passive effects only results in one double invoke', () => { function App({text}) { const [state, setState] = React.useState(0); @@ -345,8 +414,10 @@ describe('ReactDoubleInvokeEvents', () => { return text; } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); }); if (__DEV__ && __VARIANT__) { @@ -410,8 +481,8 @@ describe('ReactDoubleInvokeEvents', () => { return showChild && ; } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + ReactTestRenderer.create(, {unstable_isConcurrent: true}); }); if (__DEV__ && __VARIANT__) { @@ -430,7 +501,7 @@ describe('ReactDoubleInvokeEvents', () => { ]); } - ReactNoop.act(() => { + act(() => { _setShowChild(true); }); @@ -495,8 +566,11 @@ describe('ReactDoubleInvokeEvents', () => { ); } - ReactNoop.act(() => { - ReactNoop.render(); + let renderer; + act(() => { + renderer = ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); }); if (__DEV__ && __VARIANT__) { @@ -519,8 +593,8 @@ describe('ReactDoubleInvokeEvents', () => { ]); } - ReactNoop.act(() => { - ReactNoop.render(); + act(() => { + renderer.update(); }); expect(Scheduler).toHaveYielded([ @@ -530,8 +604,8 @@ describe('ReactDoubleInvokeEvents', () => { 'useEffect mount', ]); - ReactNoop.act(() => { - ReactNoop.render(null); + act(() => { + renderer.unmount(); }); expect(Scheduler).toHaveYielded([ diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index 191cec9492cec..05d73235b1b38 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -4180,7 +4180,11 @@ describe('Profiler', () => { const interactions = SchedulerTracing.unstable_getCurrent(); expect(interactions.size).toBe(1); interaction = Array.from(interactions)[0]; - ReactTestRenderer.create(); + ReactTestRendererAct(() => { + ReactTestRenderer.create(, { + unstable_isConcurrent: true, + }); + }); }, ); Scheduler.unstable_flushAll();