Fork updateSyncExternalStore impl in update and rerender #25650
Conversation
|
Comparing: 1e3e30d...168f118 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
| @@ -2798,7 +2836,7 @@ const HooksDispatcherOnRerender: Dispatcher = { | |||
| useDeferredValue: rerenderDeferredValue, | |||
| useTransition: rerenderTransition, | |||
| useMutableSource: updateMutableSource, | |||
| useSyncExternalStore: updateSyncExternalStore, | |||
| useSyncExternalStore: rerenderSyncExternalStore, | |||
There was a problem hiding this comment.
Need to do this same change for the other "on rerender" dispatchers
There was a problem hiding this comment.
I think this is the only place you missed
react/packages/react-reconciler/src/ReactFiberHooks.new.js
Lines 4024 to 4033 in 71e60d0
eps1lon
force-pushed
the
fix/uSES-rerender-impl
branch
from
36a0806 to
68da82a
Compare
| const prevSnapshot = | ||
| currentHook === null | ||
| ? // This is a rerender during a mount. | ||
| hook.memoizedState |
There was a problem hiding this comment.
Hmm I know this is equivalent to what #25650 did, but now that I look closer I think this is wrong for initial render. prevSnapshot should represent what's currently rendered on the screen. If there is no previous snapshot, then we should always schedule a commit effect. That's why mountSyncExternalStore always schedules an effect, without checking first.
So I think if currentHook is null, this should call mountSyncExternalStore instead.
Initial render should always mount an effect.
There was a problem hiding this comment.
Hm wouldn't this also mean we no longer use the client snapshot when we rerender during hydration?
Tried
function rerenderSyncExternalStore<T>(
subscribe: (() => void) => () => void,
getSnapshot: () => T,
getServerSnapshot?: () => T,
): T {
if (currentHook === null) {
return mountSyncExternalStore(subscribe, getSnapshot, getServerSnapshot);
} else {
const hook = updateWorkInProgressHook();
// This is a rerender during an update.
return updateSyncExternalStoreImpl(
hook,
hook.memoizedState,
subscribe,
getSnapshot,
getServerSnapshot,
);
}
}and this leads to
FAIL packages/use-sync-external-store/src/__tests__/useSyncExternalStoreShared-test.js
● Shared useSyncExternalStore behavior (shim and built-in) › regression test for #23150
Rendered more hooks than during the previous render.
There was a problem hiding this comment.
Need to call updateWorkInProgressHook and then pass the hook as an argument. The rest of the logic is the same.
There was a problem hiding this comment.
Tried that, still not working. I don't really understand how the pieces fit together here. Seems to me we're conditionally using hooks and the error is expected.
eps1lon
force-pushed
the
fix/uSES-rerender-impl
branch
from
168f118 to
1e3d745
Compare
|
Comparing: b600620...1e3d745 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
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]>
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.
Summary
Follow-up to #25578.
Applied the same pattern as used in
useDeferredValueimplementations.How did you test this change?
yarn test