Move class property transformation into new transformer file #30467
Conversation
IIRC this is actually required to get the execution order of computed names right, since all computed names need to execute after the declaration, but before any following expressions. |
|
@weswigham Yep. Should update this comment, the complication comes into play with a decorated class declaration, which gets converted into a class expression by the TypeScript transformer. This results in the expressions for computed property names and static properties to be emitted in a comma expression rather than as separate statements after the decorated class. |
|
ping @weswigham @rbuckton @DanielRosenwasser @RyanCavanaugh would you be up for taking another look? This PR will unblock related work. |
|
I would recommend that the new transformer be named |
|
@rbuckton I renamed the transformer using the "class fields" terminology. Think we're ready for another pass! |
|
@typescript-bot perf test |
|
The perf tests show a regression (and failed to post the perf results back to the PR), but it may be that your copy of |
Signed-off-by: Joseph Watts <jwatts43@bloomberg.net>
joeywatts
force-pushed
the
move-class-property-transform
branch
from
d7022a9
to
9777fb9
Compare
|
@rbuckton I've squashed the changes down to 1 commit and rebased on top of latest master. |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 9777fb9. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@rbuckton Here they are:Comparison Report - master..30467
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
In general the performance seems acceptable. The only outlier is |
|
@rbuckton does this indicate a performance issue or do you think it could be a flaky test run? I wonder why the variation in runtime is much higher than the other ones. |
|
@joeywatts I'll look into it over the next few days to see if there's something specific to Monaco on node@8.9.0 that could be causing a problem. |
|
@rbuckton were you able to see if there's anything specific to Monaco and node@8.9.0? Also, is it possible to run these performance tests locally? |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 9777fb9. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@rbuckton Here they are:Comparison Report - master..30467
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Monaco for node@8.9.0 x64 is still a bit slower than master, but it looks like its within the margin of error. I would argue that its not significant enough of a difference. the 9.4% delta from before was obviously a fluke. |
|
Thanks for the amazing work so far :) |
)" This reverts commit 3139cf2.
|
We had to revert the PR because of numerous breaks in the RWC test suites. We can give it another go once we have the issues fixed. |
|
Reverted in #31807 - see that ticket for what the breakages were |
|
@ahejlsberg thanks for the update and sorry for the trouble! Happy to help with the fix if we can. |
|
The RWC tests rely on the presence of a manually-cloned internal repo. (I believe it contains Microsoft-internal code, which is the reason details are sparse in the build logs—I only just learned this yesterday, so I’m not 100% sure of the details.) It can be triggered by commanding typescript-bot to “test this.” Maybe we can automatically trigger some extended test suites for PRs that are above some threshold of changed lines. /cc @weswigham who has been working on build automation stuff lately—thoughts? |
Eh, history has shown that some of the shortest changes can have the most real world impact. We really should just be trying it pre-merge on every PR, it's just a bit overkill to run it on every commit, and there's not really a PR state corresponding to "done and ready to merge". I could set up a bot command that corresponds to a bulky "run all the extended tests and if they pass, merge" (or a label, but that's not much different) - but that's just a shortcut for the commands we usually already run (and I'd rather still have merges finally merged by humans, as a rule). So.... Eh? |
Approved by a team member is an imperfect proxy, but might be better than nothing. At the same time, we’re a small enough team that I would buy the argument that the rate of return on automating the last 1% of something that's already 99% automated is pretty small. |
|
I looked into this on Friday and a little bit over the weekend and tracked down what was causing the problem. I will post a revised PR on Monday. |
This is a step towards addressing #12212. In this PR, the transformation of class properties is moved to a new transformer file. This transformer is currently always executed after the TypeScript transformer to preserve the existing output as much as possible (there are compatibility issues with putting this behind a target language condition, see #27644). It will be easy to put this transformation behind a language version check in the future.
There were a couple of notable choices made here.
Choice: preserve difference in emit between class expression vs. class declaration
This PR preserves this behavior, at the expense of some complication here since decorated class declarations are converted into class expressions by the TypeScript transformer and class properties are now transformed after this has already occurred.
Choice: when there is at least one parameter property, move all property initializers to the constructor body
We had to do this to preserve the evaluation order of the initializers. This choice makes no difference to current behavior of the compiler, because the class property transform is unconditionally applied in transformer.ts. So now all property initializers end up in the constructor body anyway.