From f8dddae40a3101fa6b0e373f9885644a93ab2995 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Thu, 6 Feb 2025 10:35:24 +0100 Subject: [PATCH 1/2] Double invoke effects in a subtree wrapped with `StrictMode` --- .../src/ReactFiberWorkLoop.js | 26 ++++++--- ...StrictEffectsModeDefaults-test.internal.js | 58 +++++++++++++++++++ 2 files changed, 75 insertions(+), 9 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 5f17ca31b369..1e75bf53f0d0 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -4349,16 +4349,20 @@ function flushRenderPhaseStrictModeWarningsInDEV() { function recursivelyTraverseAndDoubleInvokeEffectsInDEV( root: FiberRoot, parentFiber: Fiber, + treeFlags: Flags, isInStrictMode: boolean, ) { - if ((parentFiber.subtreeFlags & (PlacementDEV | Visibility)) === NoFlags) { + if ( + ((treeFlags | parentFiber.subtreeFlags) & (PlacementDEV | Visibility)) === + NoFlags + ) { // Parent's descendants have already had effects double invoked. // Early exit to avoid unnecessary tree traversal. return; } let child = parentFiber.child; while (child !== null) { - doubleInvokeEffectsInDEVIfNecessary(root, child, isInStrictMode); + doubleInvokeEffectsInDEVIfNecessary(root, child, treeFlags, isInStrictMode); child = child.sibling; } } @@ -4387,6 +4391,7 @@ function doubleInvokeEffectsOnFiber( function doubleInvokeEffectsInDEVIfNecessary( root: FiberRoot, fiber: Fiber, + treeFlags: Flags, parentIsInStrictMode: boolean, ) { const isStrictModeFiber = fiber.type === REACT_STRICT_MODE_TYPE; @@ -4395,7 +4400,7 @@ function doubleInvokeEffectsInDEVIfNecessary( // First case: the fiber **is not** of type OffscreenComponent. No // special rules apply to double invoking effects. if (fiber.tag !== OffscreenComponent) { - if (fiber.flags & PlacementDEV) { + if ((treeFlags | fiber.flags) & PlacementDEV) { if (isInStrictMode) { runWithFiberInDEV( fiber, @@ -4404,14 +4409,15 @@ function doubleInvokeEffectsInDEVIfNecessary( fiber, (fiber.mode & NoStrictPassiveEffectsMode) === NoMode, ); + return; } - } else { - recursivelyTraverseAndDoubleInvokeEffectsInDEV( - root, - fiber, - isInStrictMode, - ); } + recursivelyTraverseAndDoubleInvokeEffectsInDEV( + root, + fiber, + treeFlags | fiber.flags, + isInStrictMode, + ); return; } @@ -4432,6 +4438,7 @@ function doubleInvokeEffectsInDEVIfNecessary( recursivelyTraverseAndDoubleInvokeEffectsInDEV, root, fiber, + treeFlags | fiber.flags, isInStrictMode, ); } @@ -4455,6 +4462,7 @@ function commitDoubleInvokeEffectsInDEV( recursivelyTraverseAndDoubleInvokeEffectsInDEV( root, root.current, + root.current.flags, doubleInvokeEffects, ); } else { diff --git a/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js b/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js index 5829cd090426..fba0891af750 100644 --- a/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js +++ b/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js @@ -240,6 +240,64 @@ describe('StrictEffectsMode defaults', () => { }); }); + it('should double invoke effects in a mounted strict subtree', async () => { + const log = []; + function ComponentWithEffects({label}) { + React.useEffect(() => { + log.push(`useEffect mount "${label}"`); + Scheduler.log(`useEffect mount "${label}"`); + return () => { + log.push(`useEffect unmount "${label}"`); + Scheduler.log(`useEffect unmount "${label}"`); + }; + }); + + React.useLayoutEffect(() => { + log.push(`useLayoutEffect mount "${label}"`); + Scheduler.log(`useLayoutEffect mount "${label}"`); + return () => { + log.push(`useLayoutEffect unmount "${label}"`); + Scheduler.log(`useLayoutEffect unmount "${label}"`); + }; + }); + + return label; + } + + // this component intentionally introduces a component layer between the root and the StrictMode + function RenderChildren({children}) { + return children; + } + + await act(async () => { + ReactNoop.render( + + + + + + , + ); + + await waitForAll([ + 'useLayoutEffect mount "one"', + 'useLayoutEffect mount "two"', + 'useEffect mount "one"', + 'useEffect mount "two"', + ]); + expect(log).toEqual([ + 'useLayoutEffect mount "one"', + 'useLayoutEffect mount "two"', + 'useEffect mount "one"', + 'useEffect mount "two"', + 'useLayoutEffect unmount "two"', + 'useEffect unmount "two"', + 'useLayoutEffect mount "two"', + 'useEffect mount "two"', + ]); + }); + }); + it('double invoking for effects for modern roots', async () => { const log = []; function App({text}) { From 5af49d50ed21dfd7921e995de8616f8f73d9922b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Thu, 6 Feb 2025 11:43:44 +0100 Subject: [PATCH 2/2] enable the existing test case instead --- ...StrictEffectsModeDefaults-test.internal.js | 58 ------------------- .../ReactStrictMode-test.internal.js | 9 ++- 2 files changed, 4 insertions(+), 63 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js b/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js index fba0891af750..5829cd090426 100644 --- a/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js +++ b/packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js @@ -240,64 +240,6 @@ describe('StrictEffectsMode defaults', () => { }); }); - it('should double invoke effects in a mounted strict subtree', async () => { - const log = []; - function ComponentWithEffects({label}) { - React.useEffect(() => { - log.push(`useEffect mount "${label}"`); - Scheduler.log(`useEffect mount "${label}"`); - return () => { - log.push(`useEffect unmount "${label}"`); - Scheduler.log(`useEffect unmount "${label}"`); - }; - }); - - React.useLayoutEffect(() => { - log.push(`useLayoutEffect mount "${label}"`); - Scheduler.log(`useLayoutEffect mount "${label}"`); - return () => { - log.push(`useLayoutEffect unmount "${label}"`); - Scheduler.log(`useLayoutEffect unmount "${label}"`); - }; - }); - - return label; - } - - // this component intentionally introduces a component layer between the root and the StrictMode - function RenderChildren({children}) { - return children; - } - - await act(async () => { - ReactNoop.render( - - - - - - , - ); - - await waitForAll([ - 'useLayoutEffect mount "one"', - 'useLayoutEffect mount "two"', - 'useEffect mount "one"', - 'useEffect mount "two"', - ]); - expect(log).toEqual([ - 'useLayoutEffect mount "one"', - 'useLayoutEffect mount "two"', - 'useEffect mount "one"', - 'useEffect mount "two"', - 'useLayoutEffect unmount "two"', - 'useEffect unmount "two"', - 'useLayoutEffect mount "two"', - 'useEffect mount "two"', - ]); - }); - }); - it('double invoking for effects for modern roots', async () => { const log = []; function App({text}) { diff --git a/packages/react/src/__tests__/ReactStrictMode-test.internal.js b/packages/react/src/__tests__/ReactStrictMode-test.internal.js index 2867e0648379..d3061bc3c3fd 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.internal.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.internal.js @@ -206,11 +206,10 @@ describe('ReactStrictMode', () => { 'B: useLayoutEffect mount', 'A: useEffect mount', 'B: useEffect mount', - // TODO: this is currently broken - // 'B: useLayoutEffect unmount', - // 'B: useEffect unmount', - // 'B: useLayoutEffect mount', - // 'B: useEffect mount', + 'B: useLayoutEffect unmount', + 'B: useEffect unmount', + 'B: useLayoutEffect mount', + 'B: useEffect mount', ]); }); }