-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Improve .origin
preservation in getNarrowedTypeWorker
#61589
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
base: main
Are you sure you want to change the base?
Improve .origin
preservation in getNarrowedTypeWorker
#61589
Conversation
); | ||
// If no constituents are directly related, create intersections for any generic constituents that | ||
let matchedCandidates: Type[] = []; | ||
let narrowedType = mapType(type, t => |
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 whole change aims to avoid any major functional changes.
Conceptually, it just "flips" the order of the 2 nested for loops. Before candidates were iterated and that iteration was iterating through types. With this change, it's the other way around - types are iterated and they have an inner loop through candidates. This required some changes to pull this off (like the introduction of matchedCandidates
).
But why this is even needed? The goal of this code is to narrow down a type by a candidate and to truly narrow down a type a mapType
or filterType
have to be used because they try to preserve .origin
information. By having the outer loop mapping through candidate
this "merely" produced a result related to type
but not one that would directly be derived from it -so the .origin
had very slim chances of surviving when narrowing by a union candidate
@@ -557,8 +557,8 @@ if (isEmpty(test)) { | |||
> : ^^^^^^^^^^^^^^^^^^^^^^^^^ | |||
|
|||
test; // EmptyString | |||
>test : EmptyString |
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.
Those changes are fine - it's the candidate
that had .origin
on it but the goal is to preserve type
's .origin
and not .candidate
's. The contained types here are exactly the same
@typescript-bot test it |
Hey @jakebailey, 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 |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Damn, those perf results are not great 😢 But first I'll have to investigate the webpack change |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
@jakebailey could you rerun the tests here? |
@typescript-bot test it |
Hey @jakebailey, 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 |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
@jakebailey could you rerun this again? I don't think this will fix the degraded performance (I'm working on that) but it would be nice to learn if the extended test suites can catch any new functional issues with the current version |
@typescript-bot test it |
Hey @jakebailey, 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 |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
The last results were better but there is still some slight increase in check type of Compiler-Unions. I have pushed out now a version that might address this. Could you rerun tests for me again here @jakebailey 🙏 ? :) |
@typescript-bot test it |
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
fixes #61581