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

Fix uSES hydration in strict mode #26791

Merged
merged 7 commits into from
May 12, 2023
Merged

Fix uSES hydration in strict mode #26791

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
00eac85
[Fizz] Add failing test for failed hydration when using uSES in Stric…
eps1lon Feb 6, 2023
a43783e
Test intended behavior
eps1lon Feb 6, 2023
aa54e8b
Update test to use new utils
eps1lon Apr 16, 2023
484b290
Fix uSES hydration in strict mode
sophiebits May 7, 2023
cd4a706
Fix double render when hydrating in concurrent mode
sophiebits May 9, 2023
1920123
Split out rerenderSyncExternalStore
sophiebits May 9, 2023
d17a693
Merge rerenderSES and updateSES again
sophiebits May 12, 2023
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
44 changes: 44 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2507,6 +2507,50 @@ describe('ReactDOMFizzServer', () => {
},
);

it('can hydrate uSES in StrictMode with different client and server snapshot', async () => {
function subscribe() {
return () => {};
}
function getClientSnapshot() {
return 'Yay!';
}
function getServerSnapshot() {
return 'Nay!';
}

function App() {
const value = useSyncExternalStore(
subscribe,
getClientSnapshot,
getServerSnapshot,
);
Scheduler.log(value);

return value;
}

const element = (
<React.StrictMode>
<App />
</React.StrictMode>
);

await act(async () => {
const {pipe} = renderToPipeableStream(element);
pipe(writable);
});

assertLog(['Nay!']);
expect(getVisibleChildren(container)).toEqual('Nay!');

await clientAct(() => {
ReactDOMClient.hydrateRoot(container, element);
});

expect(getVisibleChildren(container)).toEqual('Yay!');
assertLog(['Nay!', 'Yay!']);
});

it(
'errors during hydration force a client render at the nearest Suspense ' +
'boundary, and during the client render it recovers',
Expand Down
31 changes: 22 additions & 9 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1799,15 +1799,28 @@ function updateSyncExternalStore<T>(
// Read the current snapshot from the store on every render. This breaks the
// normal rules of React, and only works because store updates are
// always synchronous.
const nextSnapshot = getSnapshot();
if (__DEV__) {
if (!didWarnUncachedGetSnapshot) {
const cachedSnapshot = getSnapshot();
if (!is(nextSnapshot, cachedSnapshot)) {
console.error(
'The result of getSnapshot should be cached to avoid an infinite loop',
);
didWarnUncachedGetSnapshot = true;
let nextSnapshot;
const isHydrating = getIsHydrating();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

if (isHydrating) {
// Needed for strict mode double render
if (getServerSnapshot === undefined) {
throw new Error(
'Missing getServerSnapshot, which is required for ' +
'server-rendered content. Will revert to client rendering.',
);
}
nextSnapshot = getServerSnapshot();
} else {
nextSnapshot = getSnapshot();
if (__DEV__) {
if (!didWarnUncachedGetSnapshot) {
const cachedSnapshot = getSnapshot();
if (!is(nextSnapshot, cachedSnapshot)) {
console.error(
'The result of getSnapshot should be cached to avoid an infinite loop',
);
didWarnUncachedGetSnapshot = true;
}
}
}
}
Expand Down