Skip to content

Changed type definitions for functions with array parameters #69

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

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

BlueBlizzardd
Copy link
Contributor

Functions with array parameters should now correctly return arrays in the type definition, before, all functions regardless of their parameters just used to return a single element, which doesn't reflect the intended behavior. Just a small QoL change to help work with TS.

Array parameters inside functions should now correctly return arrays in
the type definition, just a small QoL change
@Naramsim
Copy link
Member

Naramsim commented Jan 7, 2025

Hi, I'm not that familiar with Types. @climaxmba could you review this PR?

I see that basically you removed all the pipes | and duplicated the functions. Can you point me to some resources that show why this is good?

@BlueBlizzardd
Copy link
Contributor Author

Hi! Basically, these are not pipes, but rather discriminated unions or Union Types.

The thing with these is that they help determine which types are supported as parameters by each function, but it's hard to specify which type the functions should return without rewriting the functions themselves, since typescript doesn't actually know which type should return what. For example:

getBerryByName(name: string | number | string[] | number[]): Promise<Berry>;

In this example, TS knows all of the possible input types, but it will only know that it has to return a single Berry every time as it's currently defined.

A possible solution to this is to write the following instead:

getBerryByName(name: string | number | string[] | number[]): Promise<Berry | Berry[]>;

The problem with this is that now, TypeScript doesn't actually know when it will return a single berry, and when it will return multiple without rewriting the function itself in TS or making the consumer of the returned promise narrow the return type manually.

To avoid these problems, we can instead duplicate the function signatures, basically stating that: in one case, the function will return only one element if only a string or number is given to it, and in another, if the function is given an array then TypeScript will know these functions will only return an array. This is also known as Function Overloading.

Another plus with these changes is that now heterogeneous arrays are supported, which they weren't previously before (only arrays of numbers OR strings could be passed as parameters, not ones that contained both).

Copy link
Contributor

@climaxmba climaxmba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure @Naramsim, Eduardo has a point.

Comment on lines +2254 to +2255
getBerryByName(name: string | number): Promise<Berry>;
getBerryByName(names: Array<string | number>): Promise<Berry[]>;
Copy link
Contributor

@climaxmba climaxmba Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, these lines tells TypeScript that a single object is returned in the Promise, only if a string or number is given in getBerryByName. And an array of objects, only if an array of strings or numbers is given.

@Naramsim
Copy link
Member

Naramsim commented Jan 8, 2025

Thanks so much for the explanation! Very informative! Now I see the benefits and we can merge!

@Naramsim Naramsim merged commit d041a17 into PokeAPI:master Jan 8, 2025
4 checks passed
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.

3 participants