Repository navigation
Conversation
tyler-reitz
approved these changes
Oct 28, 2026
tyler-reitz
left a comment
Contributor
There was a problem hiding this comment.
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 showsdefault/defaultonmainanddefault/secondhere. - 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 givedefault/defaultand this PR givesdefault/third, so using two databases there would make it test theglobalThissharing. - The body says the old key was path-only and that secondary apps collided, but
mainalready includesapp.name; the real collision was named databases on one app. Also worth a line: this fixes remounting afterterminate(), which fails onmainwith "Firestore shutting down". _reactFireFirestoreDocKeyIdscould go onReactFireGlobalsinstead ofas any.
Happy to share both probes if you want them as tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Previously, Firestore document hooks (
useFirestoreDoc,useFirestoreDocOnce,useFirestoreDocData,useFirestoreDocDataOnce,preloadFirestoreDoc) keyed_reactFirePreloadedObservablesusing onlyref.path:DocumentReferences with the same path on different Firestore instances (such as a secondaryFirebaseAppor a named Firestore database) collided on the same cache key.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
refEqualarray index (likegetUniqueIdForFirestoreQuery) would avoid those collisions, but introduces three regressions for document hooks:0, 1, ...) across duplicate module instances while sharing_reactFirePreloadedObservablesonglobalThis.O(n)scan growth: Scanning every document reference ever seen on every render scales linearly with unique documents visited over the session.refEqualcompares converter function/object identity, sodoc(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}, whereidForassigns a monotonic numeric ID perFirestoreinstance via aWeakMapstored onglobalThis._reactFireFirestoreDocKeyIds:O(1)lookup with zero per-document memory retention: OnlyFirestoreinstances are WeakMap-keyed; nothing is retained per document reference._reactFireFirestoreDocKeyIdsonglobalThisensures multiple module copies assign the sameFirestoreinstance ID and never collide across different document paths.'converter'vs'raw') separates raw and converted reads without looping whenwithConverter({ ... })is constructed inline during render.Test Plan
.withConverter(...)document reads on the same path do not collide.withConverter({ ... })objects created during render resolve cleanly in both status and Suspense modes without render loops.vi.resetModules()) never share a cache entry for different documents.