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

Side effects of unused ES6 classes transpiled to ES5. #10467

Closed
TheLarkInn opened this issue Aug 22, 2016 · 14 comments
Closed

Side effects of unused ES6 classes transpiled to ES5. #10467

TheLarkInn opened this issue Aug 22, 2016 · 14 comments
Labels
Community Tooling This might be better handled by tooling from the community instead of built into TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented.

Comments

@TheLarkInn
Copy link
Member

In the repo link I posted below, it appears that using tools like UglifyJs is not able to 'tree-shake' classes transpiled from TS to target ES5. It appears that the IIFE, is considered a side affect and won't be removed from code even if the export is unused.

Regardless of where this cam be fixed it's important to see this fixed for the 98% of angular2 users (who favor webpack as their bundler). We will be investigating work around for users for now such as TS ES6=ES6 and then babel to transpile to ES5.

References:
webpack/webpack#2899
mishoo/UglifyJS#1261

TypeScript Version: 2.0.0+
Code
https://github.com/blacksonic/typescript-webpack-tree-shaking

Expected behavior:
Either TS removes the module or return it as an empty unused module, or the side affect is not present.
Actual behavior:
The unused export still appears in transpiled code.

@DanielRosenwasser DanielRosenwasser added Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented. In Discussion Not yet reached consensus Community Tooling This might be better handled by tooling from the community instead of built into TypeScript and removed In Discussion Not yet reached consensus labels Aug 22, 2016
@DanielRosenwasser
Copy link
Member

We will be investigating work around for users for now such as TS ES6=ES6 and then babel to transpile to ES5.

Babel also generates an IIFE if I'm not mistaken, right?

@TheLarkInn
Copy link
Member Author

@kitsonk
Copy link
Contributor

kitsonk commented Aug 22, 2016

Once you have a method, Babel reverts to an IIFE, just like TypeScript.

@sonicoder86
Copy link

It is also an issue with Babel

@TheLarkInn
Copy link
Member Author

Thanks for verifying.

@kzc
Copy link

kzc commented Aug 22, 2016

It's more than an IIFE issue. Uglify will not drop code that is assigned to a variable or a property. Uglify doesn't know how exports.Foo will be used as it lacks program flow analysis.

export class Foo {
    foo() {}
}

--->

define(["require", "exports"], function (require, exports) {
    "use strict";
    var Foo = (function () {
        function Foo() {
        }
        Foo.prototype.foo = function () { };
        return Foo;
    }());
    exports.Foo = Foo;
});

@sonicoder86
Copy link

Is it possible to generate a side-effect free ES5 class declaration?

@kitsonk
Copy link
Contributor

kitsonk commented Aug 22, 2016

Is it possible to generate a side-effect free ES5 class declaration?

It doesn't have side effects, it is that Uglify thinks that it might, but the IIFE is there so that code inside has the appropriate scope. There maybe nastier ways to write it (like using new Function) but that won't help Uglify understand that the IIFE doesn't have side effects.

@TheLarkInn
Copy link
Member Author

Thinking of this from a different perspective. Should TS be able to remove unused exports as part of the transpilation process from module ES6/2015 to target ES5?

@kitsonk
Copy link
Contributor

kitsonk commented Aug 30, 2016

How would you propose TypeScript detect unused exports? How would TypeScript know that you intend to publish your TypeScript as a package where all the published (exported) APIs are available and some final build of a project?

Tree shaking of a final bundled build is usually considered something that is the domain of build tooling and the TypeScript Design goals state this as a non-goal:

  1. Provide an end-to-end build pipeline. Instead, make the system extensible so that external tools can use the compiler for more complex build workflows.

For TypeScript 2.1 and the change to the transformation based emitter, it is likely someone would be able to create a transform that either would signal to a tree-shaker related export code that could be eliminated, or potentially build the dependency graph itself and not emit the code, but I suspect it is unlikely we will see something like this baked into TypeScript directly.

@TheLarkInn
Copy link
Member Author

Totally get what you are saying. I just wondered how much program flow analysis could already be leveraged to do this, if even possible?

In regards to the build to/bundling, I am 💯 on board with that decision. Just trying to consider ways to optimize if there is room and ability to.

All of those questions you brought up, are excellent points in why it proves to be not a trivial task.

@paldepind
Copy link

#13721 should solve this issue.

@kzc
Copy link

kzc commented Apr 10, 2017

#13721 should solve this issue.

Yes and no. Uglify would drop unused /*@__PURE__*/ annotated downleveled IIFEs if everything is in the same function scope. It does not address the module exports problem outlined in #10467 (comment) which is also an issue in webpack2.

Something like Rollup would still have to be used to hoist everything into the same function scope, renaming conflicting things as necessary.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 24, 2017

closing in favor of #13721.

@mhegazy mhegazy closed this as completed Aug 24, 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
Community Tooling This might be better handled by tooling from the community instead of built into TypeScript Needs Proposal This issue needs a plan that clarifies the finer details of how it could be implemented.
Projects
None yet
Development

No branches or pull requests

7 participants