-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Remove special any
assignability for numeric index signatures
#41660
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
Remove special any
assignability for numeric index signatures
#41660
Conversation
@typescript-bot pack this |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
interface Lifecycle<Attrs, State> { | ||
oninit?(vnode: Vnode<Attrs, State>): number; | ||
~~~~~ | ||
!!! error TS2344: Type 'State' does not satisfy the constraint 'Lifecycle<Attrs, State>'. |
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.
This was a bug. State
needs the constraint Lifecycle<Attrs, State>
.
@@ -25,10 +41,23 @@ tests/cases/compiler/tile1.ts(6,81): error TS2744: Type parameter defaults can o | |||
interface MyAttrs { id: number } | |||
class C implements ClassComponent<MyAttrs> { | |||
view(v: Vnode<MyAttrs>) { return 0; } | |||
~~~~ | |||
!!! error TS2416: Property 'view' in type 'C' is not assignable to the same property in base type 'ClassComponent<MyAttrs>'. |
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.
This basically comes down to the following break:
interface Indexable {
[x: number]: any;
}
class C implements Indexable {
}
In earlier versions, implementing C
without re-declaring the index signature was valid.
I would argue that this behavior is still surprising or incorrect for string index signatures, but that might be very breaky.
|
Updates:
|
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 7dd8232. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 7dd8232. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 7dd8232. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 7dd8232. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the parallelized community code test suite on this PR at 7dd8232. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at 7dd8232. You can monitor the build here. Update: The results are in! |
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your
and then running There is also a playground for this build and an npm module you can use via |
@DanielRosenwasser Here they are:Comparison Report - master..41660
System
Hosts
Scenarios
|
@DanielRosenwasser Here they are:Comparison Report - master..41660
System
Hosts
Scenarios
|
I don't think we're correctly running the community test suite anymore (#42875), so I can't confidently comment on if anything new broke. But I think this is now a "serious PR". |
Assuming there's nothing too concerning in the user tests, I'm okay with putting this in the nightly and watching for fallout. I stand by the current behavior being a bug 🙂 |
Experiment to fix #18757