Skip to content
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

Merged
merged 6 commits into from Mar 24, 2020
Merged

Conversation

jaulz
Copy link
Contributor

@jaulz jaulz commented Mar 3, 2020

Currently, if you use got.paginate or got.paginate.all you cannot properly use the pagination options because GotOptions extend PaginationOptions<unknown>:

export interface GotOptions extends PaginationOptions<unknown> {

This would result in unknown types:

got.paginate.all<Item>('/test', { 
  _pagination: {
    // item is of type unknown instead of Item
    filter: (item) => true
  }
})

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates.

@jaulz jaulz changed the title fix: typings for paginate [WIP] fix: typings for paginate Mar 3, 2020
@jaulz jaulz changed the title [WIP] fix: typings for paginate fix: typings for paginate Mar 3, 2020
@szmarczak szmarczak changed the title fix: typings for paginate Fix got.paginate(...) typings Mar 3, 2020
Copy link
Collaborator

@szmarczak szmarczak left a 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.

@jaulz
Copy link
Contributor Author

jaulz commented Mar 3, 2020

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 transform.

@szmarczak
Copy link
Collaborator

Yeah, but instead of passing the options every time you can extend the instance, right?

@jaulz
Copy link
Contributor Author

jaulz commented Mar 3, 2020

Got it. For extend it's of course necessary to explicitly add typings but I still don't understand why it's not a good approach? Why do we have it as a generic at all then?

got/source/types.ts

Lines 163 to 171 in cf4fdad

export interface PaginationOptions<T> {
_pagination?: {
transform?: (response: Response) => Promise<T[]> | T[];
filter?: (item: T, allItems: T[]) => boolean;
paginate?: (response: Response) => Options | false;
shouldContinue?: (item: T, allItems: T[]) => boolean;
countLimit?: number;
};
}

@szmarczak
Copy link
Collaborator

Why do we have it as a generic at all then?

So you can define your own options as PaginationOptions<Blah> (const options = ...) and cast it to PaginationOptions<unknown> later :)

One of the "solutions" would be to use any instead of unknown but it's not a solution as it doesn't require you to type the items properly.

@sindresorhus What do you think?

@jaulz
Copy link
Contributor Author

jaulz commented Mar 17, 2020

@sindresorhus any opinion? 😊

Copy link
Collaborator

@szmarczak szmarczak left a 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.

@jaulz
Copy link
Contributor Author

jaulz commented Mar 20, 2020

@szmarczak yep, I replaced it...

@sindresorhus
Copy link
Owner

I'm fine with this.

@jaulz
Copy link
Contributor Author

jaulz commented Mar 23, 2020

@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>;
Copy link
Collaborator

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>

Copy link
Contributor Author

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.

@szmarczak
Copy link
Collaborator

It's only missing a test

@jaulz
Copy link
Contributor Author

jaulz commented Mar 24, 2020

@szmarczak shouldn't the existing unit tests cover it?

@szmarczak
Copy link
Collaborator

szmarczak commented Mar 24, 2020

shouldn't the existing unit tests cover it?

What do you mean? We don't test your changes, we only test the current code.

@jaulz
Copy link
Contributor Author

jaulz commented Mar 24, 2020

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.

@szmarczak
Copy link
Collaborator

Looks good!

@jaulz
Copy link
Contributor Author

jaulz commented Mar 24, 2020

Thanks for the inputs!

@sindresorhus sindresorhus merged commit 0b798ea into sindresorhus:master Mar 24, 2020
@sindresorhus
Copy link
Owner

Thanks :)

@jaulz jaulz deleted the patch-3 branch March 24, 2020 18:10
@szmarczak szmarczak mentioned this pull request Mar 24, 2020
18 tasks
szmarczak added a commit to szmarczak/got that referenced this pull request Mar 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants