-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(firestore): support for aggregate queries including sum()
& average()
#8115
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
...ative/java/io/invertase/firebase/firestore/ReactNativeFirebaseFirestoreCollectionModule.java
Outdated
Show resolved
Hide resolved
...ative/java/io/invertase/firebase/firestore/ReactNativeFirebaseFirestoreCollectionModule.java
Outdated
Show resolved
Hide resolved
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.
exciting! couldn't but look through it even though draft+android-only+no-e2e at the moment because I contemplated implementing this myself months ago :-)
Had a couple quick thoughts but mostly just wanted to say this is exciting. Cheers
packages/firestore/ios/RNFBFirestore/RNFBFirestoreCollectionModule.m
Outdated
Show resolved
Hide resolved
average()
sum()
& average()
having native module exceptions vs promise rejects requires JS level code to handle multiple types of error vs being able to use one style
packages/firestore/ios/RNFBFirestore/RNFBFirestoreCollectionModule.m
Outdated
Show resolved
Hide resolved
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.
has one code error from just reading in iOS, noted with suggestion to add return after executing reject block- the number handling probe hit paydirt! which is kind of awful but also represents progress (known error better than unknown error...)
some new android issue ?
It really does look like progress tho, just this one is a real feature size-wise so has real subtleties
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.
awesome!
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.
still LGTM - just looked at the last couple test additions (nice)
Hi! I upgraded to 21.5.0 today because I was super happy for the support for sum(). But I think there might be an issue with typescript... I made some screenshots. Maybe I am doing something wrong, but the docs aren't updated yet, so I'm not sure how to help... Thank you! If I can help in any way please let me know :) |
Description
Related issues
closes #8027
Release Summary
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
Test Plan
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter