From 30608ce6a1d3610183745845d514b25abea62065 Mon Sep 17 00:00:00 2001 From: Tryston Perry Date: Wed, 10 Jun 2026 13:47:02 -0700 Subject: [PATCH 1/5] feat(useCollection): add opt-in queryKey for value-based subscription keying Approach v1 (caller-supplied key). When provided, the subscription is keyed on queryKey instead of the queryConstraints array reference, so upstream reference churn (e.g. arrays read from deep-cloned documents) does not tear down and reload the listener. Latest constraints are read through a ref at rebuild time. - queryKey?: string on UseCollectionOptions - regression tests in src/hooks.test.ts (react-test-renderer) - README / docs / AGENTS.md contract updates --- AGENTS.md | 5 +- README.md | 14 ++- docs/api-recipes.md | 37 ++++++++ docs/architecture.md | 5 + package.json | 2 + pnpm-lock.yaml | 54 +++++++++++ src/hooks.test.ts | 213 +++++++++++++++++++++++++++++++++++++++++++ src/hooks.ts | 46 +++++++++- 8 files changed, 369 insertions(+), 7 deletions(-) create mode 100644 src/hooks.test.ts diff --git a/AGENTS.md b/AGENTS.md index 44e8b64..94c9dae 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,7 +85,10 @@ Preserve these unless the task explicitly changes them. - `enabled: false` on hooks must not resolve paths or create subscriptions. It returns stable no-op handles. - `queryConstraints` should be treated by reference. Callers are expected to - memoize inline arrays. + memoize inline arrays. Never deep-compare `QueryConstraint` objects — they + are opaque. `queryKey` is the explicit opt-in for value-based subscription + keying: when provided, the listener survives constraint-array reference + churn and rebuilds only when the key (or path/`readOnly`) changes. - `useSyncExternalStore` snapshots and handles must have stable identity between changes. Do not rebuild snapshots on every `getSnapshot()` call. - Unmounting a subscription clears its autosave timer and unregisters its sync diff --git a/README.md b/README.md index 307d193..49d63fc 100644 --- a/README.md +++ b/README.md @@ -453,7 +453,7 @@ const spaces = useSpaces({ projectId }) ``` Use the second argument for hook options such as `enabled`, `readOnly`, -`undoable`, or collection `queryConstraints`: +`undoable`, or collection `queryConstraints` / `queryKey`: ```tsx const spaces = useSpaces( @@ -577,6 +577,18 @@ const { enabled: true, // Optional: set false until required params exist }) +// Memoize queryConstraints — the subscription is keyed on the array +// reference, and a new reference rebuilds the Firestore listener. If the +// values feeding the constraints can change reference without changing +// meaning (e.g. arrays inside another document that Firestate deep-clones +// on optimistic updates), pass a value-derived queryKey instead; the +// listener then rebuilds only when the key changes: +const stations = useCollection({ + definition: weatherStations, + queryConstraints: [where(documentId(), 'in', stationIds)], + queryKey: stationIds.join('\n'), +}) + // Update existing documents update({ space1: { name: 'Updated Name' } }) diff --git a/docs/api-recipes.md b/docs/api-recipes.md index 24d3205..15d5b1a 100644 --- a/docs/api-recipes.md +++ b/docs/api-recipes.md @@ -214,6 +214,43 @@ const queryConstraints = useMemo( const spaces = useSpaces({ projectId }, { queryConstraints }) ``` +### Dynamic queries built from document data: use `queryKey` + +`useMemo` keys on its dependencies *by reference*. If a dependency is an array +or object read out of another Firestate document, its reference changes on +every optimistic update to that document — Firestate deep-clones local state on +edit — even when the contents are identical. The memo then produces a new +constraints array, the listener is torn down and re-attached, `isLoading` +flips back to `true`, and any loading gate above the hook flashes. + +Pass `queryKey` to key the subscription on the underlying values instead. The +listener then rebuilds only when the key changes: + +```tsx +import { documentId, where } from 'firebase/firestore' +import { useMemo } from 'react' + +// stationIds comes from another document and may change reference on +// every edit to that document, even when its contents are unchanged. +const stationIds = project.data?.weatherSpec.nearestWeatherStationIds ?? [] + +const queryConstraints = useMemo( + () => [where(documentId(), 'in', stationIds)], + // eslint-disable-next-line react-hooks/exhaustive-deps + [stationIds.join('\n')] +) + +const stations = useWeatherStations( + {}, + { queryConstraints, queryKey: stationIds.join('\n') } +) +``` + +With `queryKey` set, the `queryConstraints` reference no longer matters — you +can even pass an inline array. Keep the key derived from the same values the +constraints are built from; if the key understates the query (e.g. omits a +filter value), the listener will not rebuild when that value changes. + ## Undo and Redo Undo is enabled by default. diff --git a/docs/architecture.md b/docs/architecture.md index e11854b..41a2490 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -114,6 +114,11 @@ Important details: - Disabled hooks do not resolve params or create subscriptions. - Toggling `undoable` should not rebuild Firestore listeners. - `queryConstraints` are compared by reference; callers should memoize arrays. + `QueryConstraint` objects are opaque, so Firestate never deep-compares them. + When upstream state churns array references without changing the semantic + query (e.g. ids read from a deep-cloned document), callers pass a + value-derived `queryKey` string and the subscription is keyed on that + instead. - Subscription handles are cached until state changes, so React sees stable snapshots between commits. diff --git a/package.json b/package.json index 5f5ee87..062e038 100644 --- a/package.json +++ b/package.json @@ -37,9 +37,11 @@ }, "devDependencies": { "@types/react": "^18.3.12", + "@types/react-test-renderer": "18.3.1", "firebase": "^12.0.0", "prettier": "^3.8.3", "react": "^18.3.1", + "react-test-renderer": "18.3.1", "tsdown": "^0.20.0-beta.3", "typescript": "^5.7.2", "vitest": "^2.1.8", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f1bca18..e9451c6 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -11,6 +11,9 @@ importers: '@types/react': specifier: ^18.3.12 version: 18.3.27 + '@types/react-test-renderer': + specifier: 18.3.1 + version: 18.3.1 firebase: specifier: ^12.0.0 version: 12.14.0 @@ -20,6 +23,9 @@ importers: react: specifier: ^18.3.1 version: 18.3.1 + react-test-renderer: + specifier: 18.3.1 + version: 18.3.1(react@18.3.1) tsdown: specifier: ^0.20.0-beta.3 version: 0.20.0-beta.3(typescript@5.9.3) @@ -708,6 +714,9 @@ packages: '@types/prop-types@15.7.15': resolution: {integrity: sha512-F6bEyamV9jKGAFBEmlQnesRPGOQqS2+Uwi0Em15xenOxHaf2hv6L8YCVn3rPdPJOiJfPiCnLIRyvwVaqMY3MIw==} + '@types/react-test-renderer@18.3.1': + resolution: {integrity: sha512-vAhnk0tG2eGa37lkU9+s5SoroCsRI08xnsWFiAXOuPH2jqzMbcXvKExXViPi1P5fIklDeCvXqyrdmipFaSkZrA==} + '@types/react@18.3.27': resolution: {integrity: sha512-cisd7gxkzjBKU2GgdYrTdtQx1SORymWyaAFhaxQPK9bYO9ot3Y5OikQRvY0VYQtvwjeQnizCINJAenh/V7MK2w==} @@ -917,6 +926,10 @@ packages: engines: {node: ^10 || ^12 || ^13.7 || ^14 || >=15.0.1} hasBin: true + object-assign@4.1.1: + resolution: {integrity: sha512-rJgTQnkUnH1sFw8yT6VSU3zD3sWmu6sZhIseY8VX+GRu3P6F7Fu+JNDoXfklElbLJSnc3FUQHVe4cU5hj+BcUg==} + engines: {node: '>=0.10.0'} + obug@2.1.1: resolution: {integrity: sha512-uTqF9MuPraAQ+IsnPf366RG4cP9RtUi7MLO1N3KEc+wb0a6yKpeL0lmk2IB1jY5KHPAlTc6T/JRdC/YqxHNwkQ==} @@ -953,6 +966,19 @@ packages: quansync@1.0.0: resolution: {integrity: sha512-5xZacEEufv3HSTPQuchrvV6soaiACMFnq1H8wkVioctoH3TRha9Sz66lOxRwPK/qZj7HPiSveih9yAyh98gvqA==} + react-is@18.3.1: + resolution: {integrity: sha512-/LLMVyas0ljjAtoYiPqYiL8VWXzUUdThrmU5+n20DZv+a+ClRoevUzw5JxU+Ieh5/c87ytoTBV9G1FiKfNJdmg==} + + react-shallow-renderer@16.15.0: + resolution: {integrity: sha512-oScf2FqQ9LFVQgA73vr86xl2NaOIX73rh+YFqcOp68CWj56tSfgtGKrEbyhCj0rSijyG9M1CYprTh39fBi5hzA==} + peerDependencies: + react: ^16.0.0 || ^17.0.0 || ^18.0.0 + + react-test-renderer@18.3.1: + resolution: {integrity: sha512-KkAgygexHUkQqtvvx/otwxtuFu5cVjfzTCtjXLH9boS19/Nbtg84zS7wIQn39G8IlrhThBpQsMKkq5ZHZIYFXA==} + peerDependencies: + react: ^18.3.1 + react@18.3.1: resolution: {integrity: sha512-wS+hAgJShR0KhEvPJArfuPVN1+Hz1t0Y6n5jLrGQbkb4urgPE/0Rve+1kMB1v/oWgHgm4WIcV+i7F2pTVj+2iQ==} engines: {node: '>=0.10.0'} @@ -996,6 +1022,9 @@ packages: safe-buffer@5.2.1: resolution: {integrity: sha512-rp3So07KcdmmKbGvgaNxQSJr7bGVSVk5S9Eq1F+ppbRo70+YeaDxkw5Dd8NPN+GD6bjnYm2VuPuCXmpuYvmCXQ==} + scheduler@0.23.2: + resolution: {integrity: sha512-UOShsPwz7NrMUqhR6t0hWjFduvOzbtv7toDH1/hIrfRNIDBnnBWd0CwJTGvTpngVlmwGCdP9/Zl/tVrDqcuYzQ==} + semver@7.7.3: resolution: {integrity: sha512-SdsKMrI9TdgjdweUSR9MweHA4EJ8YxHn8DFaDisvhVlUOe4BF1tLD7GAj0lIqWVl+dPb/rExr0Btby5loQm20Q==} engines: {node: '>=10'} @@ -1818,6 +1847,10 @@ snapshots: '@types/prop-types@15.7.15': {} + '@types/react-test-renderer@18.3.1': + dependencies: + '@types/react': 18.3.27 + '@types/react@18.3.27': dependencies: '@types/prop-types': 15.7.15 @@ -2038,6 +2071,8 @@ snapshots: nanoid@3.3.11: {} + object-assign@4.1.1: {} + obug@2.1.1: {} pathe@1.1.2: {} @@ -2075,6 +2110,21 @@ snapshots: quansync@1.0.0: {} + react-is@18.3.1: {} + + react-shallow-renderer@16.15.0(react@18.3.1): + dependencies: + object-assign: 4.1.1 + react: 18.3.1 + react-is: 18.3.1 + + react-test-renderer@18.3.1(react@18.3.1): + dependencies: + react: 18.3.1 + react-is: 18.3.1 + react-shallow-renderer: 16.15.0(react@18.3.1) + scheduler: 0.23.2 + react@18.3.1: dependencies: loose-envify: 1.4.0 @@ -2151,6 +2201,10 @@ snapshots: safe-buffer@5.2.1: {} + scheduler@0.23.2: + dependencies: + loose-envify: 1.4.0 + semver@7.7.3: {} siginfo@2.0.0: {} diff --git a/src/hooks.test.ts b/src/hooks.test.ts new file mode 100644 index 0000000..71deef4 --- /dev/null +++ b/src/hooks.test.ts @@ -0,0 +1,213 @@ +/** + * Regression tests for `useCollection` subscription identity. + * + * Motivating bug (HVAKR full-app flash): an app derived `queryConstraints` + * from an array living inside a document that Firestate deep-clones on every + * optimistic update. The array's *contents* never changed, but its reference + * did — so the memoized constraints array changed reference, `useCollection` + * tore down the Firestore listener, recreated the subscription with + * `isLoading: true` / `data: {}`, and the app's loading gate unmounted the + * whole route. + * + * `queryKey` is the escape hatch: when provided, the subscription is keyed + * on that string instead of the constraints array reference. + */ +import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest' + +vi.mock('firebase/firestore', async () => { + const actual = + await vi.importActual( + 'firebase/firestore' + ) + return { + ...actual, + collection: vi.fn((_firestore: unknown, path: string) => ({ + __mockColl: path, + })), + query: vi.fn((ref: unknown, ...constraints: unknown[]) => ({ + __mockQuery: { ref, constraints }, + })), + onSnapshot: vi.fn(), + } +}) + +import { createElement, type ReactElement } from 'react' +import { + create, + act, + type ReactTestRenderer, +} from 'react-test-renderer' +import * as firestore from 'firebase/firestore' +import { documentId, where, type QueryConstraint } from 'firebase/firestore' +import { useCollection, FirestateContext } from './hooks' +import { defineCollection } from './schema' +import { createStore, type FirestateStore } from './store' +import type { CollectionHandle, FirestoreObject } from './types' + +interface Station extends FirestoreObject { + name: string +} + +const stationsCollection = defineCollection({ + path: 'weatherStations', +}) + +type MockSnapshot = { + docs: Array<{ id: string; data: () => Record }> +} + +describe('useCollection queryConstraints identity', () => { + let store: FirestateStore + let renderer: ReactTestRenderer | undefined + // One entry per Firestore listener that was attached. + let listeners: Array<{ + deliver: (snapshot: MockSnapshot) => void + unsubscribe: ReturnType + }> + let latestHandle: CollectionHandle + + const onSnapshotMock = firestore.onSnapshot as unknown as ReturnType< + typeof vi.fn + > + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + store = createStore({ firestore: {} as never }) + listeners = [] + onSnapshotMock.mockImplementation( + (_query: unknown, onNext: (snapshot: MockSnapshot) => void) => { + const unsubscribe = vi.fn() + listeners.push({ deliver: onNext, unsubscribe }) + return unsubscribe + } + ) + }) + + afterEach(() => { + act(() => { + renderer?.unmount() + }) + renderer = undefined + vi.useRealTimers() + }) + + const Probe = ({ + queryConstraints, + queryKey, + }: { + queryConstraints: QueryConstraint[] + queryKey?: string + }) => { + latestHandle = useCollection({ + definition: stationsCollection, + queryConstraints, + queryKey, + }) + return null + } + + const element = (props: { + queryConstraints: QueryConstraint[] + queryKey?: string + }): ReactElement => + createElement( + FirestateContext.Provider, + { value: store }, + createElement(Probe, props) + ) + + const constraintsFor = (ids: string[]): QueryConstraint[] => [ + where(documentId(), 'in', ids), + ] + + const snapshot: MockSnapshot = { + docs: [{ id: 'ws1', data: () => ({ name: 'Station 1' }) }], + } + + /** Mount, deliver the first snapshot, and settle minLoadTime. */ + const mountAndLoad = (props: { + queryConstraints: QueryConstraint[] + queryKey?: string + }) => { + act(() => { + renderer = create(element(props)) + }) + act(() => { + listeners[0]!.deliver(snapshot) + vi.runAllTimers() + }) + expect(latestHandle.isLoading).toBe(false) + expect(latestHandle.data.ws1?.name).toBe('Station 1') + } + + it('rebuilds the subscription when queryConstraints changes reference without queryKey (documented footgun)', () => { + const ids = ['ws1', 'ws2'] + mountAndLoad({ queryConstraints: constraintsFor(ids) }) + expect(onSnapshotMock).toHaveBeenCalledTimes(1) + + // Semantically identical query, new array references — as produced by + // deriving ids from a deep-cloned parent document. + act(() => { + renderer!.update( + element({ queryConstraints: constraintsFor([...ids]) }) + ) + }) + + // Old listener torn down, new one attached, loading state reset. + expect(onSnapshotMock).toHaveBeenCalledTimes(2) + expect(listeners[0]!.unsubscribe).toHaveBeenCalledTimes(1) + expect(latestHandle.isLoading).toBe(true) + expect(latestHandle.data).toEqual({}) + }) + + it('keeps the subscription across constraint reference churn when queryKey is stable', () => { + const ids = ['ws1', 'ws2'] + const queryKey = ids.join('\n') + mountAndLoad({ queryConstraints: constraintsFor(ids), queryKey }) + expect(onSnapshotMock).toHaveBeenCalledTimes(1) + + act(() => { + renderer!.update( + element({ + queryConstraints: constraintsFor([...ids]), + queryKey: [...ids].join('\n'), + }) + ) + }) + + // Same key → same subscription: no teardown, no reload, data intact. + expect(onSnapshotMock).toHaveBeenCalledTimes(1) + expect(listeners[0]!.unsubscribe).not.toHaveBeenCalled() + expect(latestHandle.isLoading).toBe(false) + expect(latestHandle.data.ws1?.name).toBe('Station 1') + }) + + it('rebuilds the subscription when queryKey changes', () => { + const ids = ['ws1', 'ws2'] + mountAndLoad({ + queryConstraints: constraintsFor(ids), + queryKey: ids.join('\n'), + }) + + const newIds = ['ws1', 'ws3'] + act(() => { + renderer!.update( + element({ + queryConstraints: constraintsFor(newIds), + queryKey: newIds.join('\n'), + }) + ) + }) + + expect(onSnapshotMock).toHaveBeenCalledTimes(2) + expect(listeners[0]!.unsubscribe).toHaveBeenCalledTimes(1) + expect(latestHandle.isLoading).toBe(true) + + // The new listener queries with the new constraints. + const queryMock = firestore.query as unknown as ReturnType + const lastQueryConstraints = + queryMock.mock.calls[queryMock.mock.calls.length - 1]!.slice(1) + expect(lastQueryConstraints).toEqual(constraintsFor(newIds)) + }) +}) diff --git a/src/hooks.ts b/src/hooks.ts index 9d86e76..debd873 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -290,6 +290,15 @@ export interface UseCollectionOptions { readOnly?: boolean; /** Additional query constraints */ queryConstraints?: QueryConstraint[]; + /** + * Stable identity key for `queryConstraints`. When provided, the + * subscription is keyed on this string instead of the `queryConstraints` + * array reference, so upstream state that recreates the array (e.g. a + * deep-cloned parent object) does not tear down and reload the listener + * as long as the key is unchanged. Derive it from the same values the + * constraints are built from, e.g. `queryKey: ids.join('\n')`. + */ + queryKey?: string; /** Enable undo/redo for this collection (default: true) */ undoable?: boolean; /** @@ -304,14 +313,28 @@ export interface UseCollectionOptions { * Hook to subscribe to a Firestore collection with real-time updates. * * The subscription is keyed on the resolved collection path, `readOnly`, and - * the `queryConstraints` reference. When any of these change, the listener - * is torn down and re-attached with the new query. Toggling `undoable` does - * not rebuild the subscription. + * the `queryConstraints` reference (or `queryKey` when provided). When any of + * these change, the listener is torn down and re-attached with the new query. + * Toggling `undoable` does not rebuild the subscription. * * **Memoize `queryConstraints`.** An inline array (`queryConstraints={[where(...)]}`) * creates a new reference every render, which will thrash the listener. * Wrap in `useMemo` with the underlying filter values as deps. * + * **Or pass `queryKey`.** `QueryConstraint` objects are opaque, so Firestate + * cannot deep-compare them. If the values feeding your constraints can change + * reference without changing meaning (e.g. arrays inside optimistically + * deep-cloned documents), provide a value-derived `queryKey` — the listener + * then survives reference churn and only rebuilds when the key changes: + * + * ```tsx + * const stations = useCollection({ + * definition: weatherStations, + * queryConstraints: [where(documentId(), 'in', stationIds)], + * queryKey: stationIds.join('\n'), + * }) + * ``` + * * Use `enabled: false` to suppress the subscription entirely (e.g., when * route params aren't ready yet). * @@ -356,6 +379,7 @@ export const useCollection = ( params = {}, readOnly, queryConstraints, + queryKey, undoable = true, enabled = true, } = options; @@ -386,6 +410,18 @@ export const useCollection = ( : definition.path : undefined; + // QueryConstraint objects are opaque, so they can't be deep-compared. By + // default the subscription is keyed on the array reference; an explicit + // `queryKey` opts into value-based keying so reference churn (e.g. arrays + // inside deep-cloned app state) doesn't tear down the listener. The latest + // constraints live in a ref so the memo can read them without depending on + // their identity — when the key is stable they are semantically equal by + // the caller's contract. + const queryConstraintsRef = useRef(queryConstraints); + queryConstraintsRef.current = queryConstraints; + const constraintsKey: string | QueryConstraint[] | undefined = + queryKey ?? queryConstraints; + const subscription = useMemo( () => enabled && collectionPath !== undefined @@ -394,7 +430,7 @@ export const useCollection = ( definition, collectionPath, readOnly, - queryConstraints, + queryConstraints: queryConstraintsRef.current, onPushUndo, }) : null, @@ -404,7 +440,7 @@ export const useCollection = ( definition, collectionPath, readOnly, - queryConstraints, + constraintsKey, onPushUndo, ] ); From e60a9d99349b5eefb59224a7ebf7959200826822 Mon Sep 17 00:00:00 2001 From: Tryston Perry Date: Wed, 10 Jun 2026 13:50:57 -0700 Subject: [PATCH 2/5] refactor(useCollection): key by semantic query identity (queryEqual), drop queryKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approach v2 (auto-stabilization, no public API). Replaces the caller-supplied queryKey with automatic stabilization: the hook builds the query and compares it with Firestore's own queryEqual, keeping the previous constraints reference when the query is unchanged. Reference churn (e.g. constraint inputs read from a deep-cloned document) no longer tears down the listener, and a genuine query change still rebuilds it — with no caller key and no silent-staleness footgun. - removes queryKey from UseCollectionOptions (public API) - queryConstraintsEqual()/buildCollectionQuery() helpers in hooks.ts - tests rewritten to assert semantic-identity behavior (real Firestore + real query/queryEqual, onSnapshot mocked) - README / docs / AGENTS.md updated; callers no longer need to memoize --- AGENTS.md | 11 ++-- README.md | 15 +++-- docs/api-recipes.md | 42 +++++++------- docs/architecture.md | 13 +++-- src/hooks.test.ts | 129 +++++++++++++++++++++---------------------- src/hooks.ts | 126 +++++++++++++++++++++++++++++------------- 6 files changed, 192 insertions(+), 144 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 94c9dae..6396d52 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,11 +84,12 @@ Preserve these unless the task explicitly changes them. bail before the initial snapshot to avoid clobbering unknown server fields. - `enabled: false` on hooks must not resolve paths or create subscriptions. It returns stable no-op handles. -- `queryConstraints` should be treated by reference. Callers are expected to - memoize inline arrays. Never deep-compare `QueryConstraint` objects — they - are opaque. `queryKey` is the explicit opt-in for value-based subscription - keying: when provided, the listener survives constraint-array reference - churn and rebuilds only when the key (or path/`readOnly`) changes. +- `queryConstraints` are keyed by *semantic query identity*, not by array + reference. Never hand-roll a deep compare of `QueryConstraint` objects — they + are opaque. `useCollection` builds the query and compares it with Firestore's + `queryEqual`, so a fresh array producing the same query does not rebuild the + listener; only a real change to the query (or `path`/`readOnly`) does. + Callers therefore do not need to memoize `queryConstraints` for correctness. - `useSyncExternalStore` snapshots and handles must have stable identity between changes. Do not rebuild snapshots on every `getSnapshot()` call. - Unmounting a subscription clears its autosave timer and unregisters its sync diff --git a/README.md b/README.md index 49d63fc..6069bab 100644 --- a/README.md +++ b/README.md @@ -453,7 +453,7 @@ const spaces = useSpaces({ projectId }) ``` Use the second argument for hook options such as `enabled`, `readOnly`, -`undoable`, or collection `queryConstraints` / `queryKey`: +`undoable`, or collection `queryConstraints`: ```tsx const spaces = useSpaces( @@ -577,16 +577,15 @@ const { enabled: true, // Optional: set false until required params exist }) -// Memoize queryConstraints — the subscription is keyed on the array -// reference, and a new reference rebuilds the Firestore listener. If the -// values feeding the constraints can change reference without changing -// meaning (e.g. arrays inside another document that Firestate deep-clones -// on optimistic updates), pass a value-derived queryKey instead; the -// listener then rebuilds only when the key changes: +// queryConstraints are keyed by query identity, not array reference: the +// subscription rebuilds only when the query actually changes (compared via +// Firestore's queryEqual). So a new array that produces the same query — +// e.g. stationIds read from a document Firestate deep-clones on every +// optimistic update — does NOT tear down the listener. You don't need to +// memoize for correctness: const stations = useCollection({ definition: weatherStations, queryConstraints: [where(documentId(), 'in', stationIds)], - queryKey: stationIds.join('\n'), }) // Update existing documents diff --git a/docs/api-recipes.md b/docs/api-recipes.md index 15d5b1a..7dcf90f 100644 --- a/docs/api-recipes.md +++ b/docs/api-recipes.md @@ -214,42 +214,38 @@ const queryConstraints = useMemo( const spaces = useSpaces({ projectId }, { queryConstraints }) ``` -### Dynamic queries built from document data: use `queryKey` +### Dynamic queries built from document data -`useMemo` keys on its dependencies *by reference*. If a dependency is an array -or object read out of another Firestate document, its reference changes on -every optimistic update to that document — Firestate deep-clones local state on -edit — even when the contents are identical. The memo then produces a new -constraints array, the listener is torn down and re-attached, `isLoading` -flips back to `true`, and any loading gate above the hook flashes. +A subtlety with the `useMemo` recipe above: `useMemo` keys on its dependencies +*by reference*. If a dependency is an array or object read out of another +Firestate document, its reference changes on every optimistic update to that +document — Firestate deep-clones local state on edit — even when the contents +are identical. The memo then produces a new constraints array on each edit. -Pass `queryKey` to key the subscription on the underlying values instead. The -listener then rebuilds only when the key changes: +`useCollection` handles this for you. It keys the subscription on the +*semantic identity* of the query, not the array reference: it builds the query +and compares it with Firestore's own `queryEqual`. A fresh array that produces +the same query is ignored, so the listener is not torn down, `isLoading` does +not flip back to `true`, and a loading gate above the hook does not flash. You +can pass constraints derived from churning document data directly: ```tsx import { documentId, where } from 'firebase/firestore' -import { useMemo } from 'react' -// stationIds comes from another document and may change reference on -// every edit to that document, even when its contents are unchanged. +// stationIds comes from another document and may change reference on every +// edit to that document, even when its contents are unchanged. The listener +// survives that churn — it only rebuilds when the query actually changes. const stationIds = project.data?.weatherSpec.nearestWeatherStationIds ?? [] -const queryConstraints = useMemo( - () => [where(documentId(), 'in', stationIds)], - // eslint-disable-next-line react-hooks/exhaustive-deps - [stationIds.join('\n')] -) - const stations = useWeatherStations( {}, - { queryConstraints, queryKey: stationIds.join('\n') } + { queryConstraints: [where(documentId(), 'in', stationIds)] } ) ``` -With `queryKey` set, the `queryConstraints` reference no longer matters — you -can even pass an inline array. Keep the key derived from the same values the -constraints are built from; if the key understates the query (e.g. omits a -filter value), the listener will not rebuild when that value changes. +Memoizing `queryConstraints` is still a fine micro-optimization — a stable +reference takes a fast path and skips the per-render query build + compare — +but it is no longer required to keep the listener stable. ## Undo and Redo diff --git a/docs/architecture.md b/docs/architecture.md index 41a2490..6c32ce9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -113,12 +113,13 @@ Important details: - Hooks return stable disabled handles when `enabled: false`. - Disabled hooks do not resolve params or create subscriptions. - Toggling `undoable` should not rebuild Firestore listeners. -- `queryConstraints` are compared by reference; callers should memoize arrays. - `QueryConstraint` objects are opaque, so Firestate never deep-compares them. - When upstream state churns array references without changing the semantic - query (e.g. ids read from a deep-cloned document), callers pass a - value-derived `queryKey` string and the subscription is keyed on that - instead. +- `queryConstraints` are keyed by semantic query identity, not array + reference. `QueryConstraint` objects are opaque, so Firestate never + hand-rolls a deep compare; instead `useCollection` builds the query and + compares it with Firestore's own `queryEqual`. When upstream state churns + array references without changing the query (e.g. ids read from a + deep-cloned document), the listener is preserved; a genuine query change + rebuilds it. Callers need not memoize the array for correctness. - Subscription handles are cached until state changes, so React sees stable snapshots between commits. diff --git a/src/hooks.test.ts b/src/hooks.test.ts index 71deef4..76e733e 100644 --- a/src/hooks.test.ts +++ b/src/hooks.test.ts @@ -9,8 +9,16 @@ * `isLoading: true` / `data: {}`, and the app's loading gate unmounted the * whole route. * - * `queryKey` is the escape hatch: when provided, the subscription is keyed - * on that string instead of the constraints array reference. + * The fix keys the subscription on the *semantic identity* of the query: + * Firestate builds the query and compares it with Firestore's own + * `queryEqual`, so a fresh constraints array that produces the same query is + * ignored, while a genuine query change still rebuilds the listener. No + * caller-supplied key is required. + * + * Unlike the rest of the suite, these tests use a real Firestore instance and + * real `query`/`queryEqual`/`where` — only `onSnapshot` (the network edge) is + * mocked. queryEqual operates on real Query objects, so mocking the query + * builders (as other tests do) would defeat the very thing under test. */ import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest' @@ -21,24 +29,24 @@ vi.mock('firebase/firestore', async () => { ) return { ...actual, - collection: vi.fn((_firestore: unknown, path: string) => ({ - __mockColl: path, - })), - query: vi.fn((ref: unknown, ...constraints: unknown[]) => ({ - __mockQuery: { ref, constraints }, - })), onSnapshot: vi.fn(), } }) import { createElement, type ReactElement } from 'react' +import { create, act, type ReactTestRenderer } from 'react-test-renderer' +import { initializeApp, type FirebaseApp } from 'firebase/app' import { - create, - act, - type ReactTestRenderer, -} from 'react-test-renderer' -import * as firestore from 'firebase/firestore' -import { documentId, where, type QueryConstraint } from 'firebase/firestore' + getFirestore, + onSnapshot, + documentId, + query, + where, + queryEqual, + collection, + type Firestore, + type QueryConstraint, +} from 'firebase/firestore' import { useCollection, FirestateContext } from './hooks' import { defineCollection } from './schema' import { createStore, type FirestateStore } from './store' @@ -52,6 +60,18 @@ const stationsCollection = defineCollection({ path: 'weatherStations', }) +// A real (offline) Firestore — building queries and refs is purely +// client-side; onSnapshot is mocked so nothing hits the network. +let app: FirebaseApp +let firestore: Firestore +try { + app = initializeApp({ projectId: 'firestate-test' }, 'firestate-test') +} catch { + // Already initialized by a previous run in the same worker. + app = initializeApp({ projectId: 'firestate-test' }, 'firestate-test-2') +} +firestore = getFirestore(app) + type MockSnapshot = { docs: Array<{ id: string; data: () => Record }> } @@ -66,14 +86,12 @@ describe('useCollection queryConstraints identity', () => { }> let latestHandle: CollectionHandle - const onSnapshotMock = firestore.onSnapshot as unknown as ReturnType< - typeof vi.fn - > + const onSnapshotMock = onSnapshot as unknown as ReturnType beforeEach(() => { vi.clearAllMocks() vi.useFakeTimers() - store = createStore({ firestore: {} as never }) + store = createStore({ firestore }) listeners = [] onSnapshotMock.mockImplementation( (_query: unknown, onNext: (snapshot: MockSnapshot) => void) => { @@ -94,22 +112,18 @@ describe('useCollection queryConstraints identity', () => { const Probe = ({ queryConstraints, - queryKey, }: { queryConstraints: QueryConstraint[] - queryKey?: string }) => { latestHandle = useCollection({ definition: stationsCollection, queryConstraints, - queryKey, }) return null } const element = (props: { queryConstraints: QueryConstraint[] - queryKey?: string }): ReactElement => createElement( FirestateContext.Provider, @@ -126,10 +140,7 @@ describe('useCollection queryConstraints identity', () => { } /** Mount, deliver the first snapshot, and settle minLoadTime. */ - const mountAndLoad = (props: { - queryConstraints: QueryConstraint[] - queryKey?: string - }) => { + const mountAndLoad = (props: { queryConstraints: QueryConstraint[] }) => { act(() => { renderer = create(element(props)) }) @@ -141,73 +152,61 @@ describe('useCollection queryConstraints identity', () => { expect(latestHandle.data.ws1?.name).toBe('Station 1') } - it('rebuilds the subscription when queryConstraints changes reference without queryKey (documented footgun)', () => { - const ids = ['ws1', 'ws2'] - mountAndLoad({ queryConstraints: constraintsFor(ids) }) - expect(onSnapshotMock).toHaveBeenCalledTimes(1) - - // Semantically identical query, new array references — as produced by - // deriving ids from a deep-cloned parent document. - act(() => { - renderer!.update( - element({ queryConstraints: constraintsFor([...ids]) }) - ) - }) - - // Old listener torn down, new one attached, loading state reset. - expect(onSnapshotMock).toHaveBeenCalledTimes(2) - expect(listeners[0]!.unsubscribe).toHaveBeenCalledTimes(1) - expect(latestHandle.isLoading).toBe(true) - expect(latestHandle.data).toEqual({}) + it('sanity: queryEqual treats fresh arrays with equal ids as the same query', () => { + const ref = collection(firestore, 'weatherStations') + const a = query(ref, where(documentId(), 'in', ['ws1', 'ws2'])) + const b = query(ref, where(documentId(), 'in', ['ws1', 'ws2'])) + const c = query(ref, where(documentId(), 'in', ['ws1', 'ws3'])) + expect(queryEqual(a, b)).toBe(true) + expect(queryEqual(a, c)).toBe(false) }) - it('keeps the subscription across constraint reference churn when queryKey is stable', () => { + it('keeps the subscription across constraint reference churn (semantically equal, no key needed)', () => { const ids = ['ws1', 'ws2'] - const queryKey = ids.join('\n') - mountAndLoad({ queryConstraints: constraintsFor(ids), queryKey }) + mountAndLoad({ queryConstraints: constraintsFor(ids) }) expect(onSnapshotMock).toHaveBeenCalledTimes(1) + // Semantically identical query, brand-new array + constraint + // references — as produced by deriving ids from a deep-cloned parent + // document. This is the regression: with reference keying the listener + // would tear down here. act(() => { renderer!.update( - element({ - queryConstraints: constraintsFor([...ids]), - queryKey: [...ids].join('\n'), - }) + element({ queryConstraints: constraintsFor([...ids]) }) ) }) - // Same key → same subscription: no teardown, no reload, data intact. + // Same query → same subscription: no teardown, no reload, data intact. expect(onSnapshotMock).toHaveBeenCalledTimes(1) expect(listeners[0]!.unsubscribe).not.toHaveBeenCalled() expect(latestHandle.isLoading).toBe(false) expect(latestHandle.data.ws1?.name).toBe('Station 1') }) - it('rebuilds the subscription when queryKey changes', () => { + it('rebuilds the subscription when the query actually changes', () => { const ids = ['ws1', 'ws2'] - mountAndLoad({ - queryConstraints: constraintsFor(ids), - queryKey: ids.join('\n'), - }) + mountAndLoad({ queryConstraints: constraintsFor(ids) }) + expect(onSnapshotMock).toHaveBeenCalledTimes(1) const newIds = ['ws1', 'ws3'] act(() => { renderer!.update( - element({ - queryConstraints: constraintsFor(newIds), - queryKey: newIds.join('\n'), - }) + element({ queryConstraints: constraintsFor(newIds) }) ) }) + // Different query → old listener torn down, new one attached, loading + // state reset. expect(onSnapshotMock).toHaveBeenCalledTimes(2) expect(listeners[0]!.unsubscribe).toHaveBeenCalledTimes(1) expect(latestHandle.isLoading).toBe(true) + expect(latestHandle.data).toEqual({}) // The new listener queries with the new constraints. - const queryMock = firestore.query as unknown as ReturnType - const lastQueryConstraints = - queryMock.mock.calls[queryMock.mock.calls.length - 1]!.slice(1) - expect(lastQueryConstraints).toEqual(constraintsFor(newIds)) + const queryArg = onSnapshotMock.mock.calls[1]![0] + const ref = collection(firestore, 'weatherStations') + expect( + queryEqual(queryArg, query(ref, ...constraintsFor(newIds))) + ).toBe(true) }) }) diff --git a/src/hooks.ts b/src/hooks.ts index debd873..00a7cbd 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -7,7 +7,13 @@ import { useRef, useSyncExternalStore, } from "react"; -import type { QueryConstraint } from "firebase/firestore"; +import { collection, query, queryEqual } from "firebase/firestore"; +import type { + CollectionReference, + Firestore, + Query, + QueryConstraint, +} from "firebase/firestore"; import type { CollectionDefinition, CollectionHandle, @@ -22,6 +28,48 @@ import type { FirestateStore } from "./store"; import { createDocumentSubscription } from "./document"; import { createCollectionSubscription } from "./collection"; +/** + * Build the Firestore query a collection subscription would run, mirroring + * the merge order in collection.ts (`definitionConstraints` then hook-level + * `extraConstraints`). With no constraints the bare collection reference is + * itself a valid `Query` for comparison purposes. + */ +const buildCollectionQuery = ( + ref: CollectionReference, + definitionConstraints: QueryConstraint[] | undefined, + extraConstraints: QueryConstraint[] | undefined +): Query => { + const all = [ + ...(definitionConstraints ?? []), + ...(extraConstraints ?? []), + ]; + return all.length > 0 ? query(ref, ...all) : ref; +}; + +/** + * Whether two hook-level `queryConstraints` arrays produce the same Firestore + * query for a collection. `QueryConstraint` objects are opaque, so instead of + * hand-rolling a deep compare we build both queries and defer to Firestore's + * own `queryEqual`, which structurally compares filters, ordering, limits, + * and cursors. This is what lets the subscription survive reference churn + * (e.g. constraint inputs read from a deep-cloned document) while still + * rebuilding when the query genuinely changes — no caller-supplied key. + */ +const queryConstraintsEqual = ( + firestore: Firestore, + collectionPath: string, + definitionConstraints: QueryConstraint[] | undefined, + a: QueryConstraint[] | undefined, + b: QueryConstraint[] | undefined +): boolean => { + if (a === b) return true; + const ref = collection(firestore, collectionPath) as CollectionReference; + return queryEqual( + buildCollectionQuery(ref, definitionConstraints, a), + buildCollectionQuery(ref, definitionConstraints, b) + ); +}; + /** * Returned when a hook is called with `enabled: false`. Module-level constants * so getSnapshot returns a stable reference and useSyncExternalStore doesn't @@ -290,15 +338,6 @@ export interface UseCollectionOptions { readOnly?: boolean; /** Additional query constraints */ queryConstraints?: QueryConstraint[]; - /** - * Stable identity key for `queryConstraints`. When provided, the - * subscription is keyed on this string instead of the `queryConstraints` - * array reference, so upstream state that recreates the array (e.g. a - * deep-cloned parent object) does not tear down and reload the listener - * as long as the key is unchanged. Derive it from the same values the - * constraints are built from, e.g. `queryKey: ids.join('\n')`. - */ - queryKey?: string; /** Enable undo/redo for this collection (default: true) */ undoable?: boolean; /** @@ -313,28 +352,30 @@ export interface UseCollectionOptions { * Hook to subscribe to a Firestore collection with real-time updates. * * The subscription is keyed on the resolved collection path, `readOnly`, and - * the `queryConstraints` reference (or `queryKey` when provided). When any of - * these change, the listener is torn down and re-attached with the new query. - * Toggling `undoable` does not rebuild the subscription. + * the *semantic identity* of `queryConstraints`. When any of these change, the + * listener is torn down and re-attached with the new query. Toggling + * `undoable` does not rebuild the subscription. * - * **Memoize `queryConstraints`.** An inline array (`queryConstraints={[where(...)]}`) - * creates a new reference every render, which will thrash the listener. - * Wrap in `useMemo` with the underlying filter values as deps. - * - * **Or pass `queryKey`.** `QueryConstraint` objects are opaque, so Firestate - * cannot deep-compare them. If the values feeding your constraints can change - * reference without changing meaning (e.g. arrays inside optimistically - * deep-cloned documents), provide a value-derived `queryKey` — the listener - * then survives reference churn and only rebuilds when the key changes: + * **You do not need to memoize `queryConstraints`.** `QueryConstraint` objects + * are opaque, so Firestate compares the *built query* with Firestore's own + * `queryEqual` instead of comparing array references. A fresh array that + * produces the same query (e.g. constraint inputs read from a document that + * Firestate deep-clones on optimistic updates) does not rebuild the listener; + * only a genuine change to the query does: * * ```tsx + * // stationIds may change reference on every edit to its parent document, + * // even when its contents are unchanged — the listener survives anyway. * const stations = useCollection({ * definition: weatherStations, * queryConstraints: [where(documentId(), 'in', stationIds)], - * queryKey: stationIds.join('\n'), * }) * ``` * + * Memoizing is still a fine micro-optimization (it skips the per-render query + * build + compare via the reference fast-path), but it is no longer required + * for listener stability. + * * Use `enabled: false` to suppress the subscription entirely (e.g., when * route params aren't ready yet). * @@ -379,7 +420,6 @@ export const useCollection = ( params = {}, readOnly, queryConstraints, - queryKey, undoable = true, enabled = true, } = options; @@ -410,17 +450,29 @@ export const useCollection = ( : definition.path : undefined; - // QueryConstraint objects are opaque, so they can't be deep-compared. By - // default the subscription is keyed on the array reference; an explicit - // `queryKey` opts into value-based keying so reference churn (e.g. arrays - // inside deep-cloned app state) doesn't tear down the listener. The latest - // constraints live in a ref so the memo can read them without depending on - // their identity — when the key is stable they are semantically equal by - // the caller's contract. - const queryConstraintsRef = useRef(queryConstraints); - queryConstraintsRef.current = queryConstraints; - const constraintsKey: string | QueryConstraint[] | undefined = - queryKey ?? queryConstraints; + // Stabilize `queryConstraints` by *query identity*. QueryConstraint objects + // are opaque and can't be deep-compared directly, but the built query can — + // see queryConstraintsEqual(). When the incoming array produces the same + // query as the one we're already subscribed with (e.g. constraint inputs + // read from a deep-cloned document churned the array reference without + // changing the query), we keep the previous array reference so the memo + // below does not rebuild. A genuine change adopts the new reference and + // re-attaches the listener. When the path is unresolved we can't build a + // query, so we just pass the constraints through (the memo returns null). + const stableConstraintsRef = useRef(queryConstraints); + if ( + collectionPath === undefined || + !queryConstraintsEqual( + store.firestore, + collectionPath, + definition.queryConstraints, + stableConstraintsRef.current, + queryConstraints + ) + ) { + stableConstraintsRef.current = queryConstraints; + } + const stableConstraints = stableConstraintsRef.current; const subscription = useMemo( () => @@ -430,7 +482,7 @@ export const useCollection = ( definition, collectionPath, readOnly, - queryConstraints: queryConstraintsRef.current, + queryConstraints: stableConstraints, onPushUndo, }) : null, @@ -440,7 +492,7 @@ export const useCollection = ( definition, collectionPath, readOnly, - constraintsKey, + stableConstraints, onPushUndo, ] ); From 208a33e4282b699e3237704441f85fa73236ec6c Mon Sep 17 00:00:00 2001 From: Tryston Perry Date: Wed, 10 Jun 2026 14:41:05 -0700 Subject: [PATCH 3/5] refactor: single source of truth for collection query assembly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit V2's queryEqual subscription stabilization duplicated collection.ts's constraint merge order (definition constraints first, then hook-level, bare collection ref when empty). The two copies had to stay in sync by hand — drift would let the comparator keep a stale listener or reintroduce the resubscribe flash, with nothing to catch it. Extract one exported buildCollectionQuery() in collection.ts and route both the live subscription and useCollection's queryEqual comparison through it. The merge order now exists in exactly one place. No behavior change. Typecheck clean; 142/142 tests pass. --- src/collection.ts | 33 ++++++++++++++++++++++++++++----- src/hooks.ts | 35 +++++++++-------------------------- 2 files changed, 37 insertions(+), 31 deletions(-) diff --git a/src/collection.ts b/src/collection.ts index dcf3ae8..eac9e5d 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -8,6 +8,7 @@ import { WithFieldValue, QueryConstraint, type CollectionReference, + type Query, } from 'firebase/firestore' import type { CollectionDefinition, @@ -34,6 +35,28 @@ import { // even when multiple instances target the same collection path. let syncKeyCounter = 0 +/** + * Build the Firestore query a collection subscription runs: `definition`-level + * constraints first, then hook-level `extraConstraints`. With no constraints at + * all the bare collection reference is itself a valid `Query`. + * + * Single source of truth for query assembly. `useCollection` decides whether a + * fresh `queryConstraints` array is semantically the same query — and so + * whether to keep the existing listener instead of tearing it down — by + * building the prospective query with this exact function and comparing via + * Firestore's `queryEqual` (see hooks.ts). That comparison is only correct if + * it assembles the query the same way the subscription does, so both paths MUST + * go through here. Don't re-inline the merge order at either call site. + */ +export const buildCollectionQuery = ( + ref: CollectionReference, + definitionConstraints: QueryConstraint[] | undefined, + extraConstraints: QueryConstraint[] | undefined +): Query => { + const all = [...(definitionConstraints ?? []), ...(extraConstraints ?? [])] + return all.length > 0 ? query(ref, ...all) : ref +} + /** * Options for creating a collection subscription */ @@ -139,8 +162,6 @@ export const createCollectionSubscription = ( `createCollectionSubscription: definition.path is a function; pass a resolved collectionPath in options.` ) } - const allConstraints = [...(definitionConstraints ?? []), ...(extraConstraints ?? [])] - // Create collection reference const collectionRef = collection(firestore, collectionPath) as CollectionReference @@ -538,9 +559,11 @@ export const createCollectionSubscription = ( loaded = false minLoadTimeElapsed = false - const q = allConstraints.length > 0 - ? query(collectionRef, ...allConstraints) - : collectionRef + const q = buildCollectionQuery( + collectionRef, + definitionConstraints, + extraConstraints + ) unsubscribeListener = onSnapshot( q, diff --git a/src/hooks.ts b/src/hooks.ts index 00a7cbd..0e53ab1 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -7,11 +7,10 @@ import { useRef, useSyncExternalStore, } from "react"; -import { collection, query, queryEqual } from "firebase/firestore"; +import { collection, queryEqual } from "firebase/firestore"; import type { CollectionReference, Firestore, - Query, QueryConstraint, } from "firebase/firestore"; import type { @@ -26,34 +25,18 @@ import type { } from "./types"; import type { FirestateStore } from "./store"; import { createDocumentSubscription } from "./document"; -import { createCollectionSubscription } from "./collection"; - -/** - * Build the Firestore query a collection subscription would run, mirroring - * the merge order in collection.ts (`definitionConstraints` then hook-level - * `extraConstraints`). With no constraints the bare collection reference is - * itself a valid `Query` for comparison purposes. - */ -const buildCollectionQuery = ( - ref: CollectionReference, - definitionConstraints: QueryConstraint[] | undefined, - extraConstraints: QueryConstraint[] | undefined -): Query => { - const all = [ - ...(definitionConstraints ?? []), - ...(extraConstraints ?? []), - ]; - return all.length > 0 ? query(ref, ...all) : ref; -}; +import { buildCollectionQuery, createCollectionSubscription } from "./collection"; /** * Whether two hook-level `queryConstraints` arrays produce the same Firestore * query for a collection. `QueryConstraint` objects are opaque, so instead of - * hand-rolling a deep compare we build both queries and defer to Firestore's - * own `queryEqual`, which structurally compares filters, ordering, limits, - * and cursors. This is what lets the subscription survive reference churn - * (e.g. constraint inputs read from a deep-cloned document) while still - * rebuilding when the query genuinely changes — no caller-supplied key. + * hand-rolling a deep compare we build both queries — with the same + * `buildCollectionQuery` the subscription itself uses, so this check can never + * drift from the query that actually runs — and defer to Firestore's own + * `queryEqual`, which structurally compares filters, ordering, limits, and + * cursors. This is what lets the subscription survive reference churn (e.g. + * constraint inputs read from a deep-cloned document) while still rebuilding + * when the query genuinely changes — no caller-supplied key. */ const queryConstraintsEqual = ( firestore: Firestore, From 2bbed1cf7d1c0ad19dd35944a71ab219dea5037b Mon Sep 17 00:00:00 2001 From: Tryston Perry Date: Mon, 15 Jun 2026 11:59:34 -0700 Subject: [PATCH 4/5] fix: guard empty in-query comparison --- README.md | 1 + docs/api-recipes.md | 8 +++++++- src/hooks.test.ts | 48 +++++++++++++++++++++++++++++++++++++++++++++ src/hooks.ts | 13 ++++++++++++ 4 files changed, 69 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 6069bab..1182c64 100644 --- a/README.md +++ b/README.md @@ -585,6 +585,7 @@ const { // memoize for correctness: const stations = useCollection({ definition: weatherStations, + enabled: stationIds.length > 0, queryConstraints: [where(documentId(), 'in', stationIds)], }) diff --git a/docs/api-recipes.md b/docs/api-recipes.md index 7dcf90f..1964285 100644 --- a/docs/api-recipes.md +++ b/docs/api-recipes.md @@ -239,7 +239,13 @@ const stationIds = project.data?.weatherSpec.nearestWeatherStationIds ?? [] const stations = useWeatherStations( {}, - { queryConstraints: [where(documentId(), 'in', stationIds)] } + { + // Firestore rejects an `in` filter with an empty array, which is what + // you get before `project.data` loads or when the source list is empty. + // Gate the subscription so the query is only built once IDs exist. + enabled: stationIds.length > 0, + queryConstraints: [where(documentId(), 'in', stationIds)], + } ) ``` diff --git a/src/hooks.test.ts b/src/hooks.test.ts index 76e733e..ac3d5bb 100644 --- a/src/hooks.test.ts +++ b/src/hooks.test.ts @@ -112,18 +112,22 @@ describe('useCollection queryConstraints identity', () => { const Probe = ({ queryConstraints, + enabled, }: { queryConstraints: QueryConstraint[] + enabled?: boolean }) => { latestHandle = useCollection({ definition: stationsCollection, queryConstraints, + enabled, }) return null } const element = (props: { queryConstraints: QueryConstraint[] + enabled?: boolean }): ReactElement => createElement( FirestateContext.Provider, @@ -209,4 +213,48 @@ describe('useCollection queryConstraints identity', () => { queryEqual(queryArg, query(ref, ...constraintsFor(newIds))) ).toBe(true) }) + + // Regression: the documented `enabled: ids.length > 0` recipe for gating an + // `in` query. While disabled the constraints array holds an empty-array + // `in` filter — a query Firestore refuses to build. The hook must not + // compare against those constraints captured while disabled when it later + // enables, or it throws "A non-empty array is required for 'in' filters" + // during render. + it('enables an in-query gated with enabled:false on empty ids without throwing', () => { + // Disabled first render: empty `in` filter, no subscription built. + act(() => { + renderer = create( + element({ + queryConstraints: constraintsFor([]), + enabled: false, + }) + ) + }) + expect(latestHandle.isActive).toBe(false) + expect(onSnapshotMock).not.toHaveBeenCalled() + + // IDs arrive: enable with a valid, non-empty `in` filter. This render + // is where the stale empty-array constraints would be built for the + // identity compare and throw. + const ids = ['ws1', 'ws2'] + act(() => { + renderer!.update( + element({ queryConstraints: constraintsFor(ids), enabled: true }) + ) + }) + act(() => { + listeners[0]!.deliver(snapshot) + vi.runAllTimers() + }) + + expect(onSnapshotMock).toHaveBeenCalledTimes(1) + expect(latestHandle.isLoading).toBe(false) + expect(latestHandle.data.ws1?.name).toBe('Station 1') + + const queryArg = onSnapshotMock.mock.calls[0]![0] + const ref = collection(firestore, 'weatherStations') + expect( + queryEqual(queryArg, query(ref, ...constraintsFor(ids))) + ).toBe(true) + }) }) diff --git a/src/hooks.ts b/src/hooks.ts index 0e53ab1..90ec224 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -442,9 +442,21 @@ export const useCollection = ( // below does not rebuild. A genuine change adopts the new reference and // re-attaches the listener. When the path is unresolved we can't build a // query, so we just pass the constraints through (the memo returns null). + // + // The comparison may only run against constraints captured during an *active* + // render (enabled with a resolved path). Constraints captured while disabled + // or unresolved can be ones the caller is gating precisely because they don't + // form a valid query yet — e.g. `where(documentId(), 'in', [])`, which + // Firestore refuses to build. Building such a stale snapshot just to compare + // would throw, so when the prior snapshot wasn't active we adopt the current + // constraints outright. There is no live listener to preserve in that case + // (the subscription was null), so adopting a fresh reference costs nothing. const stableConstraintsRef = useRef(queryConstraints); + const stableActiveRef = useRef(false); + const active = enabled && collectionPath !== undefined; if ( collectionPath === undefined || + !stableActiveRef.current || !queryConstraintsEqual( store.firestore, collectionPath, @@ -455,6 +467,7 @@ export const useCollection = ( ) { stableConstraintsRef.current = queryConstraints; } + stableActiveRef.current = active; const stableConstraints = stableConstraintsRef.current; const subscription = useMemo( From d2d3c19dd524aa7128575e2af9b095fdf8449cba Mon Sep 17 00:00:00 2001 From: Tryston Perry Date: Mon, 15 Jun 2026 13:34:23 -0700 Subject: [PATCH 5/5] fix(useCollection): guard query identity compare against unbuildable constraints queryConstraintsEqual built both the prior and incoming constraint arrays to compare them with queryEqual. For lazy collections, the hook becomes active (enabled + resolved path) before load() attaches a listener, so a render carrying a gated placeholder like where(documentId(), 'in', []) followed by real IDs would build the stale empty-array query during the identity compare and throw, crashing the render before load() ran. Wrap the build in try/catch: if the prior snapshot can't be built, no live listener could have run it, so treat the snapshots as unequal and adopt the new constraints. Defends against any unbuildable constraint shape, not just the lazy empty-in case. --- src/hooks.test.ts | 68 ++++++++++++++++++++++++++++++++++++++++++++++- src/hooks.ts | 20 +++++++++++--- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/src/hooks.test.ts b/src/hooks.test.ts index ac3d5bb..dd0a108 100644 --- a/src/hooks.test.ts +++ b/src/hooks.test.ts @@ -60,6 +60,11 @@ const stationsCollection = defineCollection({ path: 'weatherStations', }) +const lazyStationsCollection = defineCollection({ + path: 'weatherStations', + lazy: true, +}) + // A real (offline) Firestore — building queries and refs is purely // client-side; onSnapshot is mocked so nothing hits the network. let app: FirebaseApp @@ -113,12 +118,14 @@ describe('useCollection queryConstraints identity', () => { const Probe = ({ queryConstraints, enabled, + definition = stationsCollection, }: { queryConstraints: QueryConstraint[] enabled?: boolean + definition?: typeof stationsCollection }) => { latestHandle = useCollection({ - definition: stationsCollection, + definition, queryConstraints, enabled, }) @@ -128,6 +135,7 @@ describe('useCollection queryConstraints identity', () => { const element = (props: { queryConstraints: QueryConstraint[] enabled?: boolean + definition?: typeof stationsCollection }): ReactElement => createElement( FirestateContext.Provider, @@ -257,4 +265,62 @@ describe('useCollection queryConstraints identity', () => { queryEqual(queryArg, query(ref, ...constraintsFor(ids))) ).toBe(true) }) + + // Regression: a lazy collection becomes "active" (enabled + resolved path) + // as soon as it mounts, before load() attaches any listener. If the first + // render carries a gated empty-array `in` filter and real IDs arrive on a + // later render — still before load() — the identity compare would build the + // stale empty-array query and throw "A non-empty array is required for 'in' + // filters" during render. No listener exists pre-load, so there is nothing + // to preserve and the compare must not throw. + it('does not throw when lazy constraints go from empty to valid before load()', () => { + // First render: lazy + enabled with an empty `in` filter. Lazy means no + // listener is attached yet (load() not called). + act(() => { + renderer = create( + element({ + definition: lazyStationsCollection, + queryConstraints: constraintsFor([]), + enabled: true, + }) + ) + }) + expect(latestHandle.isActive).toBe(false) + expect(onSnapshotMock).not.toHaveBeenCalled() + + // IDs arrive before load(). This render is where the stale empty-array + // constraints would be built for the identity compare and throw. + const ids = ['ws1', 'ws2'] + act(() => { + renderer!.update( + element({ + definition: lazyStationsCollection, + queryConstraints: constraintsFor(ids), + enabled: true, + }) + ) + }) + + // Still no listener — lazy defers until load() is called explicitly. + expect(onSnapshotMock).not.toHaveBeenCalled() + + // load() now attaches a listener with the valid, non-empty constraints. + act(() => { + latestHandle.load() + }) + act(() => { + listeners[0]!.deliver(snapshot) + vi.runAllTimers() + }) + + expect(onSnapshotMock).toHaveBeenCalledTimes(1) + expect(latestHandle.isLoading).toBe(false) + expect(latestHandle.data.ws1?.name).toBe('Station 1') + + const queryArg = onSnapshotMock.mock.calls[0]![0] + const ref = collection(firestore, 'weatherStations') + expect( + queryEqual(queryArg, query(ref, ...constraintsFor(ids))) + ).toBe(true) + }) }) diff --git a/src/hooks.ts b/src/hooks.ts index 90ec224..ba135d6 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -37,6 +37,14 @@ import { buildCollectionQuery, createCollectionSubscription } from "./collection * cursors. This is what lets the subscription survive reference churn (e.g. * constraint inputs read from a deep-cloned document) while still rebuilding * when the query genuinely changes — no caller-supplied key. + * + * Building a query can throw: callers commonly gate with a deliberately invalid + * placeholder like `where(documentId(), 'in', [])` while real IDs are pending, + * and Firestore refuses to construct that. If building the prior snapshot + * throws, no live listener could ever have run it, so there is nothing to + * preserve — we treat the snapshots as unequal and let the caller adopt the + * new constraints. This matters most for lazy collections, where a render can + * carry such a placeholder before `load()` attaches any listener. */ const queryConstraintsEqual = ( firestore: Firestore, @@ -47,10 +55,14 @@ const queryConstraintsEqual = ( ): boolean => { if (a === b) return true; const ref = collection(firestore, collectionPath) as CollectionReference; - return queryEqual( - buildCollectionQuery(ref, definitionConstraints, a), - buildCollectionQuery(ref, definitionConstraints, b) - ); + try { + return queryEqual( + buildCollectionQuery(ref, definitionConstraints, a), + buildCollectionQuery(ref, definitionConstraints, b) + ); + } catch { + return false; + } }; /**