Skip to content

Static field emit is IIFE #38528

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

Closed
robpalme opened this issue May 13, 2020 · 13 comments
Closed

Static field emit is IIFE #38528

robpalme opened this issue May 13, 2020 · 13 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@robpalme
Copy link

robpalme commented May 13, 2020

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:

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 @evmar for discovering this issue.

@robpalme
Copy link
Author

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
Copy link
Author

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
Copy link

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
Copy link
Author

Thanks for highlighting this @rrdelaney. I think these two issues are related and orthogonal.

@rrdelaney
Copy link

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
Copy link
Contributor

clydin commented 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
Copy link

rrdelaney commented May 14, 2020

Only emitting for downleveled code wouldn't solve Google's issue, we're still using ES2015 emit.

@clydin
Copy link
Contributor

clydin commented May 15, 2020

The problem demonstrated in this issue only represents when targeting ESNext and the useDefineForClassFields option 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
Copy link

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
Copy link

rrdelaney commented 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
Copy link
Contributor

clydin commented 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 useDefineForClassFields option.

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
Copy link

#32011 was reverted by #38719

@clydin
Copy link
Contributor

clydin commented 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.

jingyu9575 added a commit to jingyu9575/multithreaded-download-manager that referenced this issue May 28, 2020
jingyu9575 added a commit to jingyu9575/multithreaded-download-manager that referenced this issue May 28, 2020
jingyu9575 added a commit to jingyu9575/multithreaded-download-manager that referenced this issue May 28, 2020
@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants