feat(firestore): add support for local source snapshot listeners#8929
feat(firestore): add support for local source snapshot listeners#8929astanb wants to merge 7 commits intoinvertase:mainfrom
Conversation
|
@astanb is attempting to deploy a commit to the Invertase Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
Adds support for configuring Firestore snapshot listeners to listen from a specific source ('cache' or 'default'), aligning React Native Firebase behavior with newer Firestore SDK capabilities and addressing #8126.
Changes:
- Extend snapshot listener option parsing + validation to accept
source: 'default' | 'cache'. - Plumb
sourcethrough to native snapshot listener registrations on iOS and Android. - Update TS types and add/extend Jest + e2e coverage for the new option (including invalid values).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/firestore/lib/utils/index.ts | Parses SnapshotListenOptions.source, defaults it, and validates allowed values. |
| packages/firestore/lib/types/firestore.ts | Adds source?: 'default' | 'cache' to modular SnapshotListenOptions typing. |
| packages/firestore/lib/types/namespaced.ts | Updates namespaced docs/types for listener options (adds source, makes includeMetadataChanges optional). |
| packages/firestore/lib/types/internal.ts | Extends internal listener options type to include source. |
| packages/firestore/lib/FirestoreDocumentReference.ts | Passes parsed source option through document onSnapshot to native. |
| packages/firestore/lib/FirestoreQuery.ts | Passes parsed source option through query onSnapshot to native (incl. named queries). |
| packages/firestore/ios/RNFBFirestore/RNFBFirestoreDocumentModule.{h,m} | Uses FIRSnapshotListenOptions with source + includeMetadataChanges for document listeners. |
| packages/firestore/ios/RNFBFirestore/RNFBFirestoreCollectionModule.{h,m} | Uses FIRSnapshotListenOptions with source + includeMetadataChanges for query listeners. |
| packages/firestore/android/.../ReactNativeFirebaseFirestoreDocumentModule.java | Uses SnapshotListenOptions.Builder to set ListenSource + metadata changes for document listeners. |
| packages/firestore/android/.../ReactNativeFirebaseFirestoreCollectionModule.java | Uses SnapshotListenOptions.Builder to set ListenSource + metadata changes for query listeners. |
| packages/firestore/e2e/DocumentReference/onSnapshot.e2e.js | Adds e2e coverage for invalid source and cache-source document listeners. |
| packages/firestore/e2e/Query/onSnapshot.e2e.js | Adds e2e coverage for invalid source and cache-source query listeners. |
| packages/firestore/tests/firestore.test.ts | Adds unit tests for parseSnapshotArgs handling of source. |
| packages/firestore/type-test.ts | Updates type tests to exercise source: 'cache' on snapshot listeners. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await docRef.set({ enabled: true }); | ||
| await docRef.get(); | ||
|
|
||
| let unsub = () => {}; | ||
| try { | ||
| await firebase.firestore().disableNetwork(); | ||
| const callback = sinon.spy(); | ||
| unsub = docRef.onSnapshot({ source: 'cache' }, callback); | ||
| await Utils.spyToBeCalledOnceAsync(callback); | ||
| callback.args[0][0].metadata.fromCache.should.equal(true); |
There was a problem hiding this comment.
I love the added test - thank you - I wonder if this truly probes it though - I think it would be false-positive in that ListenSource.default and ListenSource.cache would have the same behavior in this scenario wouldn't they (since default listens to server and cache) ? So this doesn't really differentiate if the implementation is correct
The only thing I can think of that would truly differentiate is to have a function outside of the test app update firestore (so it didn't go in the test app local cache during set) and then have two listeners with the two different sources, do the out-of-band server set and make sure server listener was called but cache was not. Then do an in-band set and make sure they were both called
Cloud function to run in emulator could be added in to this area https://github.com/invertase/react-native-firebase/blob/main/.github/workflows/scripts/functions/src/index.ts ) where you told it a document path to update and a key/value pair perhaps (or anything that worked) via a post from the test app, and post from the test app could look like the auth ones do https://github.com/invertase/react-native-firebase/blob/main/packages/auth/e2e/helpers.js
A bit heavy but it is the only idea I have to really differentiate between the two source options and prove they work as defined vs just appear to function without error
Thoughts?
There was a problem hiding this comment.
Good point, the existing test didn't really prove anything!
Instead of going the callable function route I added an out-of-band helper function that uses direct API access to firestore, adds a little boilerplate but avoids the dependency on functions and could potentially be re-used in the future. Let me know if you'd prefer me to go the callable function route and I can do that.
mikehardy
left a comment
There was a problem hiding this comment.
The implementation here looks really solid at the native level
The typescript types have a request for more strict coherence with upstream as mentioned in comments
The test support in types and unit look great, e2e I have a false-positive worry but solving it looks annoyingly "heavy", there is prior art to copy though so is perhaps not a large task?
This looks like just a little polish away from being merge-able though and I imagine it is already working in practice, fantastic, thank you
|
Java failed lint and objective-c may as well for java: We're strict on formatting so it doesn't turn into an opinion battle but the fixes are all automated at least so shouldn't require thought except how to run them |
mikehardy
left a comment
There was a problem hiding this comment.
that e2e test is exactly what I was hoping for, fantastic
It is done IMHO to just the last places to thread the ListenSource type change through then the firestore package here has a great new feature
Description
Related issues
Release Summary
Adds the ability to specify the source ('cache'/'default') for snapshot listeners.
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
New tests added for functionality.
All tests for both platforms passing.
🔥