-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
Fix uSES hydration in strict mode #26791
Merged
Merged
Changes from 5 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 a43783e
Test intended behavior
eps1lon aa54e8b
Update test to use new utils
eps1lon 484b290
Fix uSES hydration in strict mode
sophiebits cd4a706
Fix double render when hydrating in concurrent mode
sophiebits 1920123
Split out rerenderSyncExternalStore
sophiebits d17a693
Merge rerenderSES and updateSES again
sophiebits File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1776,8 +1776,6 @@ function mountSyncExternalStore<T>( | |
// clean-up function, and we track the deps correctly, we can call pushEffect | ||
// directly, without storing any additional state. For the same reason, we | ||
// don't need to set a static flag, either. | ||
// TODO: We can move this to the passive phase once we add a pre-commit | ||
// consistency check. See the next comment. | ||
fiber.flags |= PassiveEffect; | ||
pushEffect( | ||
HookHasEffect | HookPassive, | ||
|
@@ -1799,15 +1797,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(); | ||
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; | ||
} | ||
} | ||
} | ||
} | ||
|
@@ -1830,7 +1841,7 @@ function updateSyncExternalStore<T>( | |
if ( | ||
inst.getSnapshot !== getSnapshot || | ||
snapshotChanged || | ||
// Check if the susbcribe function changed. We can save some memory by | ||
// Check if the subscribe function changed. We can save some memory by | ||
// checking whether we scheduled a subscription effect above. | ||
(workInProgressHook !== null && | ||
workInProgressHook.memoizedState.tag & HookHasEffect) | ||
|
@@ -1854,7 +1865,7 @@ function updateSyncExternalStore<T>( | |
); | ||
} | ||
|
||
if (!includesBlockingLane(root, renderLanes)) { | ||
if (!isHydrating && !includesBlockingLane(root, renderLanes)) { | ||
pushStoreConsistencyCheck(fiber, getSnapshot, nextSnapshot); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know what this is so @acdlite is probably best suited for review. Though this |
||
} | ||
} | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.