-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(firestore): add support for local source snapshot listeners #8929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
astanb
wants to merge
7
commits into
invertase:main
Choose a base branch
from
astanb:add-source-snapshot-listener-option
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
a25118a
feat(firestore): add 'source' option to enable local cache firestore …
astanb 766b7d5
chore: formatting
astanb 7412ab1
refactor(firestore): create ListenSource type to maintain parity with…
astanb 3079d53
test(firestore): added out-of-band helper for writing to firestore wi…
astanb ffaeb63
test(firestore): changed e2e tests to test local cache listener properly
astanb afa4465
chore: lint fixes
astanb 6295a5c
fix: add ListenerSource type instead of literal union
astanb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.