fix(ssr): add getServerSnapshot to useObservable's useSyncExternalStore - #779
Open
tyler-reitz wants to merge 1 commit into
Open
fix(ssr): add getServerSnapshot to useObservable's useSyncExternalStore#779tyler-reitz wants to merge 1 commit into
tyler-reitz wants to merge 1 commit into
Conversation
useObservable called useSyncExternalStore with two arguments. React requires a third, getServerSnapshot, whenever the tree is server rendered or hydrated; without it React throws "Missing getServerSnapshot, which is required for server-rendered content" and the surrounding subtree silently falls back to client rendering. The server snapshot deliberately does not return observable.immutableStatus the way getSnapshot does. preloadedObservables is a globalThis cache keyed only by observableId, so on a server it is shared by every concurrent request; seeding the server snapshot from it would let one request render data another request fetched for the same path. Only config is read here, because it arrives from the caller on this render. Today that leak is unreachable because SSR throws first, so fixing the crash without this constraint would trade a crash for a cross-request data disclosure. Adds four tests under a "Server rendering" block, all mutation verified: - dropping the third argument fails all four with React's own error - returning observable.immutableStatus instead (the straightforward implementation) passes three and fails only the leak test Fixes FirebaseExtended#748.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #748.
v5is not the default branch, so that keyword will not fire on merge. #748 needs hand-closing.useObservablecalleduseSyncExternalStorewith two arguments. React requires a third,getServerSnapshot, whenever the tree is server rendered or hydrated. Without it React throwsMissing getServerSnapshot, which is required for server-rendered contentand the surrounding subtree silently falls back to client rendering, which is why a Next.js App Router page using any reactfire hook loses SSR for that subtree.The part worth reviewing
The issue proposes returning "the same seeded
immutableStatusthe client snapshot returns". That version is unsafe and this PR deliberately does not do it.preloadedObservablesis aMaponglobalThis, keyed only byobservableId. In a browser that is one user's cache. On a server it is shared by every concurrent request, so agetServerSnapshotthat readobservable.immutableStatuswould render data request A fetched into request B's HTML whenever both touch the same path.That leak is unreachable today only because SSR throws before it can happen. So fixing the crash the obvious way would trade a crash for a cross-request data disclosure, which is the worse of the two.
This implementation reads only
config, which arrives from the caller on the current render and is therefore per-request. WithinitialDatait reportssuccessand that value; without it,loading. The result is held in a ref so the value is stable across renders, which is what React's "The result of getSnapshot should be cached" check wants.Verification
Four tests under a new
Server renderingblock, and both mutations were run rather than assumed:Missing getServerSnapshoterror. So the tests are load-bearing rather than decorative.observable.immutableStatusinstead (the straightforward implementation): 3 of 4 pass and only the leak test fails. That is the test that earns its place, and it is why the constraint above is written down in the code rather than left to reviewer memory.tscpasses on bothtsconfig.jsonandtsconfig.test.json;useObservableis 22/22; eslint reports 0 errors on both changed files.Notes
use-sync-external-store/shimis a red herring. Its own implementation ignoresgetServerSnapshoton purpose, but it resolves toReact.useSyncExternalStorewhenever React exposes it, so on React 18 and 19 the third argument reaches React's real implementation. Reading only the shim would suggest this fix cannot work.globalThiscache is still a cross-request hazard for anything that does read it on a server. This PR contains the hazard at the one place it would otherwise become reachable; it does not fix the cache itself. That is separate SSR work.eslint-disableforreact-hooks/exhaustive-deps, with the reason in a comment above it: callers routinely pass a freshconfigliteral each render, and the ref means the value is computed once per component instance anyway.