-
Notifications
You must be signed in to change notification settings - Fork 759
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
Add support for SearchResult. #1198
Conversation
lib/StripeMethod.js
Outdated
this.createResourcePathWithSymbols(spec.path || '') | ||
); | ||
spec.urlParams = !!spec.fullPath | ||
? [] |
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.
fullPath
is guaranteed not to have url params?
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.
Fixed! If we're moving everything to full paths, definitely doesn't make sense to exclude these.
This looks good, but I wonder - would the code be simpler if we removed all relative paths and used |
lib/autoPagination.js
Outdated
[reverseIteration ? 'ending_before' : 'starting_after']: lastId, | ||
}); | ||
return listPromise.then(iterate); | ||
if (spec.methodType === 'search') { |
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.
supernit: this code would feel slightly better to me if the spec.methodType
check was done once, top-level in makeAutoPaginationMethods
, rather than each time within iterate
.
Either something like
let getNextPagePromise;
if (spec.methodType === 'search') {
getNextPagePromise = ...
} else {
getNextPagePromise = ...
}
function iterate(pageResult) {
...
return getPagePromise(...).then(iterate)
}
or
let iterate;
if (spec.methodType === 'search') {
iterate = function (pageResult) {
...
};
} else {
iterate = ...
}
(the second would likely require factoring out a couple helper methods to keep it DRY)
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.
Done - good call!
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.
Nothing strictly blocking, though I'm curious what you think about fullPath everywhere
54c3f13
to
e39e26d
Compare
Thanks for the comments! For full paths, I'll lay down the ground work in this PR and then in a follow up can update the other methods to use it as it depends on basic methods. |
Removing reviews for a second. I think it'd be good to split our the fullPath logic of this PR into its own PR. |
e39e26d
to
bceadfd
Compare
I split out https://github.com/stripe/stripe-node/pull/1200/files for full path support. This should be good to go now. |
bceadfd
to
d346d57
Compare
I am merging this. Despite the fact that this functionality is part of a private beta, this will not affect the public interface of the library and can be safely removed/changed. |
Notify
r? @richardm-stripe
Summary
Adds support for search methods and
search_result
objects returned by the Stripe API. This requires updating auto pagination to support this. These paginate similarly to lists, however they rely on anext_page
token included in the response rather than using object IDs +starting_before
/ending_after
. Thus, only forward pagination is supportedTest plan
Added unit tests. We'll want to add some more rigorous tests once a resource is using this.