From 15ac3cba7638e02903e3e4566e7d5a9f3de06b07 Mon Sep 17 00:00:00 2001 From: Jeff Huleatt <3759507+jhuleatt@users.noreply.github.com> Date: Tue, 27 Oct 2026 19:07:05 +0000 Subject: [PATCH 1/2] fix(firestore): key document observable cache by refEqual instead of path string --- src/firestore.tsx | 20 +++++++++++++++----- test/firestore.test.tsx | 37 +++++++++++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 7 deletions(-) diff --git a/src/firestore.tsx b/src/firestore.tsx index ed504f97..ff890d2e 100644 --- a/src/firestore.tsx +++ b/src/firestore.tsx @@ -3,7 +3,7 @@ import { ReactFireOptions, useObservable, checkIdField, ReactFireGlobals } from import { preloadObservable, ObservableStatus } from './useObservable.js'; import { first } from 'rxjs/operators'; -import { Query as FirestoreQuery, QuerySnapshot, DocumentReference, queryEqual, DocumentData, DocumentSnapshot } from 'firebase/firestore'; +import { Query as FirestoreQuery, QuerySnapshot, DocumentReference, queryEqual, refEqual, DocumentData, DocumentSnapshot } from 'firebase/firestore'; // Since we're side-effect free, we need to ensure our observableId cache is global const cachedQueries: Array> = (globalThis as any as ReactFireGlobals)._reactFireFirestoreQueryCache || []; @@ -31,7 +31,7 @@ export async function preloadFirestoreDoc(refProvider: () => Promise(ref: DocumentReference) { - return `firestore:doc:${ref.firestore.app.name}:${ref.path}`; + return `firestore:doc:${getUniqueIdForFirestoreDoc(ref)}`; } /** @@ -50,7 +50,7 @@ export function useFirestoreDoc(ref: DocumentReference, opt * Get a firestore document and don't subscribe to changes */ export function useFirestoreDocOnce(ref: DocumentReference, options?: ReactFireOptions): ObservableStatus> { - const observableId = `firestore:docOnce:${ref.firestore.app.name}:${ref.path}`; + const observableId = `firestore:docOnce:${getUniqueIdForFirestoreDoc(ref)}`; const observable$ = doc(ref).pipe(first()); return useObservable(observableId, observable$, options); @@ -62,7 +62,7 @@ export function useFirestoreDocOnce(ref: DocumentReference, export function useFirestoreDocData(ref: DocumentReference, options?: ReactFireOptions): ObservableStatus { const idField = options ? checkIdField(options) : undefined; - const observableId = `firestore:docData:${ref.firestore.app.name}:${ref.path}:idField=${JSON.stringify(idField)}`; + const observableId = `firestore:docData:${getUniqueIdForFirestoreDoc(ref)}:idField=${JSON.stringify(idField)}`; const observable = docData(ref, { idField }); return useObservable(observableId, observable, options) as ObservableStatus; @@ -74,7 +74,7 @@ export function useFirestoreDocData(ref: DocumentReference, opti export function useFirestoreDocDataOnce(ref: DocumentReference, options?: ReactFireOptions): ObservableStatus { const idField = options ? checkIdField(options) : undefined; - const observableId = `firestore:docDataOnce:${ref.firestore.app.name}:${ref.path}:idField=${JSON.stringify(idField)}`; + const observableId = `firestore:docDataOnce:${getUniqueIdForFirestoreDoc(ref)}:idField=${JSON.stringify(idField)}`; const observable$ = docData(ref, { idField }).pipe(first()); return useObservable(observableId, observable$, options) as ObservableStatus; @@ -100,3 +100,13 @@ export function useFirestoreCollectionData(query: FirestoreQue return useObservable(observableId, observable$, options); } + +const cachedDocRefs: Array> = []; + +function getUniqueIdForFirestoreDoc(ref: DocumentReference) { + const index = cachedDocRefs.findIndex((cachedRef) => refEqual(cachedRef, ref)); + if (index > -1) { + return index; + } + return cachedDocRefs.push(ref) - 1; +} diff --git a/test/firestore.test.tsx b/test/firestore.test.tsx index 0dcf0c23..7904d351 100644 --- a/test/firestore.test.tsx +++ b/test/firestore.test.tsx @@ -1,4 +1,4 @@ -import { renderHook, act, cleanup, waitFor } from '@testing-library/react'; +import { render, renderHook, act, cleanup, waitFor } from '@testing-library/react'; import * as React from 'react'; import '@testing-library/jest-dom/extend-expect'; @@ -17,7 +17,7 @@ import { baseConfig } from './appConfig'; import { randomString } from './test-utils'; import { addDoc, collection, doc, getFirestore, query, setDoc, connectFirestoreEmulator, where } from 'firebase/firestore'; -import type { DocumentReference } from 'firebase/firestore'; +import type { DocumentReference, FirestoreDataConverter } from 'firebase/firestore'; describe('Firestore', () => { const app = initializeApp(baseConfig, 'firestore-test-suite'); @@ -352,5 +352,38 @@ describe('Firestore', () => { expect(result.current.data).toEqual([mockData]); }); + + it('separates cache entries for the same document path with and without a converter', async () => { + const colRef = collection(db, randomString()); + const rawRef = doc(colRef, 'shared'); + await setDoc(rawRef, { v: 7 }); + + const converter: FirestoreDataConverter<{ converted: boolean; v: number }> = { + toFirestore: (d) => ({ v: d.v }), + fromFirestore: (snap) => ({ converted: true, v: (snap.data() as { v: number }).v }), + }; + const convertedRef = rawRef.withConverter(converter); + + const RawDoc = () => { + const { data } = useFirestoreDocData(rawRef, { suspense: true }); + return
{JSON.stringify(data)}
; + }; + const ConvertedDoc = () => { + const { data } = useFirestoreDocData(convertedRef, { suspense: true }); + return
{JSON.stringify(data)}
; + }; + + const { getByTestId } = render( + + loading}> + + + + + ); + + await waitFor(() => expect(getByTestId('conv')).toBeInTheDocument()); + expect(JSON.parse(getByTestId('conv').textContent || '{}').converted).toBe(true); + }); }); }); From ae30f2583831e49c2047ce8039c35fcc088a5245 Mon Sep 17 00:00:00 2001 From: Jeff Huleatt <3759507+jhuleatt@users.noreply.github.com> Date: Tue, 27 Oct 2026 19:56:48 +0000 Subject: [PATCH 2/2] fix(firestore): key doc cache by Firestore instance id, converter presence, and path --- src/firestore.tsx | 26 ++++++++++---- test/firestore.test.tsx | 76 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/src/firestore.tsx b/src/firestore.tsx index ff890d2e..7313cc60 100644 --- a/src/firestore.tsx +++ b/src/firestore.tsx @@ -3,7 +3,7 @@ import { ReactFireOptions, useObservable, checkIdField, ReactFireGlobals } from import { preloadObservable, ObservableStatus } from './useObservable.js'; import { first } from 'rxjs/operators'; -import { Query as FirestoreQuery, QuerySnapshot, DocumentReference, queryEqual, refEqual, DocumentData, DocumentSnapshot } from 'firebase/firestore'; +import { Query as FirestoreQuery, QuerySnapshot, DocumentReference, queryEqual, DocumentData, DocumentSnapshot } from 'firebase/firestore'; // Since we're side-effect free, we need to ensure our observableId cache is global const cachedQueries: Array> = (globalThis as any as ReactFireGlobals)._reactFireFirestoreQueryCache || []; @@ -101,12 +101,24 @@ export function useFirestoreCollectionData(query: FirestoreQue return useObservable(observableId, observable$, options); } -const cachedDocRefs: Array> = []; +// Small ids for Firestore instances (one per app and database), so refs to the same path in another database get +// their own cache entry without scanning every ref seen so far. Kept on globalThis, like the query cache, so duplicate +// copies of ReactFire hand out the same ids. Converters are keyed by presence, not identity: a converter object +// created during render would otherwise change the key on every render. +const docKeyIds: { ids: WeakMap; next: number } = ((globalThis as any)._reactFireFirestoreDocKeyIds ??= { + ids: new WeakMap(), + next: 0, +}); + +function idFor(target: object): number { + let id = docKeyIds.ids.get(target); + if (id === undefined) { + id = docKeyIds.next++; + docKeyIds.ids.set(target, id); + } + return id; +} function getUniqueIdForFirestoreDoc(ref: DocumentReference) { - const index = cachedDocRefs.findIndex((cachedRef) => refEqual(cachedRef, ref)); - if (index > -1) { - return index; - } - return cachedDocRefs.push(ref) - 1; + return `${idFor(ref.firestore)}:${ref.converter ? 'converter' : 'raw'}:${ref.path}`; } diff --git a/test/firestore.test.tsx b/test/firestore.test.tsx index 7904d351..ae9e1a2e 100644 --- a/test/firestore.test.tsx +++ b/test/firestore.test.tsx @@ -385,5 +385,81 @@ describe('Firestore', () => { await waitFor(() => expect(getByTestId('conv')).toBeInTheDocument()); expect(JSON.parse(getByTestId('conv').textContent || '{}').converted).toBe(true); }); + + it('resolves when a converter object is created inline during render', async () => { + const colRef = collection(db, randomString()); + await setDoc(doc(colRef, 'inline'), { name: 'alice' }); + + const makeConverter = (): FirestoreDataConverter<{ name: string }> => ({ + toFirestore: (p) => p, + fromFirestore: (snap) => ({ name: String(snap.get('name')) }), + }); + + let statusRenders = 0; + const StatusProfile = () => { + statusRenders++; + const { status, data } = useFirestoreDocData(doc(colRef, 'inline').withConverter(makeConverter())); + return
{status === 'success' ? data?.name : 'loading'}
; + }; + + let suspenseRenders = 0; + const SuspenseProfile = () => { + suspenseRenders++; + const { data } = useFirestoreDocData(doc(colRef, 'inline').withConverter(makeConverter()), { suspense: true }); + return
{data?.name}
; + }; + + const { getByTestId } = render( + + + fallback}> + + + + ); + + await waitFor(() => expect(getByTestId('status-out')).toHaveTextContent('alice')); + await waitFor(() => expect(getByTestId('suspense-out')).toHaveTextContent('alice')); + expect(statusRenders).toBeLessThan(10); + expect(suspenseRenders).toBeLessThan(10); + }); + + it('never shares a cache entry for different documents across two module instances', async () => { + const colRef = collection(db, randomString()); + const x = doc(colRef, 'x'); + const y = doc(colRef, 'y'); + await setDoc(x, { v: 'x' }); + await setDoc(y, { v: 'y' }); + + const copyA = await import('../src/index'); + vi.resetModules(); + const copyB = await import('../src/index'); + expect(copyA.useFirestoreDocData).not.toBe(copyB.useFirestoreDocData); + + const A = () => { + const { data } = copyA.useFirestoreDocData(x); + return {String((data as { v?: string })?.v)}; + }; + const B = () => { + const { data } = copyB.useFirestoreDocData(y); + return {String((data as { v?: string })?.v)}; + }; + + const { getByTestId } = render( + + + + + + + + + + + ); + + await waitFor(() => expect(getByTestId('dual-a')).toHaveTextContent('x')); + await waitFor(() => expect(getByTestId('dual-b')).toHaveTextContent('y')); + }); }); });