Fix uSES hydration in strict mode #26791

sophiebits · 2023-05-07T21:11:23Z

Previously, we'd call and use getSnapshot on the second render resulting in Warning: Text content did not match. Server: "Nay!" Client: "Yay!" and then Error: Text content does not match server-rendered HTML..

Fixes #26095. Closes #26113. Closes #25650.

…tMode

Previously, we'd call and use getSnapshot on the second render resulting in `Warning: Text content did not match. Server: "Nay!" Client: "Yay!"` and then `Error: Text content does not match server-rendered HTML.`. Fixes facebook#26095. Closes facebook#26113.

react-sizebot · 2023-05-07T21:15:23Z

Comparing: 16d053d...d17a693

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.min.js	+0.04%	164.19 kB	164.25 kB	=	51.78 kB	51.78 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.04%	171.55 kB	171.61 kB	=	53.98 kB	53.98 kB
facebook-www/ReactDOM-prod.classic.js	+0.06%	570.13 kB	570.46 kB	+0.03%	100.62 kB	100.64 kB
facebook-www/ReactDOM-prod.modern.js	+0.06%	553.87 kB	554.20 kB	+0.03%	97.80 kB	97.83 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against d17a693

sophiebits · 2023-05-07T21:50:31Z

Not sure I understand the test failure.

eps1lon

🥳

eps1lon · 2023-05-08T07:19:31Z

Variant tests produce different results though.

rickhanlonii · 2023-05-08T21:09:19Z

@sophiebits I'm not sure exactly why, but the feature flag causing the failure is enableSyncDefaultUpdates. This is the flag we use to test both the OSS "sync by default" behavior (when true), and the old internal "concurrent by default" behavior (when false). So for some reason, when we're concurrent by default, the Nah logs twice.

rickhanlonii · 2023-05-08T21:36:36Z

It's the same as:

React.startTransition(() => {
  ReactDOMClient.hydrateRoot(container, element);
});

For some reason, when in a transition / concurrent rendering, we render the Nah! value a second time. I confirmed that it's not something broken in strict mode, because I see us actually render Nah 4 times in a transition (only logged to the Scheduler twice because we skip logging in double-render), and only render Nah twice in sync-by-default (only logged once to the Scheduler).

sophiebits · 2023-05-09T01:09:36Z

Hm, it's because of this check:

react/packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines 935 to 941 in 7ac5e9a

    
           if ( 
        
             renderWasConcurrent && 
        
             !isRenderConsistentWithExternalStores(finishedWork) 
        
           ) { 
        
             // A store was mutated in an interleaved event. Render again, 
        
             // synchronously, to block further mutations. 
        
             exitStatus = renderRootSync(root, lanes);

sophiebits · 2023-05-09T01:47:08Z

OK, now I think this might be right.

eps1lon · 2023-05-09T13:50:18Z

packages/react-reconciler/src/ReactFiberHooks.js

@@ -1854,7 +1865,7 @@ function updateSyncExternalStore<T>(
      );
    }

-    if (!includesBlockingLane(root, renderLanes)) {
+    if (!isHydrating && !includesBlockingLane(root, renderLanes)) {
      pushStoreConsistencyCheck(fiber, getSnapshot, nextSnapshot);


I don't know what this is so @acdlite is probably best suited for review. Though this
change makes sense to me since we're also not pushing a consistency check when mounting a sync external store during hydration.

acdlite · 2023-05-09T15:55:41Z

packages/react-reconciler/src/ReactFiberHooks.js

-        );
-        didWarnUncachedGetSnapshot = true;
+  let nextSnapshot;
+  const isHydrating = getIsHydrating();


I think this was a regression when I made it so that the "rerender" dispatcher is used during the strict mode double render. Originally the idea was to keep the getIsHydrating out of the update path because it's always false during an update.

Right now there is no special "rerender" implementation of useSES, but if we add one we can keep this extra check out of the common path.

I am skeptical that the double implementations are good for perf (we should benchmark it sometime) but I can change it.

You mean because of the code size? Execution wise I don't see why it would be slower

Code size and because the function dispatch call is more dynamic so I imagine it can’t inline a jump.

Also, where do we stop? Should we have a fourth implementation for the rerender-on-initial-mount case instead of combining it with the rerender-on-update one like we do today? Right now updateEffect has a branch for currentHook === null.

Oh I was just thinking the rerender implementation would check currentHook and call the mount or update ones as appropriate. I agree it's not worth duplicating the entire update implementation.

Like I did here for rerenderOptimisticHook:

react/packages/react-reconciler/src/ReactFiberHooks.js

Lines 2049 to 2052 in df12d7e

if (currentHook !== null) {

// This is an update. Process the update queue.

return updateOptimistic(passthrough, reducer);

}

The main reason we added the separate phases was to keep the mount path fast, not so much the update one. But subjectively I also prefer it for code organization reasons because the logic does end up being significantly different in a lot of the hooks.

Ultimately though my original comment was only a factoring nit. I'd probably structure this differently if I were doing it myself, but I liked how you had it before more than this way, so you could just revert it back to that and if I really want to I can submit a follow up PR later.

The important thing is the fix itself, and the regression test you added.

Happy to change it to what you just described! That way makes sense to me. Wasn’t sure if you approved of the fix originally.

Hmm I'm actually not sure the best way to reuse the mount code on rerender because we need it to call updateWorkInProgressHook including for the interior mountEffect. Gonna land it with the unified update+rerender code like I had before but would be happy to review a refactor if you have one in mind.

I think this also fixes a bug where updateStoreInstance would not be pushed on initial mount rerender.

sophiebits

updated

sophiebits · 2023-05-09T20:36:36Z

packages/react-reconciler/src/ReactFiberHooks.js

-        );
-        didWarnUncachedGetSnapshot = true;
+  let nextSnapshot;
+  const isHydrating = getIsHydrating();


Also, where do we stop? Should we have a fourth implementation for the rerender-on-initial-mount case instead of combining it with the rerender-on-update one like we do today? Right now updateEffect has a branch for currentHook === null.

sophiebits · 2023-05-09T20:38:11Z

packages/react-reconciler/src/ReactFiberHooks.js

@@ -3991,7 +4079,11 @@ if (__DEV__) {
    ): T {
      currentHookNameInDev = 'useSyncExternalStore';
      updateHookTypesDev();
-      return updateSyncExternalStore(subscribe, getSnapshot, getServerSnapshot);
+      return rerenderSyncExternalStore(


(also noticed that these should also maybe wrap in InvalidNestedHooksDispatcher although idk why you would try to use a hook in getSnapshot)

This reverts commit 1920123 and does a little cleanup.

Previously, we'd call and use getSnapshot on the second render resulting in `Warning: Text content did not match. Server: "Nay!" Client: "Yay!"` and then `Error: Text content does not match server-rendered HTML.`. Fixes #26095. Closes #26113. Closes #25650. --------- Co-authored-by: eps1lon <[email protected]> DiffTrain build for [4cd7065](4cd7065)

Previously, we'd call and use getSnapshot on the second render resulting in `Warning: Text content did not match. Server: "Nay!" Client: "Yay!"` and then `Error: Text content does not match server-rendered HTML.`. Fixes facebook#26095. Closes facebook#26113. Closes facebook#25650. --------- Co-authored-by: eps1lon <[email protected]>

Previously, we'd call and use getSnapshot on the second render resulting in `Warning: Text content did not match. Server: "Nay!" Client: "Yay!"` and then `Error: Text content does not match server-rendered HTML.`. Fixes #26095. Closes #26113. Closes #25650. --------- Co-authored-by: eps1lon <[email protected]> DiffTrain build for commit 4cd7065.

reccanti · 2024-10-25T18:42:16Z

Has this been included in a release yet? I'm encountering a similar issue but I'm not sure it's related

eps1lon and others added 4 commits May 7, 2023 14:04

[Fizz] Add failing test for failed hydration when using uSES in Stric…

00eac85

…tMode

Test intended behavior

a43783e

Update test to use new utils

aa54e8b

sophiebits requested review from acdlite and eps1lon May 7, 2023 21:11

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels May 7, 2023

eps1lon approved these changes May 8, 2023

View reviewed changes

sophiebits force-pushed the gh-26095 branch from 2e01dd3 to 484b290 Compare May 9, 2023 01:38

Fix double render when hydrating in concurrent mode

cd4a706

eps1lon reviewed May 9, 2023

View reviewed changes

acdlite reviewed May 9, 2023

View reviewed changes

Split out rerenderSyncExternalStore

1920123

I think this also fixes a bug where updateStoreInstance would not be pushed on initial mount rerender.

sophiebits force-pushed the gh-26095 branch from 079b6b0 to 1920123 Compare May 9, 2023 19:36

sophiebits commented May 9, 2023

View reviewed changes

Merge rerenderSES and updateSES again

d17a693

This reverts commit 1920123 and does a little cleanup.

sophiebits force-pushed the gh-26095 branch from ce51243 to d17a693 Compare May 12, 2023 21:09

sophiebits merged commit 4cd7065 into facebook:main May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix uSES hydration in strict mode #26791

Fix uSES hydration in strict mode #26791

sophiebits commented May 7, 2023 •

edited by eps1lon

Loading

react-sizebot commented May 7, 2023 •

edited

Loading

sophiebits commented May 7, 2023

eps1lon left a comment

eps1lon commented May 8, 2023

rickhanlonii commented May 8, 2023

rickhanlonii commented May 8, 2023 •

edited

Loading

sophiebits commented May 9, 2023

sophiebits commented May 9, 2023

eps1lon May 9, 2023

acdlite May 9, 2023

sophiebits May 9, 2023

acdlite May 9, 2023

sophiebits May 9, 2023

sophiebits May 9, 2023

acdlite May 11, 2023

acdlite May 11, 2023

sophiebits May 11, 2023

sophiebits May 12, 2023

sophiebits left a comment

sophiebits May 9, 2023

sophiebits May 9, 2023

reccanti commented Oct 25, 2024

	if (currentHook !== null) {
	// This is an update. Process the update queue.
	return updateOptimistic(passthrough, reducer);
	}

Fix uSES hydration in strict mode #26791

Fix uSES hydration in strict mode #26791

Conversation

sophiebits commented May 7, 2023 • edited by eps1lon Loading

react-sizebot commented May 7, 2023 • edited Loading

Critical size changes

Significant size changes

sophiebits commented May 7, 2023

eps1lon left a comment

Choose a reason for hiding this comment

eps1lon commented May 8, 2023

rickhanlonii commented May 8, 2023

rickhanlonii commented May 8, 2023 • edited Loading

sophiebits commented May 9, 2023

sophiebits commented May 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophiebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reccanti commented Oct 25, 2024

sophiebits commented May 7, 2023 •

edited by eps1lon

Loading

react-sizebot commented May 7, 2023 •

edited

Loading

rickhanlonii commented May 8, 2023 •

edited

Loading