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 got.paginate(...)
typings
#1099
Conversation
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.
Looks good, but I don't think this is a good approach.
got.extend({
_pagination: {
// `item` is unknown here
filter: item => true
...
}
})
People should type the items manually, because the type depends on the transform
function.
Hm, not sure if I got your point. Currently, you can pass the type of the item as a generic to the pagination function and you have to explicitly type the response type for |
Yeah, but instead of passing the options every time you can extend the instance, right? |
Got it. For Lines 163 to 171 in cf4fdad
|
So you can define your own options as One of the "solutions" would be to use @sindresorhus What do you think? |
@sindresorhus any opinion? 😊 |
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.
Use Except
as it's stricter.
@szmarczak yep, I replaced it... |
I'm fine with this. |
@szmarczak could you please review this PR then again? 😊 |
source/create.ts
Outdated
export interface GotPaginate { | ||
<T>(url: URLOrOptions & PaginationOptions<T>, options?: Options & PaginationOptions<T>): AsyncIterableIterator<T>; | ||
all<T>(url: URLOrOptions & PaginationOptions<T>, options?: Options & PaginationOptions<T>): Promise<T[]>; | ||
<T>(url: string | GotPaginateOptions<T>, options?: GotPaginateOptions<T>): AsyncIterableIterator<T>; |
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 think string | GotPaginateOptions<T>
should be URLOrOptions & GotPaginateOptions<T>
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 would duplicate work but also duplicate the options because GotPaginateOptions
is defined like this:
export type GotPaginateOptions<T> = Except<Options, keyof PaginationOptions<unknown>> & PaginationOptions<T>;
Though, I just pushed a commit which adds the URLOrGotPaginateOptions
type so we have that definition in one place.
It's only missing a test |
@szmarczak shouldn't the existing unit tests cover it? |
What do you mean? We don't test your changes, we only test the current code. |
I assumed that the changes are only about types and the build would anyway fail if there would be any typing issues. Anyway, now I added some explicit types that should cover the new behaviour. If you have something else in mind, let me know. |
Looks good! |
Thanks for the inputs! |
Thanks :) |
Currently, if you use
got.paginate
orgot.paginate.all
you cannot properly use the pagination options becauseGotOptions
extendPaginationOptions<unknown>
:got/source/types.ts
Line 173 in 526b4bb
This would result in unknown types:
Checklist