Skip to content

Failing test for double invoking Effects during hydration#28951

Closed
eps1lon wants to merge 1 commit intofacebook:mainfrom
eps1lon:strict-effects
Closed

Failing test for double invoking Effects during hydration#28951
eps1lon wants to merge 1 commit intofacebook:mainfrom
eps1lon:strict-effects

Conversation

@eps1lon
Copy link
Copy Markdown
Collaborator

@eps1lon eps1lon commented Apr 29, 2024

Working in render but broken in hydrateRoot.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 29, 2024
@react-sizebot
Copy link
Copy Markdown

react-sizebot commented Apr 29, 2024

Comparing: 5f06c3d...314aff8

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 = 530.18 kB 530.18 kB = 93.39 kB 93.39 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 = 655.81 kB 655.81 kB = 115.30 kB 115.30 kB
facebook-www/ReactDOM-prod.classic.js = 675.58 kB 675.58 kB = 118.54 kB 118.54 kB
facebook-www/ReactDOM-prod.modern.js = 666.00 kB 666.00 kB = 116.86 kB 116.87 kB
react-native/shims/ReactNativeViewConfigRegistry.js = 3.69 kB 3.55 kB = 1.32 kB 1.23 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
react-native/shims/ReactNativeViewConfigRegistry.js = 3.69 kB 3.55 kB = 1.32 kB 1.23 kB

Generated by 🚫 dangerJS against 314aff8

@eps1lon eps1lon force-pushed the strict-effects branch 2 times, most recently from 845ebfa to acd5fcc Compare April 29, 2024 16:19
container.innerHTML = ReactDOMServer.renderToString(element);

await act(() => {
ReactDOMClient.hydrateRoot(container, element);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Uses mountChildFibers which has side-effect tracking disabled. Side-effect tracking sets the PlacementDEV flag. Since PlacementDEV is never set here, we never double invoke:

if (fiber.flags & PlacementDEV) {
setCurrentDebugFiberInDEV(fiber);
if (isInStrictMode) {
doubleInvokeEffectsOnFiber(
root,
fiber,
(fiber.mode & NoStrictPassiveEffectsMode) === NoMode,
);
}
resetCurrentDebugFiberInDEV();
} else {

But then it's not clear yet why we need this flag at all. Shouldn't we double invoke regardless of whether something was placed?

@eps1lon eps1lon changed the title Test StrictEffects in react-dom Failing test for double invoking Effects during hydration Aug 21, 2025
@eps1lon eps1lon marked this pull request as ready for review August 21, 2025 07:42
@eps1lon
Copy link
Copy Markdown
Collaborator Author

eps1lon commented Apr 19, 2026

Landed in #35961

@eps1lon eps1lon closed this Apr 19, 2026
@eps1lon eps1lon deleted the strict-effects branch April 19, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed React Core Team Opened by a member of the React Core Team React 19

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants