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

Annotating transformed classes with #__PURE__ comment #6209

Merged
merged 1 commit into from Sep 11, 2017

Conversation

Projects
None yet
7 participants
@Andarist
Member

Andarist commented Sep 7, 2017

Q                       A
Fixed Issues fixes #5632
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? yes
Tests Added/Pass? yes
Spec Compliancy?
License MIT
Doc PR no
Any Dependency Changes? no

This is a simple change which annotate created by transform-es2015-classes IIFEs with #__PURE__ comments. This allows UglifyJS to remove those IIFE calls if their result is unused - generally speaking it allows for better Dead Code Elimination.

The core of the change is here. The rest are only adjusted tests.

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot Sep 7, 2017

Collaborator

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4825/

Collaborator

babel-bot commented Sep 7, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4825/

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Sep 7, 2017

Member

Does UglifyJS remove this?

var A = /*#__PURE__*/ /* foo */ function() { /* ... */ }(X);

Which is the output for

var A = /* foo */ class A extends X {}
Member

nicolo-ribaudo commented Sep 7, 2017

Does UglifyJS remove this?

var A = /*#__PURE__*/ /* foo */ function() { /* ... */ }(X);

Which is the output for

var A = /* foo */ class A extends X {}
@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Sep 7, 2017

Member

Also, maybe this should be enabled using an option since not everyone uses UglifyJS after Babel?

Member

nicolo-ribaudo commented Sep 7, 2017

Also, maybe this should be enabled using an option since not everyone uses UglifyJS after Babel?

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Sep 7, 2017

Member

Does UglifyJS remove this?
var A = /#PURE/ /* foo / function() { / ... */ }(X);

Unfortunately not. They explicitly check only the last comment. I might poke around in the UglifyJS repo as well to make this work, or at least investigate why it doesnt.

Which is the output for
var A = /* foo */ class A extends X {}

Should be easy to add #__PURE__ as the last leading comment instead of putting it as first one. Ofc some other plugin could mess this up again. So the true solution is to work this out in UglifyJS itself. However this is quite an edge case (I dont believe many people use comments like this, also not that many plugins annotating functions like this) and I believe this could stay unchanged.

Also, maybe this should be enabled using an option since not everyone uses UglifyJS after Babel?

Ive also thought about this, however:

  • I believe its better to have opt-out option. The biggest concern for me is that most people are not fully aware of the tools they use and how they work and I believe its better to add such comments by default and provide an option to disable this if somebody doesnt want it. Usage of UglifyJS is so vast in the community that it sounds reasonable to me to emit those annotations by default.
  • I hadnt time to investigate how passing through options from the presets to the plugins should be done ;)
Member

Andarist commented Sep 7, 2017

Does UglifyJS remove this?
var A = /#PURE/ /* foo / function() { / ... */ }(X);

Unfortunately not. They explicitly check only the last comment. I might poke around in the UglifyJS repo as well to make this work, or at least investigate why it doesnt.

Which is the output for
var A = /* foo */ class A extends X {}

Should be easy to add #__PURE__ as the last leading comment instead of putting it as first one. Ofc some other plugin could mess this up again. So the true solution is to work this out in UglifyJS itself. However this is quite an edge case (I dont believe many people use comments like this, also not that many plugins annotating functions like this) and I believe this could stay unchanged.

Also, maybe this should be enabled using an option since not everyone uses UglifyJS after Babel?

Ive also thought about this, however:

  • I believe its better to have opt-out option. The biggest concern for me is that most people are not fully aware of the tools they use and how they work and I believe its better to add such comments by default and provide an option to disable this if somebody doesnt want it. Usage of UglifyJS is so vast in the community that it sounds reasonable to me to emit those annotations by default.
  • I hadnt time to investigate how passing through options from the presets to the plugins should be done ;)
@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Sep 9, 2017

Member

It's fine as a default since it's a comment anyway, I was just confused as to what the comment is supposed to be? I thought it was @class before too @kzc

Member

hzoo commented Sep 9, 2017

It's fine as a default since it's a comment anyway, I was just confused as to what the comment is supposed to be? I thought it was @class before too @kzc

@hzoo

hzoo approved these changes Sep 9, 2017

good, pending review by @kzc or someone on uglify

also cc @boopathi

var Foo = function (_Bar) {
var Foo =
/*#__PURE__*/
function (_Bar) {

This comment has been minimized.

@kzc

kzc Sep 9, 2017

This is not important... but is there a reason why this is on 3 lines instead of 1?

@kzc

kzc Sep 9, 2017

This is not important... but is there a reason why this is on 3 lines instead of 1?

This comment has been minimized.

@Andarist

Andarist Sep 9, 2017

Member

its just how babel-generator spits out code from the AST, looks weird to me too, but 🙄

@Andarist

Andarist Sep 9, 2017

Member

its just how babel-generator spits out code from the AST, looks weird to me too, but 🙄

This comment has been minimized.

@hzoo

hzoo Sep 9, 2017

Member

It's probably just because we removed some logic with newlines using tokens/comments so it's like that now

@hzoo

hzoo Sep 9, 2017

Member

It's probably just because we removed some logic with newlines using tokens/comments so it's like that now

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Sep 9, 2017

Seems fine. I assume @Andarist verified the result against uglify.

Will non-standard static class properties be generated within the IIFE as per #5632 (comment)?

kzc commented Sep 9, 2017

Seems fine. I assume @Andarist verified the result against uglify.

Will non-standard static class properties be generated within the IIFE as per #5632 (comment)?

@xtuc

xtuc approved these changes Sep 9, 2017

Lgtm 👍

@@ -2,6 +2,8 @@ import LooseTransformer from "./loose";
import VanillaTransformer from "./vanilla";
import nameFunction from "babel-helper-function-name";
const PURE_ANNOTATION = "#__PURE__";

This comment has been minimized.

@xtuc

xtuc Sep 9, 2017

Member

We could use this across multiple plugins? We might want to add it in a helper.

@xtuc

xtuc Sep 9, 2017

Member

We could use this across multiple plugins? We might want to add it in a helper.

This comment has been minimized.

@Andarist

Andarist Sep 9, 2017

Member

@xtuc sure thing, whats the best place to add such?

@Andarist

Andarist Sep 9, 2017

Member

@xtuc sure thing, whats the best place to add such?

This comment has been minimized.

@xtuc

xtuc Sep 11, 2017

Member

Too late but I had something like babel-help-emit-pure-annotation in mind but I'm not sure it will be used apart from here.

@xtuc

xtuc Sep 11, 2017

Member

Too late but I had something like babel-help-emit-pure-annotation in mind but I'm not sure it will be used apart from here.

This comment has been minimized.

@hzoo

hzoo Sep 11, 2017

Member

I would just make it when we need/get to it. It's not a breaking change to swap it out either

@hzoo

hzoo Sep 11, 2017

Member

I would just make it when we need/get to it. It's not a breaking change to swap it out either

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Sep 9, 2017

Member

Will non-standard static class properties be generated within the IIFE as per #5632 (comment)?

I plan to work on it too, need to establish how it should be done though. The problem I see is that those 2 plugins are independent but they need to work together here somehow. Ideally also we could have 2 different outputs for:

  • transpiled classes - just pull statics into the closure
  • only transpiled statics - wrap class + statics into the closure
Member

Andarist commented Sep 9, 2017

Will non-standard static class properties be generated within the IIFE as per #5632 (comment)?

I plan to work on it too, need to establish how it should be done though. The problem I see is that those 2 plugins are independent but they need to work together here somehow. Ideally also we could have 2 different outputs for:

  • transpiled classes - just pull statics into the closure
  • only transpiled statics - wrap class + statics into the closure
@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Sep 9, 2017

Member

It's fine as a default since it's a comment anyway, I was just confused as to what the comment is supposed to be? I thought it was @class before too @kzc

It seems that @class is emitted by the TS compiler and they refuse to use the #__PURE__ comment which is understandable by the UglifyJS - they propose that people use replacement plugins from @class to #__PURE__ comments i.e. with webpack plugin.

Member

Andarist commented Sep 9, 2017

It's fine as a default since it's a comment anyway, I was just confused as to what the comment is supposed to be? I thought it was @class before too @kzc

It seems that @class is emitted by the TS compiler and they refuse to use the #__PURE__ comment which is understandable by the UglifyJS - they propose that people use replacement plugins from @class to #__PURE__ comments i.e. with webpack plugin.

@kzc

This comment has been minimized.

Show comment
Hide comment
@kzc

kzc Sep 9, 2017

Will non-standard static class properties be generated within the IIFE as per #5632 (comment) ?

The problem I see is that those 2 plugins are independent but they need to work together here somehow.

I ran into the same issue when hacking together the proof of concept. I did not know how Babel plugins are supposed to communicate/coordinate with each other - via Babylon AST only?

Maybe just eliminate babel-plugin-transform-class-properties altogether and roll that functionality into some future version of babel-plugin-transform-es201X-classes?

kzc commented Sep 9, 2017

Will non-standard static class properties be generated within the IIFE as per #5632 (comment) ?

The problem I see is that those 2 plugins are independent but they need to work together here somehow.

I ran into the same issue when hacking together the proof of concept. I did not know how Babel plugins are supposed to communicate/coordinate with each other - via Babylon AST only?

Maybe just eliminate babel-plugin-transform-class-properties altogether and roll that functionality into some future version of babel-plugin-transform-es201X-classes?

@hzoo hzoo merged commit c47258d into babel:master Sep 11, 2017

4 checks passed

babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 86.15% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Andarist Andarist deleted the Andarist:es6-classes-pure-iifes branch Sep 11, 2017

@hzoo hzoo referenced this pull request Sep 16, 2017

Merged

Babel 7 #23966

2 of 2 tasks complete

zebulonj added a commit to tedconf/conjunction that referenced this pull request Feb 16, 2018

Annotate pure calls in distribution; drop incorrectly used 'browser' …
…field from package definition.

Annotation of pure calls aids UglifyJS in dead code removal (tree shaking). This is a step forward, but until static class properties and methods are better handled by Babel, we're not reaching our full potential for tree-shaking (see babel/babel#6209 (comment)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment