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 #5632

Closed
sonicoder86 opened this issue Apr 14, 2017 · 12 comments · Fixed by #6209
Closed

Pure annotation in downlevel emits #5632

sonicoder86 opened this issue Apr 14, 2017 · 12 comments · Fixed by #6209
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories

Comments

@sonicoder86
Copy link

Feature request

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.
Uglify|JS already supports this annotation: mishoo/UglifyJS#1448
Original UglifyJS discussion: mishoo/UglifyJS#1261
For reference, implementation details are here: mishoo/UglifyJS@1e51586

The issue was originally raised at Webpack: webpack/webpack#2867

@babel-bot
Copy link
Collaborator

Hey @BlackSonic! We really appreciate you taking the time to report an issue. The collaborators
on this project attempt to help as many people as possible, but we're a limited number of volunteers,
so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack
community that typically always has someone willing to help. You can sign-up here
for an invite.

@hzoo
Copy link
Member

hzoo commented Apr 14, 2017

Hey @BlackSonic, thanks for the issue! It looks like you already made this issue in #3821 😄 ? We should close one of them

@sonicoder86
Copy link
Author

@hzoo I would close the older one as this one is more detailed and links to UglifyJS resources.

@hzoo hzoo added PR: New Feature 🚀 A type of pull request used for our changelog categories and removed i: duplicate labels Apr 15, 2017
@kzc
Copy link

kzc commented Apr 30, 2017

@BlackSonic Can you try out this naïve babel plugin?

It appears to work well enough to drop unused down-levelled classes.

$ cat PureClassExpression.js 
"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});

exports["default"] = function () {
  return {
    visitor: {
      ClassExpression: function ClassExpression(path) {
        path.addComment("leading", "#__PURE__");
      }
    }
  };
};

module.exports = exports["default"];
$ echo 'class A{run(){}} class B{jump(){}} f(new A);' | babel --presets env --plugins ./PureClassExpression.js
"use strict";

var _createClass = function () { function defineProperties(target, props) { for (var i = 0; i < props.length; i  ) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } } return function (Constructor, protoProps, staticProps) { if (protoProps) defineProperties(Constructor.prototype, protoProps); if (staticProps) defineProperties(Constructor, staticProps); return Constructor; }; }();

function _classCallCheck(instance, Constructor) { if (!(instance instanceof Constructor)) { throw new TypeError("Cannot call a class as a function"); } }

var A = /*#__PURE__*/function () {
  function A() {
    _classCallCheck(this, A);
  }

  _createClass(A, [{
    key: "run",
    value: function run() {}
  }]);

  return A;
}();

var B = /*#__PURE__*/function () {
  function B() {
    _classCallCheck(this, B);
  }

  _createClass(B, [{
    key: "jump",
    value: function jump() {}
  }]);

  return B;
}();

f(new A());
$ uglifyjs -V
uglify-js 2.8.22
$ echo 'class A{run(){}} class B{jump(){}} f(new A);' | babel --presets env --plugins ./PureClassExpression.js | uglifyjs -c toplevel,passes=3 -b
WARN: Dropping __PURE__ call [-:20,21]
WARN: Dropping unused variable B [-:20,4]
"use strict";

function _classCallCheck(instance, Constructor) {
    if (!(instance instanceof Constructor)) throw new TypeError("Cannot call a class as a function");
}

var _createClass = function() {
    function defineProperties(target, props) {
        for (var i = 0; i < props.length; i  ) {
            var descriptor = props[i];
            descriptor.enumerable = descriptor.enumerable || !1, descriptor.configurable = !0, 
            "value" in descriptor && (descriptor.writable = !0), Object.defineProperty(target, descriptor.key, descriptor);
        }
    }
    return function(Constructor, protoProps, staticProps) {
        return protoProps && defineProperties(Constructor.prototype, protoProps), staticProps && defineProperties(Constructor, staticProps), 
        Constructor;
    };
}(), A = function() {
    function A() {
        _classCallCheck(this, A);
    }
    return _createClass(A, [ {
        key: "run",
        value: function() {}
    } ]), A;
}();

f(new A());

@sonicoder86
Copy link
Author

@kzc It works perfectly with your plugin!
https://github.com/blacksonic/babel-webpack-tree-shaking/tree/pure

@kzc
Copy link

kzc commented May 1, 2017

/cc @sokra @TheLarkInn

@kzc
Copy link

kzc commented May 5, 2017

Static class properties prevent unused ES5 generated classes from being dropped by uglify due to the fact that they are generated outside the IIFE:

$ echo 'class Bar{} class Foo extends Bar { p = "prop"; static sp1 = 1; static sp2 = 2; baz(){quux()} }' | node_modules/.bin/babel --presets env --plugins babel-plugin-transform-class-properties,./PureClassExpression.js

var Bar = /*#__PURE__*/function Bar() {
  _classCallCheck(this, Bar);
};

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

  function Foo() {
    var _ref;

    var _temp, _this, _ret;

    _classCallCheck(this, Foo);

    for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key  ) {
      args[_key] = arguments[_key];
    }

    return _ret = (_temp = (_this = _possibleConstructorReturn(this, (_ref = Foo.__proto__ || Object.getPrototypeOf(Foo)).call.apply(_ref, [this].concat(args))), _this), _this.p = "prop", _temp), _possibleConstructorReturn(_this, _ret);
  }

  _createClass(Foo, [{
    key: "baz",
    value: function baz() {
      quux();
    }
  }]);

  return Foo;
}(Bar);

Foo.sp1 = 1;    // <-- prevents uglify from dropping unused class IIFE
Foo.sp2 = 2;    // <-- prevents uglify from dropping unused class IIFE

With the following (admittedly awful and likely incorrect) proof-of-concept hack, these static class properties will be generated within the IIFE so that uglify will be able to drop the generated class:

diff -u -r node_modules.orig node_modules
diff -u -r node_modules.orig/babel-plugin-transform-class-properties/lib/index.js node_modules/babel-plugin-transform-class-properties/lib/index.js
--- node_modules.orig/babel-plugin-transform-class-properties/lib/index.js	2017-05-05 13:53:24.000000000 -0400
    node_modules/babel-plugin-transform-class-properties/lib/index.js	2017-05-05 13:37:35.000000000 -0400
@@ -221,7  221,7 @@
           }
         }
 
-        path.insertAfter(nodes);
         path.node.__static_props = nodes;
       },
       ArrowFunctionExpression: function ArrowFunctionExpression(path) {
         var classExp = path.get("body");
diff -u -r node_modules.orig/babel-plugin-transform-es2015-classes/lib/vanilla.js node_modules/babel-plugin-transform-es2015-classes/lib/vanilla.js
--- node_modules.orig/babel-plugin-transform-es2015-classes/lib/vanilla.js	2017-05-05 13:53:29.000000000 -0400
    node_modules/babel-plugin-transform-es2015-classes/lib/vanilla.js	2017-05-05 13:56:04.000000000 -0400
@@ -152,6  152,11 @@
       if (body.length === 1) return t.toExpression(body[0]);
     }
 
     for (var i = 0, arr = this.path.node.__static_props; arr && i < arr.length;   i) {
         body.push(arr[i]);
     }
     this.path.node.__static_props = null;
 
     body.push(t.returnStatement(this.classRef));
 
     var container = t.functionExpression(null, closureParams, t.blockStatement(body));

With the hacked version of babel notice that the generated static properties were moved into the IIFE:

echo 'class Bar{} class Foo extends Bar { p = "prop"; static sp1 = 1; static sp2 = 2; baz(){quux()} }' | node_modules/.bin/babel --presets env --plugins babel-plugin-transform-class-properties,./PureClassExpression.js

var Bar = /*#__PURE__*/function Bar() {
  _classCallCheck(this, Bar);
};

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

  function Foo() {
    var _ref;

    var _temp, _this, _ret;

    _classCallCheck(this, Foo);

    for (var _len = arguments.length, args = Array(_len), _key = 0; _key < _len; _key  ) {
      args[_key] = arguments[_key];
    }

    return _ret = (_temp = (_this = _possibleConstructorReturn(this, (_ref = Foo.__proto__ || Object.getPrototypeOf(Foo)).call.apply(_ref, [this].concat(args))), _this), _this.p = "prop", _temp), _possibleConstructorReturn(_this, _ret);
  }

  _createClass(Foo, [{
    key: "baz",
    value: function baz() {
      quux();
    }
  }]);

  Foo.sp1 = 1;    // <-- Note: static properties were moved into the IIFE
  Foo.sp2 = 2;    // <-- Note: static properties were moved into the IIFE
  return Foo;
}(Bar);

which allows uglify to drop the unused annotated class IIFEs:

$ echo 'class Bar{} class Foo extends Bar { p = "prop"; static sp1 = 1; static sp2 = 2; baz(){quux()} }' | node_modules/.bin/babel --presets env --plugins babel-plugin-transform-class-properties,./PureClassExpression.js | node_modules/.bin/uglifyjs -c toplevel,passes=3
WARN: Side effects in initialization of unused variable _createClass [-:3,4]
WARN: Dropping __PURE__ call [-:15,23]
WARN: Dropping unused variable Foo [-:15,4]
WARN: Dropping unused function defineProperties [-:3,42]
WARN: Dropping side-effect-free statement [-:3,0]
"use strict";

@priyajeet
Copy link

priyajeet commented May 5, 2017

There is also the common use-case when folks are using react stateless components, so no class expressions. They can be made/transformed into it an IIFE to include random stuff like .displayName to remove side effects.

const Comp = ({ foo = 'bar' }) => <span>{foo}</span>;
Comp.displayName = 'Comp';
Comp.propTypes = { foo: PropTypes.string }; // Eliminated by using flowTypes instead
export default Comp;

to something like

const Comp = function(){
  const Comp = ({ foo = 'bar' }) => <span>{foo}</span>;
  Comp.displayName = 'Comp';
  Comp.propTypes = { foo: PropTypes.string }; // Eliminated by using flowTypes instead
  return Comp;
}();
export default Comp;

But the above also seems to have side effects and uglifyJS doesn't seem to eliminate, I had to manually add /*#__PURE__*/ in front of the function declaration to get rid of it.

@Andarist
Copy link
Member

@priyajeet dont use explicit .displayName, its a legacy - nowadays inferred names in the browsers should do that for you by providing .name on the functions and classes (even those anonymous assigned to variable).

The only valid use case for .displayName is while implementing a HOC (Higher Order Component) when you want to to wrap wrapped component's name with your own - still wrapped component's name should be access like this:

const wrappedDisplayName = WrappedComponent.displayName || WrappedComponent.name || 'Unknown'

So it will gracefully use inferred name for regular components.

Still the problem persists because of the context and defaultProps (which both are less popular so the problem is smaller, but it doesnt mean its non-existent).

@hzoo
Is there anything I could do to push this forward? I think its extremely important and would like to contribute. I would even split this issue in at least 2 - pure annotations for classes and dragging statics into the closure, let me start the ball rolling - #5902

@taion
Copy link
Contributor

taion commented Jun 29, 2017

displayName remains relevant for showing useful component names in minified code. It's not great to see React dev tools full of <t> or <b> or whatever.

@Andarist
Copy link
Member

@taion hmm, I thin that would be better served by an integration between devtools and sourcemaps

@hzoo
Copy link
Member

hzoo commented Sep 9, 2017

PR is #6209

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 4, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants