Add V2SearchResultPromise and V2SearchResult - #2792
Conversation
xavdid
left a comment
xavdid
left a comment
There was a problem hiding this comment.
Thanks for taking a look! There a few more things we need before we can merge.
In addition to my comments, you should add actual tests, plus type tests to ensure v2 search responses have the properties we expect. We don't have any in the generated methods yet (since you're just adding them here) but you can define one in the types test with a TODO to replace it with a real v2 search later.
| * Opaque cursor to fetch the next page of results. | ||
| * Pass back as the `page` request parameter. Null if no more pages. | ||
| */ | ||
| next_page_url: string | null; |
There was a problem hiding this comment.
Are these the only properties a list response will ever have?
Also, is this data available in the openapi spec anywhere? It's the sort of thing we'd like to generate, where possible
There was a problem hiding this comment.
These are the only fields defined in the API pattern review for v2 search.
The only v2 search endpoint currently is custom objects. I am not familiar enough with how the spec generation works for it in sdk generation, but happy to dig into it if you can point me in the right direction.
| AsyncIterableIterator<T> { | ||
| autoPagingEach( | ||
| handler: (item: T) => boolean | void | Promise<boolean | void>, | ||
| onDone?: (err: any) => void |
There was a problem hiding this comment.
the v1 autoPagingEach and autoPagingToArray doesn't have an onDone property. Why the mismatch?
There was a problem hiding this comment.
I was originally modelling it against v2List which seemed to be the pattern we wanted to emulate. The final decision on the shape of v2 search pagination hasn't been made so I removed it for now
| export interface V2SearchResultPromise<T> | ||
| extends Promise<Response<V2SearchResult<T>>>, | ||
| AsyncIterableIterator<T> { | ||
| autoPagingEach( |
There was a problem hiding this comment.
You'll need to add these to iterator responses in autoPagination.ts. We currently guard against apiMode !== 'v2', so that'll presumably change
There was a problem hiding this comment.
When I was looking into this, I saw that V2ListIterator didn't support bidirectional pagination. When we add support for V2SearchIterator would it make sense to create it as a separate method and support bidirection or update (and rename) V2ListIterator to support it as well.
This is assuming that v2 search pagination works the same way as v2 list pagination
| V2List, | ||
| V2ListPromise, | ||
| V2SearchResult, | ||
| V2SearchResultPromise, |
There was a problem hiding this comment.
These need to be added in the matching cjs files
There was a problem hiding this comment.
Is that file autogenerated from sdk-codegen?
https://github.com/stripe/stripe-node/blob/master/src/stripe.cjs.node.ts#L14469
|
The pagination for V2 search hasn't been gavel yet, but we seem to be close to a resolution soon. I'm cleaning this PR in preparation for it. Are these sufficient type tests? I'm planning on adding tests after we add pagination as well, are there any other that I'm missing. |
2 participants
Why?
V2 search api responses have a different shape than v1 search. We are adding the first v2 search api and will need to update sdk generation to return the right fields for the new search endpoint.
What?
- Adds V2SearchResultPromise interface extending Promise<Response<V2SearchResult>> with autoPagingEach and autoPagingToArray methods like V2ListPromise
See Also
https://jira.corp.stripe.com/browse/DEVSDK-3211
Changelog