-
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
[Experiment] fix(40617): handle uninitialized class member with computed key #45974
Conversation
7dc42e8
to
fed9253
Compare
@typescript-bot pack this |
Heya @andrewbranch, I've started to run the perf test suite on this PR at fed9253. You can monitor the build here. Update: The results are in! |
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at fed9253. You can monitor the build here. |
Hey @andrewbranch, 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 |
@andrewbranch Here they are:Comparison Report - main..45974
System
Hosts
Scenarios
Developer Information: |
0d8ea00
to
5966bca
Compare
5966bca
to
e3d3857
Compare
7532268
to
d62e104
Compare
674f8ac
to
f62eed2
Compare
591ce0f
to
c1ea660
Compare
c1ea660
to
5a5a04a
Compare
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 5a5a04a. 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 5a5a04a. You can monitor the build here. |
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 - main..45974
System
Hosts
Scenarios
Developer Information: |
@weswigham @andrewbranch @DanielRosenwasser is this good to go into 4.7? Looks like we've got a recent performance run for it now. |
This is something I wanted to figure out - whether the check time increases on the most recent change are within the margin of error. |
Between zero and seven hundreths of a second (depending on the test, mapping to between zero and one point four percent) sounds like noise if not noise-adjacent numbers to me. |
@typescript-bot perf test this |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 5a5a04a. You can monitor the build here. Update: The results are in! |
@DanielRosenwasser Here they are:Comparison Report - main..45974
System
Hosts
Scenarios
Developer Information: |
Yeah, this second run makes it look more like noise. We see check time decrease on some and then increase on the same repo with a different Node version. |
From the design meeting:
|
Weighing in as one user: supporting symbol-named properties in CFA (without temporary variables) seems very nice and is definitely what I would expect. For me, it's easily worth this tiny performance penalty, though I probably like/use symbol named properties more than many. |
(happy to move this to a new issue) If I read the release blog post correctly, the following test case should work, but it still produces an error. interface Foo {
[key: string]: number | string;
}
declare function generateKey(): string;
function fn(foo: Foo) {
const key = generateKey();
if (typeof foo[key] === 'number') {
foo[key].toExponential(2);
// Error: Property 'toExponential' does not exist on type 'string | number'.
}
} Is this a bug? TS Playground Link v4.7.0-dev.20220420 |
I'd definitely file that as a separate issue, otherwise we're not going to be able to track or discuss it. |
Fixes #40617
Fixes #45965
Fixes #36230
Fixes #29042
Fixes #46111
Fixes #14137