Webpack-2 recursive tree-shaking/dead code elimination. #2061

Closed
le0nik opened this Issue Feb 17, 2016 · 11 comments

Projects

None yet

9 participants

@le0nik
le0nik commented Feb 17, 2016

Hello, i've been trying out webpack@2.0.7-beta and noticed that webpack doesn't eliminate dead code recursively. Let me show you an example:

import { otherFunc1, otherFunc2 } from './otherModule';

function callOther() {
  otherFunc1();
  otherFunc2();
}

export default function () {
  if (process.env.NODE_ENV !== 'production') {
    callOther();
  }
}

Let's say process.env.NODE_ENV === 'production', so the condition is always false.

  1. I would expect the whole if block in default function to be stripped out.
  2. So callOther would get stripped out.
  3. So otherFunc1 and otherFunc2 would get stripped out(they aren't used anywhere else).

Webpack does steps 1 and 2, but skips step 3, so otherFunc1 and otherFunc2 end up in the bundle anyway.

If i call otherFunc1 and otherFunc2 directly inside the if block, they don't end up in the bundle.

My webpack.config.js:

'use strict';

var webpack = require('webpack');

var env = process.env.NODE_ENV;

var config = {
  module: {
    loaders: [
      { test: /\.js$/, loader: 'babel-loader', exclude: /node_modules/ }
    ]
  },
  output: {
    library: 'bundle',
    libraryTarget: 'umd'
  },
  plugins: [
    new webpack.DefinePlugin({
      'process.env.NODE_ENV': JSON.stringify(env)
    })
  ]
};

module.exports = config;

My .babelrc:

{
  "plugins": [
    ["transform-es2015-template-literals", { "loose": true }],
      "transform-es2015-arrow-functions",
      "transform-es2015-shorthand-properties",
      ["transform-es2015-computed-properties", { "loose": true }],
      "check-es2015-constants",
      ["transform-es2015-spread", { "loose": true }],
      "transform-es2015-parameters",
      ["transform-es2015-destructuring", { "loose": true }],
      "transform-es2015-block-scoping",
      "transform-object-rest-spread",
  ]
}

And i call webpack with:

cross-env NODE_ENV=production webpack -p src/index.js dist/bundle.min.js"

Thanks!

@klimashkin

+1

@maxs15
maxs15 commented Feb 18, 2016

Looks like I have the same issue, was it working in the previous version ?

@le0nik
le0nik commented Feb 18, 2016

@maxs15 no. Just checked versions 2.0.4-beta - 2.0.7-beta. Haven't tried the previous betas, but i doubt it used to work in those.

@cspotcode

@le0nik I think I know why this happens.

As I understand it, webpack does dead code elimination in two steps. First it figures out which exports are being referenced, and it writes the chunk's code, only assigning exports to the exports object if they're "used" somewhere. Then it passes that code to UglifyJS, which removes dead code.

  1. I would expect the whole if block in default function to be stripped out.
  2. So callOther would get stripped out.
  3. So otherFunc1 and otherFunc2 would get stripped out(they aren't used anywhere else).

Numbers 1 and 2 are handled by UglifyJS. Number 3 is handled by webpack. However, since webpack is doing its part before UglifyJS, it doesn't know it can remove those functions.

If i call otherFunc1 and otherFunc2 directly inside the if block, they don't end up in the bundle.

I think you're just getting lucky here because webpack ignores code inside false conditionals. It doesn't, however, remove that code; it waits for Uglify to take care of that.

@dnutels
dnutels commented May 2, 2016 edited

@sokra @cspotcode

To expand on this (sorry if highjacking, as at this moment it seems appropriate), this is the Webpack output, without running -p:

/***/ },
/* 1 */
/***/ function(module, exports, __webpack_require__) {

    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 Bar = function () {
        function Bar() {
            _classCallCheck(this, Bar);
        }

        _createClass(Bar, [{
            key: 'contructor',
            value: function contructor() {
                this.bar = 'classBar';
            }
        }]);

        return Bar;
    }();/* unused harmony export Bar */

for ES6 code that:

foo.js

export class Foo {
    contructor() {
        this.foo = 'classFoo';
    }
}

bar.js

export class Bar {
    contructor() {
        this.bar = 'classBar';
    }
}

Both under some-lib module

and application that consumes the above:

import {Foo} from 'some-lib';

export default class MyFoo extends Foo {
...
}

So Webpack clearly recognizes the export as unused (as expected).

So what is the recommendation to minify it? Uglify doesn't do that for reasons outlined above.

What I am trying to say is - since there is no working minification solution, to say "tree-shaking is working" is kind of misleading, isn't it?

@TheLarkInn
Member

@dnutels @cspotcode @le0nik:

If I understand correctly, Webpack will perform the "tree shaking" part of the equation. Tree shaking creates "dead code" which can be eliminated through a couple plugins (webpack-closure-compiler, UglifyJsPlugin). Also, I believe LoaderOptionsPlugin is the new way to specify to loaders what should be minified/optimized/etc. because of #283 #283 (comment).

Has anyone tested this issue out in the latest beta. I'd like to get this into tracking if this is still a problem.

@dnutels
dnutels commented Jun 28, 2016

@TheLarkInn
Haven't tested this yet, but can tell you that LoaderOptionsPlugin is sort of a unicorn, as I was unable to find any documentation on it aside from this: https://gist.github.com/sokra/27b24881210b56bbaff7#loader-options--minimize

@cspotcode

I have an idea for how this can work. UglifyJS can already do all the dead code elimination we need. No sense re-implementing it when all we need to do is emit the right kind of code for Uglify.

ES6 exports can be thought of as global variables. Imagine fooModule.js does export var foo = 'foo'; and barModule.js does import {foo} from './fooModule'; console.log(foo); You can think about it like a single variable that's written in one place and accessed at the other.

var __webpack_es6_named_export__fooModule__foo__ = 'foo';
console.log(__webpack_es6_named_export__fooModule__foo__);

If webpack emits code that looks like the above, UglifyJS can perform recursive dead code elimination. (or "tree-shaking;" two words for the same thing) Then webpack can rewrite all occurrences of __webpack_es6_named_export__... to be full accesses / assignments on the relevant exports objects. Then UglifyJS can take the result and mangle variable names.

This was referenced Jul 21, 2016
@halt-hammerzeit

So, does webpack include unused imports in the bundle or not?

@TheLarkInn
Member
TheLarkInn commented Sep 2, 2016 edited

It does. Webpack can mark unused imports/exports, (see usedExports property on HarmonyExportSpecifierDependency (I believe)), however it's simply marking the statements and specifiers. Is it possible, maybe, but would be extremely complex and time consuming. We believe that a generic tool should be passed an AST program, to assist in analyzing and optimizing (and used/implemented in Webpack similar to UglifyJsPlugin) that any bundler etc can use. (Like babili, but more than just ES6 I think)

We'd love to have help from Flow, TS, babili, esprima, acorn, uglify, whatever, and framework teams to make one unifying tool to that can provide a High Level general purpose static program flow analysis optimizer/minifier. I know each library, parser has separate needs, but I think there could be a high-level approach at some of this, and benefits everyone.

/cc @hzoo @gaearon @danielrosenwasser @wycats @igorminar @Rich-Harris @aryia

My super super high level thoughts:

Tool takes ES6(or mixed commonjs, etc whatever is possible)AST/Program, tool does optimization, returns either optimized AST Program for respective parsers/compilers/etc to use.

@TheLarkInn
Member

Should move discussion to #2867

@sokra sokra closed this Sep 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment