fix(firestore): key document cache by Firestore instance, converter presence, and path (#840)

The document hooks keyed their cache by app name and path, so two databases on one app, or the same path read with and without a converter, shared an entry and returned each other's data.

The key is now a per-instance id, converter presence, and the path. Instance ids come from a WeakMap on globalThis, so duplicate copies of ReactFire agree on them. Converters are keyed by presence rather than identity, so an inline withConverter({...}) created during render does not loop. Two different converters on the same path still share an entry, as before.

Tests cover raw vs converter, inline converters, named databases, and two module copies reading two databases.

Author: jhuleatt

src/firestore.tsx

@@ -31,7 +31,7 @@ export async function preloadFirestoreDoc(refProvider: () => Promise<DocumentRef
 }
 
 function getDocObservableId<T = DocumentData>(ref: DocumentReference<T>) {
-  return `firestore:doc:${ref.firestore.app.name}:${ref.path}`;
+  return `firestore:doc:${getUniqueIdForFirestoreDoc(ref)}`;
 }
 
 /**
@@ -50,7 +50,7 @@ export function useFirestoreDoc<T = DocumentData>(ref: DocumentReference<T>, opt
  * Get a firestore document and don't subscribe to changes
  */
 export function useFirestoreDocOnce<T = DocumentData>(ref: DocumentReference<T>, options?: ReactFireOptions<T>): ObservableStatus<DocumentSnapshot<T>> {
-  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<T = DocumentData>(ref: DocumentReference<T>,
 export function useFirestoreDocData<T = unknown>(ref: DocumentReference<T>, options?: ReactFireOptions<T>): ObservableStatus<T | undefined> {
   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<T>;
@@ -74,7 +74,7 @@ export function useFirestoreDocData<T = unknown>(ref: DocumentReference<T>, opti
 export function useFirestoreDocDataOnce<T = unknown>(ref: DocumentReference<T>, options?: ReactFireOptions<T>): ObservableStatus<T | undefined> {
   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<T>;
@@ -100,3 +100,25 @@ export function useFirestoreCollectionData<T = DocumentData>(query: FirestoreQue
 
   return useObservable(observableId, observable$, options);
 }
+
+// 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<object, number>; 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<T = DocumentData>(ref: DocumentReference<T>) {
+  return `${idFor(ref.firestore)}:${ref.converter ? 'converter' : 'raw'}:${ref.path}`;
+}

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,12 +17,16 @@ 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');
   const db = getFirestore(app);
   connectFirestoreEmulator(db, 'localhost', 8085);
+  const secondDb = getFirestore(app, 'second-db');
+  connectFirestoreEmulator(secondDb, 'localhost', 8085);
+  const thirdDb = getFirestore(app, 'third-db');
+  connectFirestoreEmulator(thirdDb, 'localhost', 8085);
 
   const Provider = ({ children }: { children: React.ReactNode }) => (
     <FirebaseAppProvider firebaseApp={app}>
@@ -51,7 +55,9 @@ describe('Firestore', () => {
 
     // clear all Firestore emulator data
     // do this AFTER cleaning up hooks, otherwise they'll re-emit values
-    await fetch(`http://localhost:8085/emulator/v1/projects/rxfire-525a3/databases/(default)/documents`, { method: 'DELETE' });
+    for (const database of ['(default)', 'second-db', 'third-db']) {
+      await fetch(`http://localhost:8085/emulator/v1/projects/rxfire-525a3/databases/${database}/documents`, { method: 'DELETE' });
+    }
   });
 
   test('double check - emulator is running', async () => {
@@ -352,5 +358,173 @@ 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 <div data-testid="raw">{JSON.stringify(data)}</div>;
+      };
+      const ConvertedDoc = () => {
+        const { data } = useFirestoreDocData(convertedRef, { suspense: true });
+        return <div data-testid="conv">{JSON.stringify(data)}</div>;
+      };
+
+      const { getByTestId } = render(
+        <Provider>
+          <React.Suspense fallback={<div data-testid="fb">loading</div>}>
+            <RawDoc />
+            <ConvertedDoc />
+          </React.Suspense>
+        </Provider>
+      );
+
+      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 <div data-testid="status-out">{status === 'success' ? data?.name : 'loading'}</div>;
+      };
+
+      let suspenseRenders = 0;
+      const SuspenseProfile = () => {
+        suspenseRenders++;
+        const { data } = useFirestoreDocData(doc(colRef, 'inline').withConverter(makeConverter()), { suspense: true });
+        return <div data-testid="suspense-out">{data?.name}</div>;
+      };
+
+      const { getByTestId } = render(
+        <Provider>
+          <StatusProfile />
+          <React.Suspense fallback={<div data-testid="suspense-fb">fallback</div>}>
+            <SuspenseProfile />
+          </React.Suspense>
+        </Provider>
+      );
+
+      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 <span data-testid="dual-a">{String((data as { v?: string })?.v)}</span>;
+      };
+      const B = () => {
+        const { data } = copyB.useFirestoreDocData(y);
+        return <span data-testid="dual-b">{String((data as { v?: string })?.v)}</span>;
+      };
+
+      const { getByTestId } = render(
+        <copyA.FirebaseAppProvider firebaseApp={app}>
+          <copyA.FirestoreProvider sdk={db}>
+            <A />
+          </copyA.FirestoreProvider>
+          <copyB.FirebaseAppProvider firebaseApp={app}>
+            <copyB.FirestoreProvider sdk={db}>
+              <B />
+            </copyB.FirestoreProvider>
+          </copyB.FirebaseAppProvider>
+        </copyA.FirebaseAppProvider>
+      );
+
+      await waitFor(() => expect(getByTestId('dual-a')).toHaveTextContent('x'));
+      await waitFor(() => expect(getByTestId('dual-b')).toHaveTextContent('y'));
+    });
+
+    it('separates cache entries for the same document path in two databases on one app', async () => {
+      const path = `${randomString()}/shared`;
+      await setDoc(doc(db, path), { v: 'default' });
+      await setDoc(doc(secondDb, path), { v: 'second' });
+
+      const DefaultDoc = () => {
+        const { data } = useFirestoreDocData(doc(db, path));
+        return <span data-testid="db-default">{String((data as { v?: string })?.v)}</span>;
+      };
+      const SecondDoc = () => {
+        const { data } = useFirestoreDocData(doc(secondDb, path));
+        return <span data-testid="db-second">{String((data as { v?: string })?.v)}</span>;
+      };
+
+      const { getByTestId } = render(
+        <Provider>
+          <DefaultDoc />
+          <SecondDoc />
+        </Provider>
+      );
+
+      await waitFor(() => expect(getByTestId('db-default')).toHaveTextContent('default'));
+      await waitFor(() => expect(getByTestId('db-second')).toHaveTextContent('second'));
+    });
+
+    it('separates cache entries for the same path in two databases across two module instances', async () => {
+      // Both copies are freshly imported, so if the id table were module-local each would give its first database
+      // id 0 and the two keys would collide.
+      const path = `${randomString()}/shared`;
+      await setDoc(doc(db, path), { v: 'default' });
+      await setDoc(doc(thirdDb, path), { v: 'third' });
+
+      vi.resetModules();
+      const copyA = await import('../src/index');
+      vi.resetModules();
+      const copyB = await import('../src/index');
+
+      const A = () => {
+        const { data } = copyA.useFirestoreDocData(doc(db, path));
+        return <span data-testid="copies-default">{String((data as { v?: string })?.v)}</span>;
+      };
+      const B = () => {
+        const { data } = copyB.useFirestoreDocData(doc(thirdDb, path));
+        return <span data-testid="copies-third">{String((data as { v?: string })?.v)}</span>;
+      };
+
+      const { getByTestId } = render(
+        <copyA.FirebaseAppProvider firebaseApp={app}>
+          <A />
+          <copyB.FirebaseAppProvider firebaseApp={app}>
+            <B />
+          </copyB.FirebaseAppProvider>
+        </copyA.FirebaseAppProvider>
+      );
+
+      await waitFor(() => expect(getByTestId('copies-default')).toHaveTextContent('default'));
+      await waitFor(() => expect(getByTestId('copies-third')).toHaveTextContent('third'));
+    });
   });
 });