feat(diregapic)(pagination): support map handle for DIREGAPIC Pagination#1052
feat(diregapic)(pagination): support map handle for DIREGAPIC Pagination#1052alexander-fenster merged 4 commits intomasterfrom
Conversation
| // For pagination response with protobuf map type, use tuple as representation. | ||
| if (result && !Array.isArray(result)) { | ||
| for (const [key, value] of Object.entries(result)) { | ||
| cache.push([key, value]); |
There was a problem hiding this comment.
I am not sure if we need to define a type for this tuple. @alexander-fenster what do you suggest the type here? My guess here is that we could define a MapResponseTuple
type MapResponseTuple = [String, ??(not sure)]
There was a problem hiding this comment.
It would be [string, ResponseType], and the cache should be defined as ResponseType | [string, ResponseType].
There was a problem hiding this comment.
(I think having just a ResponseType which is basically any object is OK)
9f3c271 to
b1c6128
Compare
b1c6128 to
2a2c7b7
Compare
alexander-fenster
left a comment
There was a problem hiding this comment.
LGTM with comments
vam-google
left a comment
There was a problem hiding this comment.
I'll let Alex to comment on the type-script fine grain details, but overall the change looks good to me! look simple & robust and gets the job done.
| [Symbol.asyncIterator]() { | ||
| let nextPageRequest: RequestType | null | undefined = request; | ||
| const cache: {}[] = []; | ||
| const cache: Array<ResponseType | [string, ResponseType]> = []; |
There was a problem hiding this comment.
Another option
const cache: (ResponseType | [string, ResponseType]) = [],
Base on my search Array<T> vs T[] are quite similar, [] is shorthand way, I choose Array<T> because it is more readable with multiple types in my opinion.
Let me know if you have any concern. @alexander-fenster Thanks.
There was a problem hiding this comment.
They are similar, and gts forces us to use Array<> for complex types and [] for simple types. You did it correctly here.
summer-ji-eng
left a comment
There was a problem hiding this comment.
@alexander-fenster Made change, and please take another quick look, and I will merge the PR.
Thanks a lot for review my PRs.
🤖 I have created a release \*beep\* \*boop\* --- ## [2.18.0](https://www.github.com/googleapis/gax-nodejs/compare/v2.17.1...v2.18.0) (2021-07-13) ### Features * make OperationsClient closeable ([#1047](https://www.github.com/googleapis/gax-nodejs/issues/1047)) ([2dbba29](https://www.github.com/googleapis/gax-nodejs/commit/2dbba29dde552fb35c275a4a44b06fb4698eb5cf)) * support map handle for DIREGAPIC Pagination ([#1052](https://www.github.com/googleapis/gax-nodejs/issues/1052)) ([faab4c6](https://www.github.com/googleapis/gax-nodejs/commit/faab4c652c4943fc18c792995180bf59dbd5c7bc)) ### Bug Fixes * make pagination work for empty responses ([#1043](https://www.github.com/googleapis/gax-nodejs/issues/1043)) ([cbe2d3f](https://www.github.com/googleapis/gax-nodejs/commit/cbe2d3f9de4ec01e8e61699b5fa6bf7b34b870a5)) * replace isBrowser() with home made feature detection ([#1054](https://www.github.com/googleapis/gax-nodejs/issues/1054)) ([2c8e56d](https://www.github.com/googleapis/gax-nodejs/commit/2c8e56d5812af7b08ff6d68169d1d8ea325e03c2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
The DIREGAPIC pagination design suggests that the client libraries should return the whole contents of the dictionary key as a single resource being paginated. In the TypeScript implementation, we use tuple as representation.
Next TODO: Update client library surface of pagination response iterator type. Changes in gapic-generator-typescript
On the client side, the usage is
The output: