-
-
Notifications
You must be signed in to change notification settings - Fork 549
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
Add UnionToTuple
type
#167
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.
Interesting addition! I tried to squeeze in a quick review, I hope all the comments of mine still makes sense 🙏
source/union-to-tuple.d.ts
Outdated
// This type may come from a library that you are using. | ||
type ActionsInSomeLibrary = 'create' | 'read' | 'update' | 'delete' | 'aggregate'; | ||
|
||
type AllowedActions = Exclude<ActionsInSomeLibrary, 'aggregate'>; | ||
|
||
const actions: UnionToTuple<AllowedActions> = ['create', 'read', 'update', 'delete']; | ||
|
||
actions.includes(requestedAction) // Validate |
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.
What benefit does this provide over AllowedActions[]
? It's likely just me missing something, but I'm thinking, if I'm missing something then someone else can as well, so then we should clarify it 🙂
const actions: AllowedActions[] = ['create', 'read', 'update', 'delete'];
I get the impression that actions.includes(requestedAction)
will work better when the type of actions
is UnionToTuple<AllowedActions>
rather than AllowedActions[]
? But I can't reproduce that: TypeScript Playground example
Also, I think that requestedAction
needs to be defined in this example, else one doesn't know what type that one has at all, and that seems to be vital to the example. So add something like const requestedAction = 'create';
?
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.
What benefit does this provide over
AllowedActions[]
?
With just AllowedActions[]
you are allowed to do const actions: AllowedActions[] = [];
and const actions: AllowedActions[] = ['create', 'create', 'create'];
which are not what is actually intended. Do you think this should be talked about in the description?
: never | ||
> extends ((merged: infer Intersection) => void) | ||
// Transforms ('A' | 'B') into [[[], "A"], "B"], but destructures the arrays | ||
? readonly [...UnionToTuple<Exclude<Union, Intersection>>, Intersection] |
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.
Why is this set to readonly
? Because it's expected to only be used with string
only unions? And that way it would essentially not be possible to manipulate anyway? But then, wouldn't this readonly
be fairly redundant as any meaningful mutation would anyhow already be rejected by the tuple values in itself?
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.
Without readonly
you are allowed to do manipulations like
const actions: UnionToTuple<AllowedActions> = ['create', 'read', 'update', 'delete'];
actions.shift();
declare type Union = 'A' | 'B'; | ||
declare const tuple: UnionToTuple<Union>; | ||
expectAssignable<readonly ['A', 'B']>(tuple); |
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 we need to extend these tests quite a bit. This only tests a "sunny day scenario", the simplest of cases, we should have some tests that tries to break it.
I eg. tried this:
type AdvancedUnion = 'create' | number;
type AdvancedTuple = UnionToTuple<AdvancedUnion>;
const foo: AdvancedTuple = ['create', 123];
And that fails (see playground) as for some reason the calculated tuple becomes [number, 'create']
rather than the expected ['create', number]
.
I similarly tested UnionToTuple<'create' | boolean>
, which results in [false, true, 'create']
, so also it returns things in the wrong order, but it also extends boolean
into false
and true
respectively.
If UnionToTuple
only supports string
unions, then it should be changed to UnionToTuple<Union extends String>
+ a test should be added with expectError()
that ensures that it eg. UnionToTuple<'create' | boolean>
then fails.
Alternatively, if UnionToTuple
is supposed to support more than just strings, then tests should be added to ensure that + the implementation fixed so that it works as expected.
I would also like a test for:
type Foo = 'abc' | 'def';
type Bar = Foo | 'xyz';
type GiveMeThatTuple = UnionToTuple<Bar>;
Which would highlight if the expected outcome is ['abc' | 'def', 'xyz']
or ['abc', 'def', 'xyz']
. It may be obvious, but yet not obvious.
Co-authored-by: Pelle Wessman <pelle@kodfabrik.se>
|
||
@example | ||
``` | ||
type Union = 'A' | 'B'; |
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.
Can you include the import statement? It's nice when examples are directly runnable.
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.
Just added import statements and declared the requestedAction
variable as well.
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
This is extremely useful for some cases, especially with template literal types now where you can get a string union from a tuple, derive another useful union from that, but then cannot get a tuple type for the derived union. For example:
I don't fully understand the error cases other have pointed out here, but perhaps restricting UnionToTuple to operating only on string unions actually solves most of the use cases? |
Yeah, this becomes even more powerful with template literal types as you demonstrated. I would be okay with constraining this to only string unions since I don't know how to solve the issue with non-string values that @voxpelli encountered above. |
Hey guys. While testing this some more, I found that there were multiple times where TS would randomly change the position of some values inside a string union, which would then break tuple types that were previously working. For example, there was a time where a tuple was assigned to I initially thought that only computed unions would be a problem as it talked about in the TS repo issue, but it seems like the order of manually declared unions are also not stable. Because of this, I'll have to close this PR. If anyone still needs a way to convert string unions to tuples for validation, I posted this solution which uses |
Maybe this PR can still be useful? You could just add a note that "the order in which the tuple is generated is not guaranteed". |
The problem is that every time TS changes the order of your tuple, your code will not compile until you also change it. One way to fix this would be to generate all possible permutations of the union so that any order you use would be valid. So for a union of 10 elements, we would have to generate 10! (3628800) types, which obliterates the performance. |
Only if your code depends on the order of the generated tuple, right? Since this type will be documented to not guarantee order, users of this type will know that they should only use it when the order does not matter. |
ya could put it in a |
This one is controversial, but I and a lot of other people have run into this...
Convert an explicit union type to an tuple.
The elements order of the tuple and of the union will be the same. Because of that, you should not use this if the union is computed instead of manually declared, because the TypeScript compiler does not guarantee that the order of the union elements will always be the same.
Inspired by this issue in the TypeScript repo.
Use-case: You need to do validation but you don't have an enum, only an union type. Similar to the
Extract
example in the readme, but in that case thechangePersonData
function does not validate thekey
parameter at runtime, making it only safe inside TypeScript. Using a tuple andincludes
for validation gives us runtime safety for that kind of function.