-
-
Notifications
You must be signed in to change notification settings - Fork 662
FixedLengthArray: Fix element type
#1246
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
Conversation
82cd904 to
fc2891f
Compare
|
Also, IMO, it doesn't feel very useful if we can only generate tuples having the same type in all positions. Maybe in the next major version we should change the API to simply accept a tuple, like: export type FixedLengthArray<TArray> = Except<TArray, ArrayLengthMutationKeys>;If I understand correctly, the only purpose of this type is to remove methods like |
|
Should these tests pass? expectAssignable<string[]>({} as FixedLengthArray<string, 3>);declare const a: FixedLengthArray<string, 3>;
declare const i: number;
expectType<string>(a[i]); |
|
A few more tests to add: expectType<3>({} as FixedLengthArray<string, 3>['length']);
expectType<string>({} as FixedLengthArray<string, 3>[number]); |
That sounds more like a different type. This type is homogeneous while your suggestion is heterogeneous. Could maybe be: export type FixedTuple<T extends unknown[]> = Except<T, ArrayLengthMutationKeys>;? |
True, but now that we have type RGB = FixedTuple<BuildTuple<3, number>>So, does it still make sense to have two separate types? |
This shouldn't, because expectAssignable<readonly string[]>({} as FixedLengthArray<string, 3>);
This one is related to the following point:
You can't index the array using the non-literal
This test is similar to the above. LMK if you see any issues with removing |
Yeah, discoverability. |
I'll add a note regarding this in the doc. |
|
How about this? expectType<string | undefined>({} as FixedLengthArray<string, 3>[number]); |
Yeah, I was also thinking about this. I think this is better. So, instead of removing the This also helps when we create non-tuples using |
a197bbd to
8c789eb
Compare
8c789eb to
728c4f4
Compare
source/fixed-length-array.d.ts
Outdated
| Use-cases: | ||
| - Declaring fixed-length tuples or arrays with a large number of items. | ||
| - Creating a range union (for example, `0 | 1 | 2 | 3 | 4` from the keys of such a type) without having to resort to recursive types. |
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.
Creating a range union (for example,
0 | 1 | 2 | 3 | 4from the keys of such a type) without having to resort to recursive types.
Not sure what this means, should we remove this?
In fact feels like we can completely remove the use-cases section, it doesn't add enough value IMO.
Declaring fixed-length tuples or arrays with a large number of items.
Feels like the primary use-case is to create arrays with "large number of items", which isn't true.
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 it was there to suggest it could be used for something like this:
type IndexRange<L extends number> =
Exclude<keyof FixedLengthArray<unknown, L>, keyof any[]>;but we can drop it. not that useful.
|
@sindresorhus Made significant changes to this PR, also updated the JSDoc completely, please review. |
|
Updated the PR, here's a quick summary:
|
||||||||||||||||||||||||||||||||||||||

Fixes #1245
Updated the implementation of
FixedLengthArray. The updated implementation simply callsBuildTupleand then removes keys like'pop','push'from it.Notes:
ArrayPrototypefromFixedLengthArray. This was used for internal computation and shouldn't have been exposed, so it's more of a bug than a breaking change, and since it had a default type, it's safe to remove it.numbertoArrayLengthMutationKeys, so that out-of-bounds access is prevented. I can't think of a case where this would break something.