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

Set MaxInt to 51 as the existing logic isn't inclusive #46

Closed
wants to merge 1 commit into from

Conversation

lukemansell
Copy link
Contributor

@lukemansell lukemansell commented Jul 28, 2023

Closes #45

Sets MaxInt to 51 as existing logic isn't inclusive

Problem

Currently if you pass through 50 on methods which have limit?: MaxInt<50>, you get a typescript error saying that you can only do 0-49. On the Spotify API/Documentation, 50 is allowed so this should be fixed to allow it to be inclusive of the number set.

Solution

Set the MaxInt to 51, without changing the underlying logic.

Side note for reviewer

I tried changing the existing logic for MaxInt (existing logic below)

export type MaxInt<T extends number> = number extends T ? number : _Range<T, []>;
export type _Range<T extends number, R extends unknown[]> = R['length'] extends T ? R[number] : _Range<T, [R['length'], ...R]>;

But, I don't have the best Typescript knowledge, so I couldn't work out a way to make this inclusive - I believe it's an open discussion on Typescript issues for making this nicer to do microsoft/TypeScript#54925 So setting these to MaxInt<51> was a bit of a compromise to make this work. Maybe someone else has a nicer way to fix MaxInt/_range

@lukemansell lukemansell changed the title bug: Set MaxInt to 51 as the existing logic isn't inclusive Set MaxInt to 51 as the existing logic isn't inclusive Jul 28, 2023
@RobinHeidenis
Copy link
Contributor

I ran in to this issue recently, and did some experimenting with the original type. I fixed it like this:

export type MaxInt<T extends number> = number extends T ? number : _Range<T, []>;
export type _Range<T extends number, R extends unknown[]> = R['length'] extends T ? R[number] | T : _Range<T, [R['length'], ...R]>;

The fix specifically is to add | T to the first R['length'] extends T ? ternary true value. This adds the T type to the result, so if you pass the type 50 it now also includes 50 in the final type.

Fixing the type here allows you to only change it once, instead of everywhere else. It's also a little easier for future contributors, as they don't have to know you need to pass 51 to make it 50.

@lukemansell
Copy link
Contributor Author

@RobinHeidenis - Did you want to make a branch and commit that up? You can link it to #45

Yeah, changing it to 51 wasn't too ideal, but I couldn't find a way to get it to work.

@thisisjofrank
Copy link
Contributor

Hey folks. I've merged @RobinHeidenis's suggestion. so going to close this. Thanks both!

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.

MaxInt value is not inclusive
3 participants