-
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): Firestore Filter
instance. Use Filter
, Filter.or()
& Filter.and()
in Firestore queries.
#7045
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
Filter
instance. Use Filter
, Filter.or()
& Filter.and()
in Firestore queries.
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 was about to ask "where's the modular API?" then remembered firestore doesn't have it yet :-). Will commit the example.com changes and otherwise LGTM, this is an amazing feature
I did see some formatting moving on the java side and I think we've had false-negatives on google-java-format before so I'll check that locally and push a spacing commit as needed, but otherwise good to go
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.
Oh wait - what about types? Should there be a typescript types update in https://github.com/invertase/react-native-firebase/blob/main/packages/firestore/lib/index.d.ts ? Seems like there has to be or this won't be usable in typescript projects
Yep as suspected there was a false-negative on java formatting, I ran Only pending issue for me is typescript types question now |
packages/firestore/lib/index.d.ts
Outdated
* The Filter.and() static function used to generate a logical AND query using multiple Filter instances. | ||
* e.g. Filter.and(Filter('name', '==', 'Ada'), Filter('name', '==', 'Bob')) | ||
*/ | ||
and(...queries: Filter[]): Filter; |
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.
🤔 these types don't seem like a fit vs the firebase-js-sdk types, is that intentional?
Here's the and()
from firebase-js-sdk https://firebase.google.com/docs/reference/js/firestore_.md#and
Has all sorts of QueryFilterCompositeConstraint stuff, takes QueryFilterConstraint[] etc types that aren't present here
Not sure how this will work as a drop-in replacement
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've updated types to match naming: 20e9c8d. Let me know if this works.
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.
The types seem much more compatible now yes - I think code written for firebase-js-sdk will interop here without changes in general. The types aren't 100% but at the same time there is no SLA on compound or queries yet, so if types break a little it isn't a breaking change here and people can tell us in practice where there are problems as folks attempt to use it - so +1
Separately, as a feature (beyond nitpicking on types), this is awesome. I know lots of people will be excited
Great work! 👏 When can we expect this release? 👍 |
…r()` & `Filter.and()` in Firestore queries. (#7045)
@russellwheatley @mikehardy Any reason for the 10 query operators limit? I don't see any reason for that check to exist, if we hit any limit firebase api's should be the ones complaining right? |
@filipef101 in general we try to enforce things limitations that are clearly described in the documents before attempting to call the cloud APIs. However in this case I believe the limits have changed from a strict "10" to a new kind-of-vague "no more than 30 disjunctions based on the query's disjunctive normal form" https://cloud.google.com/firestore/docs/query-data/queries#query_limitations It may make sense for you open an issue related to this, quoting the new limitations and lobbying for a removal of local validation here, to be replaced with server validation / error handling. Before you do that, can you verify what exactly will happen (via direct manipulation of the code in node_modules to allow things through...) if you send two examples to probe the behavior:
|
Description
Latest
Filter
feature for RNFB. Integrated into the existing implementation. This allows chaining even with existing query capability:It is largely based on recursion.
This PR also preserves the query order of chained filters that the user generates.
We still send the same list of filters over to the native side except the list can have objects that are nested Filter queries:
Related issues
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