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.

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

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.

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.

4 participants