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

Pure annotation in downlevel emits #13721

Closed
jods4 opened this Issue Jan 27, 2017 · 29 comments

Comments

Projects
None yet
@jods4
Copy link

jods4 commented Jan 27, 2017

Minifiers like Uglify try to remove dead code from minified builds.
To completely remove a declaration, they have to prove that it is side-effect free.
Unfortunately in JS this is often not possible as pretty much anything can have arbitrary effects.

An important issue is that ES5 emit for class can't easily be determined to be side-effect free.
As a result, tree-shaking is ineffective. In simple words: no class is removed from the output, even if it is never used.

A very simple solution could be to have a hint for the minifier, inside a special comment.
If the emit added a /** #pure */ comment at the beginning of ES5 classes it could easily be detected and removed by Uglify.

var V6Engine = /** #pure */(function () {
    function V6Engine() {
    }
    V6Engine.prototype.toString = function () {
        return 'V6';
    };
    return V6Engine;
}());

This is an important enhancement that can dramatically reduce the size of unused code, especially in large libraries like Angular or Aurelia.

Is this something the TS team would consider?
Original discussion in Uglify: mishoo/UglifyJS2#1261

I am aware of #3882 and #7770 and this is not the same.
Those issues are about extending the language to include some "pure" annotations.
This issue is just about adding a hint (comment) inside the emit template.

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Jan 27, 2017

If uglify adds this support we can consider emitting the comment. I am assuming this does not apply to classes with static intializers or decorated ones.
I am also assuming this applies to enums, and namespaces with only "side-effect-free" class declarations

@jods4

This comment has been minimized.

Copy link

jods4 commented Jan 27, 2017

Thanks. We'll see what happens but whether TS would be willing to add the comment is a big factor.

The static initializers and decorators were brought up in the Uglify comments as well.
Currently TS emits those outside the class-defining function, so you wouldn't need to take them into account. This class:

@mammal
class Animal {
    static frob = 24;
}

turns into this:

// The class definition itself is pure
var Animal = /** #pure */(function () {
    function Animal() {
    }
    return Animal;
}());

// Uglify will need to decide what to do with the following (if it can drop them or not)
Animal.frob = 24;
Animal = __decorate([
    mammal
], Animal);

Static initializers and decorators are less common, so that's already a big step forward.

Enums and namespaces are also less common and smaller than classes so they were not discussed AFAIK. I guess anything that could help drop unused code is welcome.

@ysangkok

This comment has been minimized.

Copy link

ysangkok commented Feb 23, 2017

@mhegazy Uglify now merged support for pure statements.

@jods4

This comment has been minimized.

Copy link

jods4 commented Feb 23, 2017

For reference, the implementation details are here:
mishoo/UglifyJS2@1e51586

@mhegazy mhegazy added In Discussion and removed Revisit labels Feb 24, 2017

@kzc

This comment has been minimized.

Copy link

kzc commented Feb 28, 2017

For what it's worth, the pure function call comment annotation feature has been released in uglify-js@2.8.1:

$ echo 'foo(); var a=/*#__PURE__*/(function(){console.log("Hello");}()); bar();' | bin/uglifyjs -c toplevel
WARN: Dropping __PURE__ call [-:1,26]
WARN: Dropping unused variable a [-:1,11]
foo();bar();

fongandrew added a commit to esperco/lutra-redux that referenced this issue Mar 8, 2017

Upgrade to Webpack2.
Also switches TypeScript to emit ES2015 modules instead of CommonJS
so we can take advantage of tree shaking to reduce bundle sizes.
The gains here are barely noticeable though (there's an outstanding
issue with how TypeScript emits classes and how Uglify interacts
with that that may help once that's fixed), but every little bit
helps.

mishoo/UglifyJS2#1261
Microsoft/TypeScript#13721
webpack/webpack#2899

TODO: Look to see if we can convert more of our more imports to
ES2015 to impove tree shaking.
@jods4

This comment has been minimized.

Copy link

jods4 commented Mar 9, 2017

I was musing some more and while you're considering this issue I'd like to try to push it further.

Adding /*#__PURE__*/ in downlevel emit neatly solves the problem of tree-shaking classes in ES3/5.
But decorators are still an issue and unfortunately very common in frameworks like Angular and Aurelia since they provide a very convenient way to attach metadata.

Because it's impossible to prove that decorators are side-effect free (and in the strict sense they most often are not), any class that has a decorator cannot be tree-shaken, no matter if ES5 or ES6.

I think it is possible, with two changes:

  1. By injecting an additional /*#__PURE__*/ before __decorate. The trick is that it can't be always added like in the class codegen, because some decorators might have interesting side-effects.
    My idea is: if there's a #__PURE__ comment before the decorators in TS, add a #__PURE__ comment before the __decorate call.
// Turn this Typescript code:

/*#__PURE__*/
@cacheable
class Frob { }

// Into this ES6 emit:
var Frob = class { };
Frob = /*#__PURE__*/__decorate([cacheable], Frob);
  1. This is unfortunately not enough because Uglify does not have data flow analysis and the multiple assignments/usage of Frob prevents removing. But the emit could easily be changed to the following form with a single assignment:
var Frob = __decorate([cacheable], class {});

And this can be tree-shaken if decorated with a pure comment.

This comment from Uglify team explains the various patterns that work (or not) in more details.


Bonus round:
What would be awesome but more far-fetched is having the /*#__PURE__*/ comment on the decorators themselves and then adding it in front of __decorate if they all have it. That's a lot better from a user perspective but it's also not a local change anymore in TS.

@IgorMinar

This comment has been minimized.

Copy link

IgorMinar commented Mar 30, 2017

I explored this for the use in Angular and can confirm that all downleveled classes previously retained by Uglify are being correctly removed if the IIFEs are prefixed with the annotation.

I'd love to see TS emit the /*@__PURE__*/ annotation by default for all downleveled ES classes.

@RyanCavanaugh

This comment has been minimized.

Copy link
Member

RyanCavanaugh commented Apr 3, 2017

@DanielRosenwasser please follow up with relevant teams to see what we can do

@TheLarkInn

This comment has been minimized.

Copy link
Member

TheLarkInn commented May 11, 2017

This could solve a considerable amount of size bloat problems for angular users attempting to minify their transpiled TS.

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented May 16, 2017

We have some reservations on the use of PURE, since it already has a different meaning in this context, see pure function definition in Wikipedia.

Would /*@__nosideeffects__*/ be an option here?

@kzc

This comment has been minimized.

Copy link

kzc commented May 17, 2017

Would /*@__nosideeffects__*/ be an option here?

Uglify already supports /*@__PURE__*/ in line with its command line option pure_funcs. They are in use in the wild. It would be nice to support a single convention.

Barring that, could you have the typescript compiler allow the user to override the annotation string?

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented May 17, 2017

Uglify already supports /@PURE/ in line with its command line option pure_funcs.

I understand.

It would be nice to support a single convention.

I am saying this convention is wrong; pure functions already have a well defined meanings in the realm of programming languages, compilers and optimizers. using the same term to mean something different is misleading at best.
I am suggesting that uglify supports another annotation as well, let's call it /*@__nosideeffects__*/; since the implementation is already in place this should be rather trivial. they can then deprecate the /*@__PURE__*/ annotation, or not, up to the uglify team; but the TS compiler would only emit the /*@__nosideeffects__*/ which is really the check that is proposed here, making sure that the class has no observable side-effects outside defining a symbol.

@chuckjaz

This comment has been minimized.

Copy link
Contributor

chuckjaz commented May 22, 2017

Not in love with any I have come up with but /** @encapsulatedeffect */ or simply /** @encapsulated */ seem the best assuming the implied definition of encapsulated from my previous comment.

@sandersn

This comment has been minimized.

Copy link
Member

sandersn commented May 26, 2017

Maybe /** @delimitedeffect */ or /** @delimited */? Although that implies that there is some outer scope that delimits the effects, which is not the same as effects-if-referenced.

@mhegazy mhegazy assigned rbuckton and unassigned DanielRosenwasser Jun 8, 2017

@mhegazy mhegazy added this to the TypeScript 2.4.1 milestone Jun 8, 2017

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Jun 8, 2017

We discussed this int he last design meeting, and the conclusion was to use something declarative like /** @class **/. the new comment will be emitted for all classes, regardless of them being decorated or not.

--removeComents would cause the comment to not be emitted.

@rbuckton

This comment has been minimized.

Copy link
Member

rbuckton commented Jun 19, 2017

This small change has a fairly significant impact on our tests. While I'm not opposed to the change, do we want to always emit the /** @class */ annotation or should it be behind a flag?

@rbuckton

This comment has been minimized.

Copy link
Member

rbuckton commented Jun 22, 2017

Fixed, pending PR.

@AlgusDark

This comment has been minimized.

Copy link

AlgusDark commented Jun 24, 2017

@rbuckton I'm having the issue #16727, where typescript removes my manual comment (/*#__PURE__*/). PR #16631 will fix that?

@Koslun

This comment has been minimized.

Copy link

Koslun commented Jun 24, 2017

@AlgusDark I don't think it will improve on the surprising ways the compiler can remove comments, at least not in general.

It will however directly address the underlying feature you're trying to implement. Namely annotating each class so that Uglify can tree-shake it. After PR #16631 lands you can just have to replace /** @class */ with /*#__PURE__*/ to tell Uglify it's safe to remove. Hopefully we'll be able to customize it in Uglify rather than having to create a separate plugin or loader just to do this.

@AlgusDark

This comment has been minimized.

Copy link

AlgusDark commented Jun 24, 2017

@Koslun the problem here is that I'm doing it in a function, not a class transpiled by TypeScript. So I believe that we need to check why compiler is removing that comment at the beginning of the function call, since that's something I want to keep.

My code is in a HOC from React:

const HOC = /*@__PURE__*/withHelpersModifiers(Component)
export default HOC;

@kitsonk kitsonk marked this as a duplicate of #17181 Jul 14, 2017

weswigham added a commit to weswigham/TypeScript that referenced this issue Aug 14, 2017

weswigham added a commit that referenced this issue Aug 14, 2017

weswigham added a commit to weswigham/TypeScript that referenced this issue Aug 14, 2017

@Microsoft Microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.