Repository navigation
Static field emit is IIFE #38528
Description
TypeScript Version: 3.9.2
Search Terms: static field, static class members, emit
Code
// @target: esnext
// @useDefineForClassFields: true
class C {
static x = 3;
}Expected behavior:
Expected JS emit is:
class C {
static x = 3;
}Actual behavior:
Actual JS emit gets wrapped in an unwanted IIFE.
let C = /** @class */ (() => {
class C {
static x = 3;
}
return C;
})();Playground Link:
- Correct behavior in TypeScript 3.8.3
- Correct behavior in TypeScript 3.9.0-dev.20200214
- Incorrect behavior in TypeScript 3.9.0-dev.20200215
- Incorrect behavior in TypeScript 3.9.2
Related Issues: #38374
The bug is that classes with static fields are now getting wrapped in an IIFE. This undesirable bloat occurs unconditionally. There does not appear to be anyway to turn this off to get back to standard JS emit.
The emit change was introduced by #32011
Credit to Evan Martin (@evmar) for discovering this issue.
Activity
robpalme commentedon May 13, 2020
Also, I should apologize for not catching this sooner. We promised to set up CI coverage of TypeScript candidate releases. And we did set it up! Unfortunately the CI early-exited when it hit a package that wasn't ready for TypeScript 3.9, which stopped it reaching our emit-equivalence check for any of the packages. Having just disabled the early-exit, it immediately revealed this issue in the Beta 😬 We'll do better for the next release.
robpalme commentedon May 13, 2020
Ok, I think I understand the full issue now.
Dead-code elimination for unused classes was previously failing for down-leveled classes with static fields. So the IIFE + magic-comment was introduced to fix this.
The remaining problem is that the fix was scoped too widely, and ends up getting applied even in cases where no down-leveling is required.
rrdelaney commentedon May 13, 2020
We (Google) are also running into this issue 😞 I filed #38374 to add a flag controlling the IIFE emit, because although the emit is valuable to some users, it messes up our toolchain.
robpalme commentedon May 13, 2020
Thanks for highlighting this Ryan Delaney (@rrdelaney). I think these two issues are related and orthogonal.
rrdelaney commentedon May 13, 2020
I think we might be on the same page here. The idea behind #38374 is to allow us to have lossless emit behind a flag, while still allowing the community to have optimizer hints if they want them. The IIFE doesn't work with our pipelines but is definitely helpful for a lot of people.
clydin commentedon May 13, 2020
The issue here is that the ESNext target is not accounted for in the logic. In all actual current ECMAScript targets, static is down-leveled. This was definitely an oversight.
rrdelaney commentedon May 14, 2020
Only emitting for downleveled code wouldn't solve Google's issue, we're still using ES2015 emit.
clydin commentedon May 15, 2020
The problem demonstrated in this issue only represents when targeting ESNext and the
useDefineForClassFieldsoption is enabled. In this specific situation, static class properties are not down-leveled and are instead kept as direct members of the class. As a result, a wrapper has no real value (assuming no decorators are present) since the wrapper would only contain the class definition.rrdelaney commentedon May 16, 2020
This is also an issue when targeting ES2015: https://www.typescriptlang.org/play?target=2&module=1#code/MYGwhgzhAEDC0G8BQ1oQC5nQS2NADgE4D2+0AvNAIwAMA3EitAOYCm6ACifgBQCUiJqkLsAroQB2cAHRFSDVAF8kyoA
The output from 3.8 allowed for better understanding of the class declaration by static analyzers. Having the wrapper is definitely valuable for something like Terser, but it is not desirable in all situations.
rrdelaney commentedon May 16, 2020
Oh wait ok I think I understand this better now. This issue is a subset of the problem we are experiencing, where we would not like the wrapper for ES2015 classes at all. I should incorporate this fix into my CL as well then.
Sorry for the misunderstanding 😅 I’m going to rebase my PR on top of yours
clydin commentedon May 16, 2020
No problem and yes, that's correct. This particular issue is occurring due to an interesting edge case involving ESNext and the
useDefineForClassFieldsoption.As an aside and more generally, is there any information on the choice of behavior in this situation regarding the static property retention? I assume it is related to ECMAScript spec compliance regarding initialization ordering but I'm curious if there is any background on the topic.
tchetwin commentedon May 22, 2020
#32011 was reverted by #38719
clydin commentedon May 22, 2020
That’s unfortunate.
The alternative is to reimplement the TS transformer’s wrapping logic separately to support minification against some commonly used tools (terser, etc.). There’s a variety of edge cases that need to be taken into account that make that transformation non-trivial.
The revert also causes the awkward situation where TypeScript’s ES5 output has the potential to be better optimized and smaller than ES2015+ output since the ES5 output is wrapped.
Revert typescript to 3.8 in devDependencies
Revert typescript to 3.8 in devDependencies
Revert typescript to 3.8 in devDependencies
perf(): reenable const class optimization