diff --git a/AGENTS.md b/AGENTS.md index 44e8b64..6396d52 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,8 +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. +- `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 307d193..1182c64 100644 --- a/README.md +++ b/README.md @@ -577,6 +577,18 @@ const { enabled: true, // Optional: set false until required params exist }) +// 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, + enabled: stationIds.length > 0, + queryConstraints: [where(documentId(), 'in', stationIds)], +}) + // Update existing documents update({ space1: { name: 'Updated Name' } }) diff --git a/docs/api-recipes.md b/docs/api-recipes.md index 24d3205..1964285 100644 --- a/docs/api-recipes.md +++ b/docs/api-recipes.md @@ -214,6 +214,45 @@ const queryConstraints = useMemo( const spaces = useSpaces({ projectId }, { queryConstraints }) ``` +### Dynamic queries built from document data + +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. + +`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' + +// 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 stations = useWeatherStations( + {}, + { + // 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)], + } +) +``` + +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 Undo is enabled by default. diff --git a/docs/architecture.md b/docs/architecture.md index e11854b..6c32ce9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -113,7 +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. +- `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/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/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.test.ts b/src/hooks.test.ts new file mode 100644 index 0000000..dd0a108 --- /dev/null +++ b/src/hooks.test.ts @@ -0,0 +1,326 @@ +/** + * 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. + * + * 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' + +vi.mock('firebase/firestore', async () => { + const actual = + await vi.importActual( + 'firebase/firestore' + ) + return { + ...actual, + 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 { + 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' +import type { CollectionHandle, FirestoreObject } from './types' + +interface Station extends FirestoreObject { + name: string +} + +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 +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 }> +} + +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 = onSnapshot as unknown as ReturnType + + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + store = createStore({ firestore }) + 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, + enabled, + definition = stationsCollection, + }: { + queryConstraints: QueryConstraint[] + enabled?: boolean + definition?: typeof stationsCollection + }) => { + latestHandle = useCollection({ + definition, + queryConstraints, + enabled, + }) + return null + } + + const element = (props: { + queryConstraints: QueryConstraint[] + enabled?: boolean + definition?: typeof stationsCollection + }): 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[] }) => { + 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('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 (semantically equal, no key needed)', () => { + const ids = ['ws1', 'ws2'] + 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]) }) + ) + }) + + // 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 the query actually changes', () => { + const ids = ['ws1', 'ws2'] + mountAndLoad({ queryConstraints: constraintsFor(ids) }) + expect(onSnapshotMock).toHaveBeenCalledTimes(1) + + const newIds = ['ws1', 'ws3'] + act(() => { + renderer!.update( + 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 queryArg = onSnapshotMock.mock.calls[1]![0] + const ref = collection(firestore, 'weatherStations') + expect( + 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) + }) + + // 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 9d86e76..ba135d6 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -7,7 +7,12 @@ import { useRef, useSyncExternalStore, } from "react"; -import type { QueryConstraint } from "firebase/firestore"; +import { collection, queryEqual } from "firebase/firestore"; +import type { + CollectionReference, + Firestore, + QueryConstraint, +} from "firebase/firestore"; import type { CollectionDefinition, CollectionHandle, @@ -20,7 +25,45 @@ import type { } from "./types"; import type { FirestateStore } from "./store"; import { createDocumentSubscription } from "./document"; -import { createCollectionSubscription } from "./collection"; +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 — 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. + * + * 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, + collectionPath: string, + definitionConstraints: QueryConstraint[] | undefined, + a: QueryConstraint[] | undefined, + b: QueryConstraint[] | undefined +): boolean => { + if (a === b) return true; + const ref = collection(firestore, collectionPath) as CollectionReference; + try { + return queryEqual( + buildCollectionQuery(ref, definitionConstraints, a), + buildCollectionQuery(ref, definitionConstraints, b) + ); + } catch { + return false; + } +}; /** * Returned when a hook is called with `enabled: false`. Module-level constants @@ -304,13 +347,29 @@ 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 *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. + * **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)], + * }) + * ``` + * + * 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). @@ -386,6 +445,43 @@ export const useCollection = ( : definition.path : undefined; + // 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). + // + // 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, + definition.queryConstraints, + stableConstraintsRef.current, + queryConstraints + ) + ) { + stableConstraintsRef.current = queryConstraints; + } + stableActiveRef.current = active; + const stableConstraints = stableConstraintsRef.current; + const subscription = useMemo( () => enabled && collectionPath !== undefined @@ -394,7 +490,7 @@ export const useCollection = ( definition, collectionPath, readOnly, - queryConstraints, + queryConstraints: stableConstraints, onPushUndo, }) : null, @@ -404,7 +500,7 @@ export const useCollection = ( definition, collectionPath, readOnly, - queryConstraints, + stableConstraints, onPushUndo, ] );