diff --git a/src/useObservable.ts b/src/useObservable.ts index 6364c9bf..dc1d833b 100644 --- a/src/useObservable.ts +++ b/src/useObservable.ts @@ -104,7 +104,55 @@ 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 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. + 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..4ec50261 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,77 @@ 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'); + }); + + 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'); + }); + }); });