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

Pure annotation in downlevel emits #13721

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

Pure annotation in downlevel emits #13721

jods4 opened this issue Jan 27, 2017 · 29 comments
Assignees
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@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/UglifyJS#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
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

@mhegazy mhegazy added Suggestion An idea for TypeScript Revisit An issue worth coming back to labels Jan 27, 2017
@jods4
Copy link
Author

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

@mhegazy Uglify now merged support for pure statements.

@jods4
Copy link
Author

jods4 commented Feb 23, 2017

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

@mhegazy mhegazy added In Discussion Not yet reached consensus and removed Revisit An issue worth coming back to labels Feb 24, 2017
@kzc
Copy link

kzc commented Feb 28, 2017

For what it's worth, the pure function call comment annotation feature has been released in [email protected]:

$ 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
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/UglifyJS#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
Copy link
Author

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
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 RyanCavanaugh added Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Apr 3, 2017
@RyanCavanaugh
Copy link
Member

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

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

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

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 rbuckton added the Fixed A PR has been merged for this issue label Jun 22, 2017
@rbuckton
Copy link
Member

Fixed, pending PR.

@AlgusDark
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
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
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 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.
Labels
Committed The team has roadmapped this issue Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests