From 9f26b11666e34adda532e4f8538116c413f14d50 Mon Sep 17 00:00:00 2001 From: Tyler Dixon Date: Wed, 5 Aug 2026 11:22:46 -0700 Subject: [PATCH 1/2] fix(ssr): add getServerSnapshot to useObservable's useSyncExternalStore 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 #748. --- src/useObservable.ts | 39 ++++++++++++++++++++++++- test/useObservable.test.tsx | 58 ++++++++++++++++++++++++++++++++++++- 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/src/useObservable.ts b/src/useObservable.ts index 6364c9bf..07ace14a 100644 --- a/src/useObservable.ts +++ b/src/useObservable.ts @@ -104,7 +104,44 @@ export function useObservable(observableId: string, source: Observa return observable.immutableStatus; }, [observable]); - const update = useSyncExternalStore(subscribe, getSnapshot); + // `useSyncExternalStore` requires a third argument when the tree is rendered on a + // server or hydrated; without it React throws "Missing getServerSnapshot, which is + // required for server-rendered content" and the surrounding subtree falls back to + // client rendering. + // + // This 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 safe to read here, because it comes from the caller on this render. + // + // The result is memoized per component instance because React compares the value it + // returns across renders, and a fresh object each time is what triggers the + // "The result of getSnapshot should be cached" error. + const serverSnapshotRef = React.useRef | undefined>(undefined); + const getServerSnapshot = React.useCallback<() => ObservableStatus>(() => { + if (serverSnapshotRef.current === undefined) { + const initialDataValue = config?.initialData ?? config?.startWithValue; + + serverSnapshotRef.current = { + status: hasInitialData ? 'success' : 'loading', + hasEmitted: hasInitialData, + isComplete: false, + data: initialDataValue, + error: undefined, + firstValuePromise: observable.firstEmission + } as ObservableStatus; + } + + return serverSnapshotRef.current; + // `config.initialData` and `config.startWithValue` are read above but deliberately left + // out of the dependency array. Callers routinely pass a fresh `config` literal on every + // render, so including them would rebuild this callback constantly, and the ref means + // the value is computed once per component instance regardless. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [observable, hasInitialData]); + + const update = useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot); // Return a new object with initialData overlaid rather than mutating the shared // _immutableStatus reference, which is the same object across all components diff --git a/test/useObservable.test.tsx b/test/useObservable.test.tsx index fd7250a9..fd4b8492 100644 --- a/test/useObservable.test.tsx +++ b/test/useObservable.test.tsx @@ -1,8 +1,9 @@ import '@testing-library/jest-dom/extend-expect'; import { act, cleanup, render, renderHook, waitFor } from '@testing-library/react'; import * as React from 'react'; +import { renderToString } from 'react-dom/server'; import { of, Subject, BehaviorSubject, throwError } from 'rxjs'; -import { useObservable, FirebaseAppProvider } from '../src/index'; +import { useObservable, FirebaseAppProvider, ReactFireOptions } from '../src/index'; import { initializeApp } from 'firebase/app'; import { baseConfig } from './appConfig'; @@ -391,4 +392,59 @@ describe('useObservable', () => { window.removeEventListener('error', onError); }); }); + + describe('Server rendering', () => { + // Renders `status` and `data` so the assertions read the snapshot React actually used, + // rather than a value the test computed for itself. + const Probe = ({ observableId, observable$, config }: { observableId: string; observable$: Subject; config?: ReactFireOptions }) => { + const { status, data } = useObservable(observableId, observable$, { suspense: false, ...config }); + // A single interpolated child, because adjacent JSX text nodes render with `` + // separators between them and the assertions below match on the plain string. + return
{`${status}:${String(data)}`}
; + }; + + it('renders on the server instead of throwing', () => { + const observable$: Subject = new Subject(); + + // Without a getServerSnapshot, React throws "Missing getServerSnapshot, which is + // required for server-rendered content" and the whole subtree falls back to client + // rendering. This is the #748 regression test: delete the third argument to + // useSyncExternalStore and this assertion fails. + expect(() => renderToString()).not.toThrow(); + }); + + it('reports loading on the server when there is no initialData', () => { + const observable$: Subject = new Subject(); + + const html = renderToString(); + + expect(html).toContain('loading:undefined'); + }); + + it('reports initialData on the server when it is provided', () => { + const observable$: Subject = new Subject(); + + const html = renderToString(); + + expect(html).toContain('success:seeded'); + }); + + it('does not leak a cached value from another request into the server snapshot', async () => { + // `preloadedObservables` lives on `globalThis` and is keyed only by observableId, so on + // a server every concurrent request shares it. A getServerSnapshot that read + // `observable.immutableStatus` would render whatever the previous request left behind. + // Here the first render stands in for that earlier request. + const observable$: Subject = new Subject(); + const observableId = 'ssr-no-cross-request-leak'; + + const { result } = renderHook(() => useObservable(observableId, observable$, { suspense: false })); + act(() => observable$.next('first-request-secret')); + await waitFor(() => expect(result.current.data).toEqual('first-request-secret')); + + const html = renderToString(); + + expect(html).not.toContain('first-request-secret'); + expect(html).toContain('loading:undefined'); + }); + }); }); From f2882a3bf811acfd842aac14cb63836ca8e720fc Mon Sep 17 00:00:00 2001 From: Tyler Dixon Date: Thu, 6 Aug 2026 12:15:10 -0700 Subject: [PATCH 2/2] test(ssr): cover the initialData branch of getServerSnapshot Armando found that the branch was uncovered: neutering it to always return loading/false/undefined leaves all 22 tests passing. Verified independently before writing this. The reason is that the overlay below handles the ordinary case. Whenever `!observable.hasValue && hasData`, it sets status, data and hasEmitted itself, so the server snapshot never gets to decide anything. The branch is only reachable when the shared cache ALREADY holds a value for the id and the caller also passes `initialData`: the overlay is skipped and the server snapshot ships, correctly preferring the caller's value over the stored one. The new test seeds the cache the way the leak test does, then server-renders the same observableId with `initialData`. Mutation-verified: against the neutered branch it fails with `loading:undefined`, and it passes against the real one. It doubles as a second cross-request check. Also corrects two things in the comment, both his: - It claimed only `config` is read here, while `firstValuePromise` reads `observable.firstEmission` from the shared cache. Nothing leaks, since that is a `Promise`, but the wording was wrong. The comment now says why it is safe, and records that the `as ObservableStatus` cast would hide the field going missing from both tsc and the suite. - React's warning names `getServerSnapshot`, not `getSnapshot`. Adds a clause scoping the protection to React 18+, since below that the shim ignores the third argument on the server too. --- src/useObservable.ts | 17 ++++++++++++++--- test/useObservable.test.tsx | 18 ++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/src/useObservable.ts b/src/useObservable.ts index 07ace14a..dc1d833b 100644 --- a/src/useObservable.ts +++ b/src/useObservable.ts @@ -109,15 +109,26 @@ export function useObservable(observableId: string, source: Observa // required for server-rendered content" and the surrounding subtree falls back to // client rendering. // + // This applies on React 18 and up. Below that, `use-sync-external-store/shim` ignores the + // third argument on the server as well as the client, so both shapes render identically + // and nothing here regresses. It also does nothing for it. + // // This 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 safe to read here, because it comes from the caller on this render. + // it would let one request render data another request fetched for the same path. + // + // So no field below carries data across requests: `status`, `hasEmitted` and `data` come + // from `config`, which is the caller's own input on this render. `firstValuePromise` does + // read the shared `observable`, but it is a `Promise` that resolves without a value, + // so it discloses nothing. It is not optional on `ObservableStatus`, and the + // `as ObservableStatus` cast below means neither `tsc` nor the tests would notice if it + // were dropped: a caller doing `status.firstValuePromise.then(...)` on the server would + // just throw. // // The result is memoized per component instance because React compares the value it // returns across renders, and a fresh object each time is what triggers the - // "The result of getSnapshot should be cached" error. + // "The result of getServerSnapshot should be cached" error. const serverSnapshotRef = React.useRef | undefined>(undefined); const getServerSnapshot = React.useCallback<() => ObservableStatus>(() => { if (serverSnapshotRef.current === undefined) { diff --git a/test/useObservable.test.tsx b/test/useObservable.test.tsx index fd4b8492..4ec50261 100644 --- a/test/useObservable.test.tsx +++ b/test/useObservable.test.tsx @@ -446,5 +446,23 @@ describe('useObservable', () => { expect(html).not.toContain('first-request-secret'); expect(html).toContain('loading:undefined'); }); + + it('prefers the callers initialData over a value already in the shared cache', async () => { + // The branch above that reads `initialData` is only reachable when the cache ALREADY + // holds a value for this id: otherwise the overlay in `useObservable` sets status, + // data and hasEmitted itself and the server snapshot never decides anything. So seed + // the cache first, exactly as the leak test does, and only then pass `initialData`. + const observable$: Subject = new Subject(); + const observableId = 'ssr-initial-data-beats-cache'; + + const { result } = renderHook(() => useObservable(observableId, observable$, { suspense: false })); + act(() => observable$.next('another-requests-value')); + await waitFor(() => expect(result.current.data).toEqual('another-requests-value')); + + const html = renderToString(); + + expect(html).toContain('success:my-own-data'); + expect(html).not.toContain('another-requests-value'); + }); }); });