From 27dfc441ebab666d5b677a58b954c4191380b7c2 Mon Sep 17 00:00:00 2001 From: Tyler Dixon Date: Wed, 5 Aug 2026 11:22:46 -0700 Subject: [PATCH 1/4] 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 f66a5522..94b33e1f 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 f16d327a..deb1bce1 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 } from '../src/index'; +import { useObservable, ReactFireOptions } from '../src/index'; describe('useObservable', () => { afterEach(cleanup); @@ -329,4 +330,59 @@ describe('useObservable', () => { expect(refreshedComp).toHaveTextContent('James'); }); }); + + 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 6e42acf086e4307015f9b4ad6419c53f8b751e97 Mon Sep 17 00:00:00 2001 From: Tyler Dixon Date: Thu, 6 Aug 2026 12:15:10 -0700 Subject: [PATCH 2/4] 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 94b33e1f..d7251faf 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 deb1bce1..e7b9c40f 100644 --- a/test/useObservable.test.tsx +++ b/test/useObservable.test.tsx @@ -384,5 +384,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'); + }); }); }); From dc9884eee7f084ddc77c88ff5034cbbdc1b8616a Mon Sep 17 00:00:00 2001 From: Tyler Dixon Date: Wed, 19 Aug 2026 13:21:30 -0700 Subject: [PATCH 3/4] docs(ssr): prune comments on the getServerSnapshot change --- src/useObservable.ts | 36 +++++++----------------------------- test/useObservable.test.tsx | 24 ++++++++---------------- 2 files changed, 15 insertions(+), 45 deletions(-) diff --git a/src/useObservable.ts b/src/useObservable.ts index d7251faf..f6b45079 100644 --- a/src/useObservable.ts +++ b/src/useObservable.ts @@ -104,31 +104,11 @@ export function useObservable(observableId: string, source: Observa return observable.immutableStatus; }, [observable]); - // `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 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. - // - // 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 getServerSnapshot should be cached" error. + // Reads only `config`, never `observable.immutableStatus`: `preloadedObservables` is a + // `globalThis` cache keyed only by `observableId`, so a server shares it across concurrent + // requests, and seeding from it would render one request's data into another's HTML. + // The `as` cast below hides a missing `firstValuePromise` from `tsc` and the tests. + // Held in a ref because React requires a stable value across renders. const serverSnapshotRef = React.useRef | undefined>(undefined); const getServerSnapshot = React.useCallback<() => ObservableStatus>(() => { if (serverSnapshotRef.current === undefined) { @@ -145,10 +125,8 @@ export function useObservable(observableId: string, source: Observa } 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. + // Callers pass a fresh `config` literal each render, so the fields read above are kept + // out of the deps; the ref computes the value once per instance anyway. // eslint-disable-next-line react-hooks/exhaustive-deps }, [observable, hasInitialData]); diff --git a/test/useObservable.test.tsx b/test/useObservable.test.tsx index e7b9c40f..5f1bb300 100644 --- a/test/useObservable.test.tsx +++ b/test/useObservable.test.tsx @@ -332,22 +332,18 @@ describe('useObservable', () => { }); 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. + // Renders `status` and `data` so assertions read the snapshot React actually used. 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. + // One interpolated child: adjacent JSX text nodes render with `` between them. 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. + // The #748 regression test: delete the third argument to useSyncExternalStore and + // this fails with "Missing getServerSnapshot". expect(() => renderToString()).not.toThrow(); }); @@ -368,10 +364,8 @@ describe('useObservable', () => { }); 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. + // `preloadedObservables` is on `globalThis`, keyed only by observableId, so concurrent + // server requests share it. The first render below stands in for an earlier request. const observable$: Subject = new Subject(); const observableId = 'ssr-no-cross-request-leak'; @@ -386,10 +380,8 @@ describe('useObservable', () => { }); 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`. + // The `initialData` branch is only reachable when the cache already holds a value for + // this id; otherwise `useObservable`'s overlay decides and the snapshot never does. const observable$: Subject = new Subject(); const observableId = 'ssr-initial-data-beats-cache'; From 22feb8eca99af8eb940ec712f472590eed9ab43f Mon Sep 17 00:00:00 2001 From: Tyler Dixon Date: Wed, 19 Aug 2026 14:01:17 -0700 Subject: [PATCH 4/4] test(ssr): cover the streaming renderer as well as renderToString --- test/useObservable.test.tsx | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/test/useObservable.test.tsx b/test/useObservable.test.tsx index 5f1bb300..56fb96a0 100644 --- a/test/useObservable.test.tsx +++ b/test/useObservable.test.tsx @@ -1,7 +1,8 @@ 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 { Writable } from 'node:stream'; +import { renderToString, renderToPipeableStream } from 'react-dom/server'; import { of, Subject, BehaviorSubject, throwError } from 'rxjs'; import { useObservable, ReactFireOptions } from '../src/index'; @@ -347,6 +348,37 @@ describe('useObservable', () => { expect(() => renderToString()).not.toThrow(); }); + // The App Router streams rather than calling renderToString, and streaming surfaces + // failures the synchronous renderer does not, so the fix is checked against both. + it('renders on the server under the streaming renderer', async () => { + const observable$: Subject = new Subject(); + let error: unknown; + + const html = await new Promise((resolve, reject) => { + const chunks: string[] = []; + const sink = new Writable({ + write(chunk, _encoding, callback) { + chunks.push(chunk.toString()); + callback(); + } + }); + sink.on('finish', () => resolve(chunks.join(''))); + sink.on('error', reject); + + const stream = renderToPipeableStream(, { + onError(e) { + error = e; + }, + onAllReady() { + stream.pipe(sink); + } + }); + }); + + expect(error).toBeUndefined(); + expect(html).toContain('loading:undefined'); + }); + it('reports loading on the server when there is no initialData', () => { const observable$: Subject = new Subject();