Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double invoke effects in a subtree wrapped with StrictMode #32315

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Double invoke effects in a subtree wrapped with StrictMode #32315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8dddae
Double invoke effects in a subtree wrapped with `StrictMode`
Andarist Feb 6, 2025
5af49d5
enable the existing test case instead
Andarist Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4349,16 +4349,20 @@ function flushRenderPhaseStrictModeWarningsInDEV() {
function recursivelyTraverseAndDoubleInvokeEffectsInDEV(
root: FiberRoot,
parentFiber: Fiber,
treeFlags: Flags,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The problem is that in this scenario only the root gets PlacementDev flag because reconcileChildren routes to mountChildFibers for all of the descendant's fibers.
  2. That in turn is a reconciler function created with shouldTrackSideEffects === false
  3. So .subtreeFlags can't be relied upon because, well, those flags are not set on those descendant fibers so they are also not propagated onto their ancestors' .subtreeFlags

That led me to propagate the treeFlags through the callers' chain here

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;
}
}
Expand Down Expand Up @@ -4387,6 +4391,7 @@ function doubleInvokeEffectsOnFiber(
function doubleInvokeEffectsInDEVIfNecessary(
root: FiberRoot,
fiber: Fiber,
treeFlags: Flags,
parentIsInStrictMode: boolean,
) {
const isStrictModeFiber = fiber.type === REACT_STRICT_MODE_TYPE;
Expand All @@ -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,
Expand All @@ -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;
}

Expand All @@ -4432,6 +4438,7 @@ function doubleInvokeEffectsInDEVIfNecessary(
recursivelyTraverseAndDoubleInvokeEffectsInDEV,
root,
fiber,
treeFlags | fiber.flags,
isInStrictMode,
);
}
Expand All @@ -4455,6 +4462,7 @@ function commitDoubleInvokeEffectsInDEV(
recursivelyTraverseAndDoubleInvokeEffectsInDEV(
root,
root.current,
root.current.flags,
doubleInvokeEffects,
);
} else {
Expand Down
9 changes: 4 additions & 5 deletions packages/react/src/__tests__/ReactStrictMode-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
]);
});
}
Expand Down
Loading