Annotating transformed classes with #__PURE__ comment by Andarist · Pull Request #6209 · babel/babel

Andarist · 2017-09-07T15:08:32Z

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 · 2017-09-07T15:19:40Z

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

nicolo-ribaudo · 2017-09-07T15:43:47Z

Does UglifyJS remove this?

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

Which is the output for

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

nicolo-ribaudo · 2017-09-07T15:51:55Z

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

Andarist · 2017-09-07T17:40:13Z

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 · 2017-09-09T03:16:57Z

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

good, pending review by @kzc or someone on uglify

also cc @boopathi

kzc · 2017-09-09T04:12:05Z

packages/babel-core/test/fixtures/transformation/misc/regression-1155/expected.js

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


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

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

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

kzc · 2017-09-09T04:26:41Z

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

Lgtm 👍

xtuc · 2017-09-09T08:00:13Z

packages/babel-plugin-transform-es2015-classes/src/index.js

 import VanillaTransformer from "./vanilla";
 import nameFunction from "babel-helper-function-name";

+const PURE_ANNOTATION = "#__PURE__";


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

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

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.

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

Andarist · 2017-09-09T08:17:24Z

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 · 2017-09-09T08:19:19Z

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 · 2017-09-09T15:11:19Z

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?

carrickjason · 2018-10-17T19:08:28Z

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 Do you know if this got solved anywhere? I'm still seeing class properties outside of the IIFE which is breaking treeshaking.

Andarist · 2018-10-18T08:28:19Z

You could try this https://github.com/emotion-js/emotion/blob/64a39f1eb6abf5afe6949b702436423938f34fd0/scripts/build/fix-dce-for-classes-with-statics.js , just keep in mind that this might break your code - it's not overly tested 😉

I unfortunately couldn't find time yet to revive my old PR to fix this issue, I was hoping that this PR would get merged in #8130 which would the whole WAY easier.

Annotating transformed classes with #__PURE__ comment

fa30ff9

jridgewell approved these changes Sep 8, 2017

View reviewed changes

hzoo mentioned this pull request Sep 9, 2017

Pure annotation in downlevel emits #5632

Closed

hzoo approved these changes Sep 9, 2017

View reviewed changes

kzc reviewed Sep 9, 2017

View reviewed changes

xtuc approved these changes Sep 9, 2017

View reviewed changes

hzoo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Sep 11, 2017

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

Andarist deleted the es6-classes-pure-iifes branch September 11, 2017 15:56

hzoo mentioned this pull request Sep 16, 2017

Babel 7 twbs/bootstrap#23966

Merged

2 tasks

pshrmn mentioned this pull request Sep 17, 2017

(Don't Merge) Expect ReactRouterDOM as global in react-router-config UMD remix-run/react-router#5344

Closed

qgustavor mentioned this pull request Nov 14, 2017

Babel classes prevent tree shaking/minification qgustavor/mega#16

Closed

NMinhNguyen mentioned this pull request Apr 19, 2018

Support @nosideeffects all code (not just externs) google/closure-compiler#2256

Closed

natew mentioned this pull request May 25, 2018

proposal-class-properties doesn't output PURE annotation (7 beta.47) #8043

Open

nicolo-ribaudo mentioned this pull request Jun 6, 2018

Default exported class is not being tree shook when using webpack and babel-loader. It's tree-shaking correctly if we disable the babel-loader or exclude:['transform-es2015-classes'] in env preset #8101

Closed

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019

Uh oh!

Conversation

Andarist commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo commented Sep 7, 2017

Uh oh!

nicolo-ribaudo commented Sep 7, 2017

Uh oh!

Andarist commented Sep 7, 2017

Uh oh!

hzoo commented Sep 9, 2017

Uh oh!

hzoo left a comment

Choose a reason for hiding this comment

Uh oh!

kzc Sep 9, 2017

Choose a reason for hiding this comment

Uh oh!

Andarist Sep 9, 2017

Choose a reason for hiding this comment

Uh oh!

hzoo Sep 9, 2017

Choose a reason for hiding this comment

Uh oh!

kzc commented Sep 9, 2017

Uh oh!

xtuc left a comment

Choose a reason for hiding this comment

Uh oh!

xtuc Sep 9, 2017

Choose a reason for hiding this comment

Uh oh!

Andarist Sep 9, 2017

Choose a reason for hiding this comment

Uh oh!

xtuc Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

hzoo Sep 11, 2017

Choose a reason for hiding this comment

Uh oh!

Andarist commented Sep 9, 2017

Uh oh!

Andarist commented Sep 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kzc commented Sep 9, 2017

Uh oh!

carrickjason commented Oct 17, 2018

Uh oh!

Andarist commented Oct 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Andarist commented Sep 7, 2017 •

edited

Loading

babel-bot commented Sep 7, 2017 •

edited

Loading

Andarist commented Sep 9, 2017 •

edited

Loading