Skip to content

Double invoke effects in a subtree wrapped with StrictMode #32315

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.

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

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Feb 6, 2025

Summary

Currently, strict effects don't behave as expected when StrictMode isn't placed at the root of the app. Wrapping only a part of the app is a valid use case described in the docs: https://react.dev/reference/react/StrictMode#enabling-strict-mode-for-a-part-of-the-app

How did you test this change?

I added tests in the repo to verify that it works 😉


I expect some subtleties around this so this might not be a fully correct PR (yet). With some guidance I could improve it. I'm especially not sure if the code paths related to Offscree/Visibility are changed correctly. If you could suggest some extra test cases for them, I'd appreciate it

@react-sizebot
Copy link

react-sizebot commented Feb 6, 2025

Comparing: ff62833...89afbaa

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 515.43 kB 515.43 kB = 92.03 kB 92.03 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 558.46 kB 558.46 kB = 99.25 kB 99.25 kB
facebook-www/ReactDOM-prod.classic.js = 636.83 kB 636.83 kB = 111.91 kB 111.91 kB
facebook-www/ReactDOM-prod.modern.js = 627.15 kB 627.15 kB = 110.33 kB 110.33 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 5af49d5

@@ -4079,16 +4079,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

eps1lon

This comment was marked as outdated.

@Andarist Andarist force-pushed the fix/double-invoke-effects-in-subtree-strict-mode branch from 82ee1ee to 5af49d5 Compare February 6, 2025 10:43
@Andarist

This comment was marked as resolved.

@Andarist

This comment was marked as resolved.

@rickhanlonii
Copy link
Member

Thanks for the PR @Andarist, I looked into this a bit and React 19 has the expected behavior.

It's not safe for <StrictMode> to double invoke on mount unless it is the root component. This is because it will double invoke in the children, and not the parent, but in production that could never happen. If we did this, then it would mean you could write code that works in development (depending on this double firing in the subtree) but breaks in production.

So this is considered a bugfix for React 19. I have a PR to add a note explaining the difference.

In the future, when we land the Activity API, we plan to double invoke effect on mount within <Activity> subtrees of <StrictMode> since this is something that can and will happen in production, similar to how we double invoke for <Suspense> boundaries remounting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants