-
Notifications
You must be signed in to change notification settings - Fork 148
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
Overload getAll function to allow array destructuring #515
Conversation
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.
Thanks very much for digging in and taking a stab at overloads (and adding tests, etc.!) but looking closer I think overloads don't actually help here. So do you mind going dropping the "overload" and just using your more permissive function signature like you did in the original PR? Thanks!
@mikelehen I have just pushed an update to the PR that uses a tuple type along with the rest element. What this means is that the first parameter is required and must be of type |
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.
Thanks! Left a couple more comments. Sorry for so much back-forth.
No worries! Just updated the PR with single signature. This should hopefully maintain compatibility albeit being overly permissive. |
@schmidt-sebastian JSDoc updated in latest commit. I have also fixed the failing test for Also, I noticed that |
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.
Thanks! I think this looks good to me now. 😁
@JustinBeckwith @crwilcox Either of you know if there's a trick to making the CI run (for an externally contributed PR)? |
@mikelehen I think last time this worked when @kinwa91 assigned the |
Anyone with |
@cxam @kinwa91 Nice, thanks! |
Fixes #501
This PR overloads the getAll function in Firestore and Transaction class to allow passing in a destructured array of documents.