-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Fixed an issue with not being able to use mapped type over union constraint as rest param #49947
Changes from 1 commit
6554965
f94efbd
ce8483c
29d4bb9
940e1ad
88764fe
8e78414
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
…traint as rest param
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
=== tests/cases/compiler/restParamUsingMappedTypeOverUnionConstraint.ts === | ||
// repro 29919#issuecomment-470948453 | ||
|
||
type Mapped<T> = { [P in keyof T]: T[P] extends string ? [number] : [string] } | ||
>Mapped : Symbol(Mapped, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 0, 0)) | ||
>T : Symbol(T, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 2, 12)) | ||
>P : Symbol(P, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 2, 20)) | ||
>T : Symbol(T, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 2, 12)) | ||
>T : Symbol(T, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 2, 12)) | ||
>P : Symbol(P, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 2, 20)) | ||
|
||
declare function test<T extends [number] | [string]>( | ||
>test : Symbol(test, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 2, 78)) | ||
>T : Symbol(T, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 4, 22)) | ||
|
||
args: T, | ||
>args : Symbol(args, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 4, 53)) | ||
>T : Symbol(T, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 4, 22)) | ||
|
||
fn: (...args: Mapped<T>) => void | ||
>fn : Symbol(fn, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 5, 10)) | ||
>args : Symbol(args, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 6, 7)) | ||
>Mapped : Symbol(Mapped, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 0, 0)) | ||
>T : Symbol(T, Decl(restParamUsingMappedTypeOverUnionConstraint.ts, 4, 22)) | ||
|
||
): number | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
=== tests/cases/compiler/restParamUsingMappedTypeOverUnionConstraint.ts === | ||
// repro 29919#issuecomment-470948453 | ||
|
||
type Mapped<T> = { [P in keyof T]: T[P] extends string ? [number] : [string] } | ||
>Mapped : Mapped<T> | ||
|
||
declare function test<T extends [number] | [string]>( | ||
>test : <T extends [number] | [string]>(args: T, fn: (...args: Mapped<T>) => void) => number | ||
|
||
args: T, | ||
>args : T | ||
|
||
fn: (...args: Mapped<T>) => void | ||
>fn : (...args: Mapped<T>) => void | ||
>args : Mapped<T> | ||
|
||
): number | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
// @noEmit: true | ||
// @strict: true | ||
|
||
// repro 29919#issuecomment-470948453 | ||
|
||
type Mapped<T> = { [P in keyof T]: T[P] extends string ? [number] : [string] } | ||
|
||
declare function test<T extends [number] | [string]>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This repro is a lot easier to understand for me because it has fewer moving parts and fewer repeated identifier, so it's easier to tell what dependencies exist between elements: declare function test<T extends [number] | []>(
fn: (...args: { [_ in keyof T]: [boolean] | [string] }) => void
): number There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with the fact that the proposed version is more succinct but at the same time, I don't feel like the added test case is overly verbose. For me personally, this is a little bit harder to read because it's less "usual". The inlined mapped type also doesn't make it completely obvious to me if this mapped type is still homomorphic or not (although that's just my lack of expertise). Overall, I don't mind making a change but perhaps it would be good to have some guidelines in the Please let me know if I should adjust the test, add a new one or leave it as is. |
||
args: T, | ||
fn: (...args: Mapped<T>) => void | ||
): number |
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.
There is a good chance that this should do the same thing in the
whenFalse
branch of this ternary expression:but to make such a change I will have to figure out a test case validating it. I don't think it's going to be super hard but my available time is limited rn.
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.
The false branch is just for read-only types, so yeah, it should probably be the same, otherwise it'll work for non-readonly tuples, but not readonly ones.
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've pushed out a fix and tests for this.