Skip to content
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

Consider recognizing /** @class */-annotated IIFEs for dead code removal #2279

Closed
DanielRosenwasser opened this issue Aug 15, 2017 · 10 comments

Comments

@DanielRosenwasser
Copy link

DanielRosenwasser commented Aug 15, 2017

With microsoft/TypeScript#16631 (the PR to solve microsoft/TypeScript#13721), TypeScript now emits a

/** @class */

comment when downleveling classes. Our hope is that Uglify can understand the intent and provide some mechanism to remove unreferenced downleveled classes.

Currently the work is in our RC branch, but depending on when other tools could actually understand these changes, we might postpone to our 2.6 release instead.

CC @TheLarkInn @IgorMinar @chuckjaz @alexeagle @mhegazy

@TheLarkInn
Copy link

Hey Uglify team @kzc et al. Thanks for the continuing support and hard work y'all have been doing with uglifyes. If you are all green light on this feature we'd (webpack) be happy to help in anyway for the implementation of this if needed.

Otherwise keep up the awesome work. 👏👏👏

@kzc
Copy link
Contributor

kzc commented Aug 15, 2017

No uglify change is required. It's trivial for the user to replace /** @class */ with /*@__PURE__*/ with a regex. Likewise for Rollup and Webpack with a plugin.

@TheLarkInn
Copy link

TheLarkInn commented Aug 15, 2017

Would you accept a PR to create a feature which sets an "alternate" label to /@PURE/?

options: {
  pureLabel: /** @class */
}

This way tooling that need a bit more flexibility could do so from uglify, etc. And technically if we needed to we could write a webpack-loader, if last resort. I think from this stance the TS folks want to have an awesome win OOB.

Thoughts? Either way we can run with both options. Thanks for the quick response.

@kzc
Copy link
Contributor

kzc commented Aug 16, 2017

/*@__PURE__*/ annotations are already in use in the wild. I don't see the need for alternate pure annotation names.

uglifyjs-webpack-plugin would be the logical place for the /** @class */ to /*@__PURE__*/ annotation replacement logic. By the same token, the Typescript compiler's annotation could be configurable.

@mhegazy
Copy link

mhegazy commented Aug 16, 2017

We originally considered use of "@PURE", the issue is that it is missleading since the compiler can in no way attest that the class definition is pure (when decorators and static intializers are involved). It is particularly confusing since the term "pure" when used with functions in compilers/minifiers already had a well defined meaning.

"@Class" on the other hand is more declarative and makes the handofff from the transpiler to the minifier simpler, it basically notes that the code originated in an ES6 class, and lets the minifier decide what to do with it.

There is also prior art for "@class" in the jsdoc releam.

I understand that uglify already has an annotation in place with "@pure", and we are not suggesting this support be removed; we would recommend adding "@Class" as an alternative. We also would be happy to help if you are accepting PRs for this proposal.

@kzc
Copy link
Contributor

kzc commented Aug 16, 2017

We originally considered use of "@pure", the issue is that it is missleading since the compiler can in no way attest that the class definition is pure (when decorators and static intializers are involved).

@mhegazy You're missing the point. It is not misleading nor is it relevant if the function call is actually pure. When the user marks a function call with a pure annotation they are instructing uglify to treat it as if it is pure for the purposes of code removal when its return value is unused and the programmer has decided that any potential side effects are not significant to program execution. Uglify has been using this terminology for pure_funcs functionality without issue for years. For example, some uglify users list the babel-generated _classCallCheck as being pure with pure_funcs so those calls can be dropped from ES5 generated code.

Just curious... will the typescript compiler generate /** @enum */ annotated ES5 IIFEs for emums or use /** @class */?

@mhegazy
Copy link

mhegazy commented Aug 16, 2017

The expectation here is that the TypeScript compiler would emit @PURE on the emitted code, egardless of who is going to use it, it reads "pure"; and pure functions are a well established concept. The output is not only consumed by uglify, it can be used by closure, rollup, etc; it can also be inspected by humans and we would like the output to be accurate. It would have been different if the commment had "uglifyjs:pure" but it does not. We found it misleading for the compiler to emit a comment that it knows to be inaccurate.

For enums, TS already has a concept of const enums, that are erased from the output, so this is not relevant here. Namespaces with no statements might be, but we rather stick to standard ES concepts in inter tool communication.

@kzc
Copy link
Contributor

kzc commented Aug 16, 2017

The output is not only consumed by uglify, it can be used by closure, rollup, etc

Both webpack and rollup have string replace plugins that can change the typescript generated annotation to a form that uglify can process. No code changes required.

@alexlamsl
Copy link
Collaborator

@kzc good to close?

@kzc
Copy link
Contributor

kzc commented Aug 21, 2017

yep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants