From 20b0c0139f0569c71d52815bf6efd52a8a6e7027 Mon Sep 17 00:00:00 2001 From: Tianyu Yao Date: Fri, 9 Jun 2023 13:50:22 -0400 Subject: [PATCH 1/3] Add an option in StrictMode to disable double useEffect in legacy strict mode --- packages/react-reconciler/src/ReactFiber.js | 8 +++++++ .../react-reconciler/src/ReactFiberHooks.js | 4 +++- .../react-reconciler/src/ReactTypeOfMode.js | 15 ++++++------ .../ReactOffscreenStrictMode-test.js | 24 +++++++++++++++++++ .../ReactStrictMode-test.internal.js | 21 ++++++++++++++++ 5 files changed, 64 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 84ef41f4fb9..89497d9c970 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -87,6 +87,7 @@ import { StrictLegacyMode, StrictEffectsMode, ConcurrentUpdatesByDefaultMode, + NoStrictPassiveEffectsMode, } from './ReactTypeOfMode'; import { REACT_FORWARD_REF_TYPE, @@ -539,6 +540,9 @@ export function createFiberFromTypeAndProps( if ((mode & ConcurrentMode) !== NoMode) { // Strict effects should never run on legacy roots mode |= StrictEffectsMode; + if (pendingProps.unstable_disableStrictPassiveEffect) { + mode |= NoStrictPassiveEffectsMode; + } } break; case REACT_PROFILER_TYPE: @@ -752,6 +756,10 @@ export function createFiberFromOffscreen( lanes: Lanes, key: null | string, ): Fiber { + if (__DEV__) { + // StrictMode in Offscreen should always run double passive effects + mode &= ~NoStrictPassiveEffectsMode; + } const fiber = createFiber(OffscreenComponent, pendingProps, key, mode); fiber.elementType = REACT_OFFSCREEN_TYPE; fiber.lanes = lanes; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 19ad56d0f8c..f40380d8cdb 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -58,6 +58,7 @@ import { DebugTracingMode, StrictEffectsMode, StrictLegacyMode, + NoStrictPassiveEffectsMode, } from './ReactTypeOfMode'; import { NoLane, @@ -2257,7 +2258,8 @@ function mountEffect( ): void { if ( __DEV__ && - (currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode + (currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode && + (currentlyRenderingFiber.mode & NoStrictPassiveEffectsMode) === NoMode ) { mountEffectImpl( MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect, diff --git a/packages/react-reconciler/src/ReactTypeOfMode.js b/packages/react-reconciler/src/ReactTypeOfMode.js index 07e08cff210..6da450ffeac 100644 --- a/packages/react-reconciler/src/ReactTypeOfMode.js +++ b/packages/react-reconciler/src/ReactTypeOfMode.js @@ -9,11 +9,12 @@ export type TypeOfMode = number; -export const NoMode = /* */ 0b000000; +export const NoMode = /* */ 0b0000000; // TODO: Remove ConcurrentMode by reading from the root tag instead -export const ConcurrentMode = /* */ 0b000001; -export const ProfileMode = /* */ 0b000010; -export const DebugTracingMode = /* */ 0b000100; -export const StrictLegacyMode = /* */ 0b001000; -export const StrictEffectsMode = /* */ 0b010000; -export const ConcurrentUpdatesByDefaultMode = /* */ 0b100000; +export const ConcurrentMode = /* */ 0b0000001; +export const ProfileMode = /* */ 0b0000010; +export const DebugTracingMode = /* */ 0b0000100; +export const StrictLegacyMode = /* */ 0b0001000; +export const StrictEffectsMode = /* */ 0b0010000; +export const ConcurrentUpdatesByDefaultMode = /* */ 0b0100000; +export const NoStrictPassiveEffectsMode = /* */ 0b1000000; diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js index 129d4d0ddc8..17613295a66 100644 --- a/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js +++ b/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js @@ -55,6 +55,30 @@ describe('ReactOffscreenStrictMode', () => { ]); }); + // @gate __DEV__ && enableOffscreen + it('should trigger strict effects when disableStrictPassiveEffect is presented on StrictMode', async () => { + await act(() => { + ReactNoop.render( + + + + + , + ); + }); + + expect(log).toEqual([ + 'A: render', + 'A: render', + 'A: useLayoutEffect mount', + 'A: useEffect mount', + 'A: useLayoutEffect unmount', + 'A: useEffect unmount', + 'A: useLayoutEffect mount', + 'A: useEffect mount', + ]); + }); + // @gate __DEV__ && enableOffscreen && useModernStrictMode it('should not trigger strict effects when offscreen is hidden', async () => { await act(() => { diff --git a/packages/react/src/__tests__/ReactStrictMode-test.internal.js b/packages/react/src/__tests__/ReactStrictMode-test.internal.js index 48efe5e7d5f..ef5cfa246ad 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.internal.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.internal.js @@ -104,6 +104,27 @@ describe('ReactStrictMode', () => { ]); }); + it('should include legacy + strict effects mode, but not strict passive effect with disableStrictPassiveEffect', async () => { + await act(() => { + const container = document.createElement('div'); + const root = ReactDOMClient.createRoot(container); + root.render( + + + , + ); + }); + + expect(log).toEqual([ + 'A: render', + 'A: render', + 'A: useLayoutEffect mount', + 'A: useEffect mount', + 'A: useLayoutEffect unmount', + 'A: useLayoutEffect mount', + ]); + }); + it('should allow level to be increased with nesting', async () => { await act(() => { const container = document.createElement('div'); From 10ce45a311e2b0b716480174c9b6c499fd20e972 Mon Sep 17 00:00:00 2001 From: Tianyu Yao Date: Tue, 20 Jun 2023 16:14:04 -0700 Subject: [PATCH 2/3] Rename to DO_NOT_USE_disableStrictPassiveEffect --- packages/react-reconciler/src/ReactFiber.js | 2 +- .../src/__tests__/ReactOffscreenStrictMode-test.js | 2 +- packages/react/src/__tests__/ReactStrictMode-test.internal.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 89497d9c970..ff98c8bba6b 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -540,7 +540,7 @@ export function createFiberFromTypeAndProps( if ((mode & ConcurrentMode) !== NoMode) { // Strict effects should never run on legacy roots mode |= StrictEffectsMode; - if (pendingProps.unstable_disableStrictPassiveEffect) { + if (pendingProps.DO_NOT_USE_disableStrictPassiveEffect) { mode |= NoStrictPassiveEffectsMode; } } diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js index 17613295a66..f6ef02de522 100644 --- a/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js +++ b/packages/react-reconciler/src/__tests__/ReactOffscreenStrictMode-test.js @@ -59,7 +59,7 @@ describe('ReactOffscreenStrictMode', () => { it('should trigger strict effects when disableStrictPassiveEffect is presented on StrictMode', async () => { await act(() => { ReactNoop.render( - + diff --git a/packages/react/src/__tests__/ReactStrictMode-test.internal.js b/packages/react/src/__tests__/ReactStrictMode-test.internal.js index ef5cfa246ad..379196d9c50 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.internal.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.internal.js @@ -109,7 +109,7 @@ describe('ReactStrictMode', () => { const container = document.createElement('div'); const root = ReactDOMClient.createRoot(container); root.render( - + , ); From 659af98cdc035c159318e022365f01e14cebf66d Mon Sep 17 00:00:00 2001 From: Tianyu Yao Date: Tue, 20 Jun 2023 17:02:58 -0700 Subject: [PATCH 3/3] Add a feature flag to avoid leaking --- packages/react-reconciler/src/ReactFiber.js | 6 +++++- .../react/src/__tests__/ReactStrictMode-test.internal.js | 1 + packages/shared/ReactFeatureFlags.js | 1 + packages/shared/forks/ReactFeatureFlags.native-fb.js | 1 + packages/shared/forks/ReactFeatureFlags.native-oss.js | 1 + packages/shared/forks/ReactFeatureFlags.test-renderer.js | 1 + .../shared/forks/ReactFeatureFlags.test-renderer.native.js | 1 + .../shared/forks/ReactFeatureFlags.test-renderer.www.js | 1 + packages/shared/forks/ReactFeatureFlags.www-dynamic.js | 1 + packages/shared/forks/ReactFeatureFlags.www.js | 1 + 10 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index ff98c8bba6b..15705b59787 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -39,6 +39,7 @@ import { enableDebugTracing, enableFloat, enableHostSingletons, + enableDO_NOT_USE_disableStrictPassiveEffect, } from 'shared/ReactFeatureFlags'; import {NoFlags, Placement, StaticMask} from './ReactFiberFlags'; import {ConcurrentRoot} from './ReactRootTags'; @@ -540,7 +541,10 @@ export function createFiberFromTypeAndProps( if ((mode & ConcurrentMode) !== NoMode) { // Strict effects should never run on legacy roots mode |= StrictEffectsMode; - if (pendingProps.DO_NOT_USE_disableStrictPassiveEffect) { + if ( + enableDO_NOT_USE_disableStrictPassiveEffect && + pendingProps.DO_NOT_USE_disableStrictPassiveEffect + ) { mode |= NoStrictPassiveEffectsMode; } } diff --git a/packages/react/src/__tests__/ReactStrictMode-test.internal.js b/packages/react/src/__tests__/ReactStrictMode-test.internal.js index 379196d9c50..11ed37f5997 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.internal.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.internal.js @@ -104,6 +104,7 @@ describe('ReactStrictMode', () => { ]); }); + // @gate enableDO_NOT_USE_disableStrictPassiveEffect it('should include legacy + strict effects mode, but not strict passive effect with disableStrictPassiveEffect', async () => { await act(() => { const container = document.createElement('div'); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 7cb339c83cc..1236429619a 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -237,3 +237,4 @@ export const consoleManagedByDevToolsDuringStrictMode = true; // components will encounter in production, especially when used With . // TODO: clean up legacy once tests pass WWW. export const useModernStrictMode = false; +export const enableDO_NOT_USE_disableStrictPassiveEffect = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index e96ce86b002..0edbf1eea70 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -82,6 +82,7 @@ export const enableFloat = true; export const enableHostSingletons = true; export const useModernStrictMode = false; +export const enableDO_NOT_USE_disableStrictPassiveEffect = false; export const enableFizzExternalRuntime = false; export const diffInCommitPhase = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 97556a05514..1ad75cefa49 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -68,6 +68,7 @@ export const enableFloat = true; export const enableHostSingletons = true; export const useModernStrictMode = false; +export const enableDO_NOT_USE_disableStrictPassiveEffect = false; export const enableFizzExternalRuntime = false; export const enableDeferRootSchedulingToMicrotask = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 6437b4de070..67d35fbdd1e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -68,6 +68,7 @@ export const enableFloat = true; export const enableHostSingletons = true; export const useModernStrictMode = false; +export const enableDO_NOT_USE_disableStrictPassiveEffect = false; export const enableFizzExternalRuntime = false; export const enableDeferRootSchedulingToMicrotask = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 55535ce5e46..0954b32e9af 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -66,6 +66,7 @@ export const enableFloat = true; export const enableHostSingletons = true; export const useModernStrictMode = false; +export const enableDO_NOT_USE_disableStrictPassiveEffect = false; export const enableDeferRootSchedulingToMicrotask = true; export const diffInCommitPhase = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 1df2ff6b9ee..00808854a4d 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -70,6 +70,7 @@ export const enableFloat = true; export const enableHostSingletons = true; export const useModernStrictMode = false; +export const enableDO_NOT_USE_disableStrictPassiveEffect = false; export const enableFizzExternalRuntime = false; export const enableDeferRootSchedulingToMicrotask = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 58cfcdfff42..a1197be4058 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -28,6 +28,7 @@ export const enableDeferRootSchedulingToMicrotask = __VARIANT__; export const diffInCommitPhase = __VARIANT__; export const enableAsyncActions = __VARIANT__; export const alwaysThrottleRetries = __VARIANT__; +export const enableDO_NOT_USE_disableStrictPassiveEffect = __VARIANT__; // Enable this flag to help with concurrent mode debugging. // It logs information to the console about React scheduling, rendering, and commit phases. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 426a9ec8013..36b5236df83 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -30,6 +30,7 @@ export const { diffInCommitPhase, enableAsyncActions, alwaysThrottleRetries, + enableDO_NOT_USE_disableStrictPassiveEffect, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build.