Skip to content
This repository was archived by the owner on Mar 4, 2026. It is now read-only.

Fix destructuring for getAll#502

Closed
ushu wants to merge 1 commit intogoogleapis:masterfrom
ushu:fix-getAll-destructuring
Closed

Fix destructuring for getAll#502
ushu wants to merge 1 commit intogoogleapis:masterfrom
ushu:fix-getAll-destructuring

Conversation

@ushu
Copy link
Copy Markdown

@ushu ushu commented Dec 28, 2018

This is a proposal to fix #501 : it literally restores the previous array-based signature, while keeping support for ReadOptions-typed arguments.

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Dec 28, 2018
@ushu
Copy link
Copy Markdown
Author

ushu commented Dec 28, 2018

I signed the CLA docs

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 28, 2018
@schmidt-sebastian
Copy link
Copy Markdown
Contributor

schmidt-sebastian commented Dec 29, 2018

Hi @ushu! Thanks for doing this! While this should work for TypeScript users, it would be nice if we also cleaned up the SDK code itself (https://github.com/googleapis/nodejs-firestore/blob/master/dev/src/index.ts#L707 and https://github.com/googleapis/nodejs-firestore/blob/master/dev/src/transaction.ts#L161). Do you want to take a stab at that as well? Let us know if you are busy and we can take over :)

Note that the original intent of this signature was that to make it clear that the order of arguments is "DocumentReferences followed by field mask". This is probably not worth breaking our users over though... Thanks for catching this!

@mikelehen mikelehen self-assigned this Jan 2, 2019
@mikelehen
Copy link
Copy Markdown
Contributor

Superseded by #515.

@mikelehen mikelehen closed this Jan 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Typings: Cannot destructure array anymore when calling getAll

4 participants