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.
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
fix(types)!: properly format listing methods with gaxOptions #925
fix(types)!: properly format listing methods with gaxOptions #925
Changes from all commits
9f997cb
4ff9eed
80c3e70
61fde11
ae02a1d
427454e
a7359cf
1f69818
47c1e59
01aa00d
4ffe38e
e849076
956e7e6
58133d1
86b70c0
393e5da
67ce623
2fc13bf
9c4fa0e
6837b0c
dc7a6b0
77b8297
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Since we're in a breaking change, let's not even worry about supporting this improper usage. Most people have probably been using it in
reqOpts
already anyway.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 completely agree with this but I think we have no choice but to do this while
CallOptions
still haspageSize
andpageToken
parameters (see my reply here). Someone is bound to set it here instead of inreqOpts
.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.
That's a good point! I sent a PR to gax to remove those options. After that, we can re-generate the docs, and remove them. I would be surprised if we had any users putting those options in there. Already, turning off auto-pagination and setting
pageSize
andpageToken
manually is rare. Setting options through gax (a very unfamiliar concept for a user) would be even more unlikely.I would be okay with still dropping our backup support while we wait for the changes from the linked PR to take effect. Otherwise, I think waiting is worth it, to avoid the complexity of support. Of course, we can always come back and remove the code later, so whatever everyone thinks is best :)
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.
@stephenplusplus do you know how long it would take to get the gax PR merged and into a release?
The main concern I have is that this PR needs to go into v5.0.0. We need v5.0.0 to go out asap since it's got the backup & restore support which is already a GA'ed feature but is currently not supported by a Node.js release.
So if we think the gax change will take more than a week to get through, then we should merge this as is then remove the code 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'm not sure how long that would take. Hopefully @alexander-fenster can take a look at that PR (googleapis/gax-nodejs#821) when he's available and give us a better idea.
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.
We just made a semver major for gax a few weeks ago. From one hand, major releases are cheap, but from another hand, having too many majors makes it harder to hot fix security issues so I would better wait for some time before making gax v3.0.0. So I would probably suggest you go to v5.0.0 here as is. What do you think @skuruppu @stephenplusplus?
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 @alexander-fenster. It's okay with me to wait. The temporary code won't be too bad. Alternatively, I still think we're safe to remove support for the options anyway, and if anybody in the world notices, we can release a hotfix. But that's completely your call @skuruppu.
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 agree that it's so very unlikely that anyone would be trying to set these options through CallOptions. But just in case, we'll leave the temporary code. It won't be too hard to remove it when gax v3.0.0 is ready. I'm open to doing another major release whenever we need to.