Skip to content

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

Open
jhuleatt wants to merge 2 commits into
mainfrom
fix/firestore-ref-equal-cache
Open

jhuleatt wants to merge 2 commits into
mainfrom
fix/firestore-ref-equal-cache

Conversation

@jhuleatt

@jhuleatt jhuleatt commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Previously, Firestore document hooks (useFirestoreDoc, useFirestoreDocOnce, useFirestoreDocData, useFirestoreDocDataOnce, preloadFirestoreDoc) keyed _reactFirePreloadedObservables using only ref.path:

  • Two DocumentReferences with the same path on different Firestore instances (such as a secondary FirebaseApp or a named Firestore database) collided on the same cache key.
  • Two DocumentReferences to the same path where one attached .withConverter(...) and the other did not collided on the same cache key, returning unconverted data to the converted hook (or vice versa).

Using a refEqual array index (like getUniqueIdForFirestoreQuery) would avoid those collisions, but introduces three regressions for document hooks:

  1. Cross-instance collision when multiple copies of ReactFire are loaded: A module-local index array hands out overlapping integer indices (0, 1, ...) across duplicate module instances while sharing _reactFirePreloadedObservables on globalThis.
  2. O(n) scan growth: Scanning every document reference ever seen on every render scales linearly with unique documents visited over the session.
  3. Inline converter render loop: refEqual compares converter function/object identity, so doc(db, ...).withConverter({ ... }) written inline during render produces a new cache key on every render and never leaves loading/Suspense fallback.

Fix

Key Firestore document observables by ${idFor(ref.firestore)}:${ref.converter ? 'converter' : 'raw'}:${ref.path}, where idFor assigns a monotonic numeric ID per Firestore instance via a WeakMap stored on globalThis._reactFireFirestoreDocKeyIds:

  • O(1) lookup with zero per-document memory retention: Only Firestore instances are WeakMap-keyed; nothing is retained per document reference.
  • Safe across duplicate module instances: Storing _reactFireFirestoreDocKeyIds on globalThis ensures multiple module copies assign the same Firestore instance ID and never collide across different document paths.
  • Inline converters work as in v4.2.6: Keying by converter presence ('converter' vs 'raw') separates raw and converted reads without looping when withConverter({ ... }) is constructed inline during render.
  • Known limitation (same as v4.2.6): Two different converters attached to the same document path on the same Firestore instance at the same time still share a cache entry.

Test Plan

  • Added regression test verifying raw and .withConverter(...) document reads on the same path do not collide.
  • Added regression test verifying inline withConverter({ ... }) objects created during render resolve cleanly in both status and Suspense modes without render loops.
  • Added regression test verifying two module instances (vi.resetModules()) never share a cache entry for different documents.

@jhuleatt
jhuleatt requested a review from tyler-reitz October 27, 2026 19:07
@jhuleatt jhuleatt changed the title fix(firestore): key document observable cache by refEqual instead of path string fix(firestore): key document cache by Firestore instance, converter presence, and path Oct 27, 2026

@tyler-reitz tyler-reitz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The raw-vs-converter and inline-converter tests each go red when their part of the key is reverted, and keying by presence is the right trade: identity loops on inline converters, and main ignores converters entirely.

Non-blocking:

  • The instance id is untested: replacing it with a constant keeps all 18 green. A named-database test would cover it; on the emulator, getFirestore(app, 'second-db') with the same path shows default/default on main and default/second here.
  • The two-copies test passes with the id table module-local, and on main's source, because both copies see one database. With two databases on the same path, module-local ids give default/default and this PR gives default/third, so using two databases there would make it test the globalThis sharing.
  • The body says the old key was path-only and that secondary apps collided, but main already includes app.name; the real collision was named databases on one app. Also worth a line: this fixes remounting after terminate(), which fails on main with "Firestore shutting down".
  • _reactFireFirestoreDocKeyIds could go on ReactFireGlobals instead of as any.

Happy to share both probes if you want them as tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants