- Notifications
You must be signed in to change notification settings - Fork 13.2k
Use better context scope for class constructor implementation signatures#58168
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
Use better context scope for class constructor implementation signatures #58168
Uh oh!
There was an error while loading. Please reload this page.
Conversation
jakebailey left a comment
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.
@typescript-bot test it
typescript-bot commented Apr 12, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
typescript-bot commented Apr 12, 2024
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
MichaelMitchell-at commented Apr 12, 2024
Hm, the repro I provided in #58158 is fixed now, but I'm still seeing the issue in our codebase. I'll have to update the repro to more closely resemble our actual code. |
typescript-bot commented Apr 12, 2024
@jakebailey Here are the results of running the user tests comparing Everything looks good! |
typescript-bot commented Apr 12, 2024
@jakebailey Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
MichaelMitchell-at commented Apr 12, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
@weswigham here's another regressed case that doesn't yet pass typeAssign<T,U>=Omit<T,keyofU>&U;classBase<T>{constructor(publict: T){}}exportclassFoo<T>extendsBase<T>{update(): Foo<Assign<T,{x: number}>>{constv: Assign<T,{x: number}>=Object.assign(this.t,{x: 1});returnnewFoo(v);}} |
weswigham commented Apr 12, 2024
@MichaelMitchell-at handled~ @typescript-bot test it |
typescript-bot commented Apr 12, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
MichaelMitchell-at commented Apr 12, 2024
Nice, that did the trick, thank you |
typescript-bot commented Apr 12, 2024
Hey @weswigham, the results of running the DT tests are ready. Everything looks the same! |
typescript-bot commented Apr 12, 2024
@weswigham Here are the results of running the user tests comparing Everything looks good! |
typescript-bot commented Apr 12, 2024
@weswigham Here they are:tscComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
typescript-bot commented Apr 12, 2024
@weswigham Here are the results of running the top 400 repos comparing Everything looks good! |
Since those type parameters are reachable anywhere in the body of the class, not just the constructor itself.
Honestly, we could always do the
getImplementationSignaturecall, it doesn't have to be conditional, but we like trying to cut down on the type parameter copying we do, where possible. And certainly, as long as you only ever use the signature externally to the scope within which its' type parameters are internally reachable, you don't need to copy it.Fixes#58074