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

Tree shaking not working for functions executed in JS file when compared with Rollup #4784

Open
ankurp opened this Issue Apr 26, 2017 · 14 comments

Comments

Projects
None yet
9 participants
@ankurp

ankurp commented Apr 26, 2017

Do you want to request a feature or report a bug?

This is a bug where the same project can tree shake and remove unused code when bundled with Rollup but it does not when bundle is generated with webpack 2. To replicate and give an example I have created a repo to replicate this bug https://github.com/ankurp/rollup-webpack-test/

What is the current behavior?

Webpack 2 should remove code that is not used even if a function or class defined in a JS file gets executed. For example the following code below should remove the square function and the getSquare function which it does when bundled with Rollup but when bundling with Webpack 2 it includes getSquare because that function is invoked in the math.js file. This is the wrong behavior when compared with rollup which removes that unused function as well.

// math.js
function getSquare() { return x => x * x; }
export const square = getSquare();
export const cube = x => x * x * x;

// index.js
import { cube } from './math';
console.log(cube(3));

If the current behavior is a bug, please provide the steps to reproduce.

Best way to reproduce is to run yarn and then yarn build in this repo https://github.com/ankurp/rollup-webpack-test/

The repo has three basic JS files to replicate this bug and also generates both rollup and webpack output for comparison with optimized compressed builds on which tree shaking is performed.

What is the expected behavior?

Webpack 2 should remove getSquare function from the bundle as it is not imported or used.

If this is a feature request, what is motivation or use case for changing the behavior?

This is a bug that needs to be fixed so that the tree shaking feature works as expected.

Please mention other relevant information such as the browser version, Node.js version, webpack version and Operating System.

This was run using webpack 2.

@mbilokonsky

This comment has been minimized.

Show comment
Hide comment
@mbilokonsky

mbilokonsky Apr 26, 2017

paging @TheLarkInn who knows everything about webpack.

mbilokonsky commented Apr 26, 2017

paging @TheLarkInn who knows everything about webpack.

@jouni-kantola

This comment has been minimized.

Show comment
Hide comment
@jouni-kantola

jouni-kantola Apr 26, 2017

@ankurp: I added a manifest chunk to strip out the webpack runtime. When I run your example with webpack 2.2.0 (as you use in your repo), I get the following output, which represents the bug you've reported:

webpackJsonp(
  [0, 1],
  [
    function(n, t, r) {
      "use strict";
      r(1);
      r.d(t, "a", function() {
        return u;
      });
      var u = function() {
        return 1;
      };
    },
    function(n, t, r) {
      "use strict";
      function u() {
        return function(n) {
          return n * n;
        };
      }
      var e = (u(), function(n) {
        return n * n * n;
      });
      e(3);
    },
    function(n, t, r) {
      "use strict";
      Object.defineProperty(t, "__esModule", { value: !0 });
      var u = r(0);
      console.log(r.i(u.a)());
    }
  ],
  [2]
);

With webpack 2.4.1, I get the following output:

webpackJsonp(
  [0],
  [
    function(n, t, e) {
      "use strict";
      e(2);
      e.d(t, "a", function() {
        return u;
      });
      var u = function() {
        return 1;
      };
    },
    function(n, t, e) {
      "use strict";
      Object.defineProperty(t, "__esModule", { value: !0 });
      var u = e(0);
      console.log(e.i(u.a)());
    },
    function(n, t, e) {
      "use strict";
    }
  ],
  [1]
);

Update to the latest version of webpack and have another go.

With that said, rollup's generated bundle contain less even with 2.4.1, so there is definitely room for improvement.

jouni-kantola commented Apr 26, 2017

@ankurp: I added a manifest chunk to strip out the webpack runtime. When I run your example with webpack 2.2.0 (as you use in your repo), I get the following output, which represents the bug you've reported:

webpackJsonp(
  [0, 1],
  [
    function(n, t, r) {
      "use strict";
      r(1);
      r.d(t, "a", function() {
        return u;
      });
      var u = function() {
        return 1;
      };
    },
    function(n, t, r) {
      "use strict";
      function u() {
        return function(n) {
          return n * n;
        };
      }
      var e = (u(), function(n) {
        return n * n * n;
      });
      e(3);
    },
    function(n, t, r) {
      "use strict";
      Object.defineProperty(t, "__esModule", { value: !0 });
      var u = r(0);
      console.log(r.i(u.a)());
    }
  ],
  [2]
);

With webpack 2.4.1, I get the following output:

webpackJsonp(
  [0],
  [
    function(n, t, e) {
      "use strict";
      e(2);
      e.d(t, "a", function() {
        return u;
      });
      var u = function() {
        return 1;
      };
    },
    function(n, t, e) {
      "use strict";
      Object.defineProperty(t, "__esModule", { value: !0 });
      var u = e(0);
      console.log(e.i(u.a)());
    },
    function(n, t, e) {
      "use strict";
    }
  ],
  [1]
);

Update to the latest version of webpack and have another go.

With that said, rollup's generated bundle contain less even with 2.4.1, so there is definitely room for improvement.

@ankurp

This comment has been minimized.

Show comment
Hide comment
@ankurp

ankurp Apr 27, 2017

ankurp commented Apr 27, 2017

@ankurp

This comment has been minimized.

Show comment
Hide comment
@ankurp

ankurp May 3, 2017

This is not fixed 100%. I see that in my example it was able to remove the code but when I use it in a react app with one component it is not able to tree shake. I will update the example as it is still an issue.

ankurp commented May 3, 2017

This is not fixed 100%. I see that in my example it was able to remove the code but when I use it in a react app with one component it is not able to tree shake. I will update the example as it is still an issue.

@ankurp

This comment has been minimized.

Show comment
Hide comment
@ankurp

ankurp May 3, 2017

@jouni-kantola I updated the example in my repo and now exporting a react component https://github.com/ankurp/rollup-webpack-test/blob/master/app/lib.js#L2 and I see the ReactComponent does not get removed even when it is not used at all in the index.js which is the entry point https://github.com/ankurp/rollup-webpack-test/blob/master/app/index.js

ankurp commented May 3, 2017

@jouni-kantola I updated the example in my repo and now exporting a react component https://github.com/ankurp/rollup-webpack-test/blob/master/app/lib.js#L2 and I see the ReactComponent does not get removed even when it is not used at all in the index.js which is the entry point https://github.com/ankurp/rollup-webpack-test/blob/master/app/index.js

@ankurp

This comment has been minimized.

Show comment
Hide comment
@ankurp

ankurp May 3, 2017

@TheLarkInn any help regarding this would be appreciated as I am using the latest version of webpack. If this is a known issue please let us know as well so we can consolidate the issues.

ankurp commented May 3, 2017

@TheLarkInn any help regarding this would be appreciated as I am using the latest version of webpack. If this is a known issue please let us know as well so we can consolidate the issues.

@ankurp

This comment has been minimized.

Show comment
Hide comment
@ankurp

ankurp May 3, 2017

One other thing I noticed is that when you extend from React.Component, unused code does not get removed. But if I use stateless component then the component code is removed as it is not used.

// Does not perform treeshaking on this code
import React from 'react';
export class ReactComponent extends React.Component {
  render() {
    return <h1>Hello, {this.props.name}</h1>;
  }
}
export default ReactComponent;

// Treeshaking works if I use stateless component as such
import React from 'react';
export const ReactComponent = (props) => {
  return <h1>Hello, {props.name}</h1>;
}
export default ReactComponent;

ankurp commented May 3, 2017

One other thing I noticed is that when you extend from React.Component, unused code does not get removed. But if I use stateless component then the component code is removed as it is not used.

// Does not perform treeshaking on this code
import React from 'react';
export class ReactComponent extends React.Component {
  render() {
    return <h1>Hello, {this.props.name}</h1>;
  }
}
export default ReactComponent;

// Treeshaking works if I use stateless component as such
import React from 'react';
export const ReactComponent = (props) => {
  return <h1>Hello, {props.name}</h1>;
}
export default ReactComponent;
@jouni-kantola

This comment has been minimized.

Show comment
Hide comment
@jouni-kantola

jouni-kantola May 3, 2017

Now that you're using a class instead these issues come into play:
webpack: #4080
uglify: mishoo/UglifyJS2#1261

jouni-kantola commented May 3, 2017

Now that you're using a class instead these issues come into play:
webpack: #4080
uglify: mishoo/UglifyJS2#1261

@markbrocato

This comment has been minimized.

Show comment
Hide comment
@markbrocato

markbrocato May 31, 2017

Here is another very simple example which illustrates the bug:

https://github.com/markbrocato/webpack-tree-shaking-exports

The presence of any function call in an export's declaration results in webpack being unable to shake it out of the tree.

markbrocato commented May 31, 2017

Here is another very simple example which illustrates the bug:

https://github.com/markbrocato/webpack-tree-shaking-exports

The presence of any function call in an export's declaration results in webpack being unable to shake it out of the tree.

@dmitriid

This comment has been minimized.

Show comment
Hide comment
@dmitriid

dmitriid Sep 15, 2017

The bug is still there, another simple example https://github.com/dmitriid/webpack-dec-tree-shaking

dmitriid commented Sep 15, 2017

The bug is still there, another simple example https://github.com/dmitriid/webpack-dec-tree-shaking

@istiti

This comment has been minimized.

Show comment
Hide comment
@istiti

istiti Oct 24, 2017

+++++++++++++ 1

@TheLarkInn

istiti commented Oct 24, 2017

+++++++++++++ 1

@TheLarkInn

@xiachaoxulu

This comment has been minimized.

Show comment
Hide comment
@xiachaoxulu

xiachaoxulu Nov 16, 2017

so, how to do?

xiachaoxulu commented Nov 16, 2017

so, how to do?

@DevSide

This comment has been minimized.

Show comment
Hide comment
@DevSide

DevSide Dec 8, 2017

I use explicit /*#__PURE__*/ as a workaround.

const _square = /*#__PURE__*/ function getSquare () { return x => x * x; }()
export const square = _square

DevSide commented Dec 8, 2017

I use explicit /*#__PURE__*/ as a workaround.

const _square = /*#__PURE__*/ function getSquare () { return x => x * x; }()
export const square = _square
@edmorley

This comment has been minimized.

Show comment
Hide comment
@edmorley

edmorley Mar 13, 2018

Contributor

This works now for me with webpack 4.

ie:

  1. yarn add webpack webpack-cli
  2. Create these files:
// src/index.js
import { cube } from './math';
console.log(cube(3));
// src/math.js
function getSquare() { return x => x * x; }
export const square = getSquare();
export const cube = x => x * x * x;
// webpack.config.js
const UglifyJsPlugin = require("uglifyjs-webpack-plugin");

module.exports = {
  mode: 'production',
  optimization: {
    minimizer: [
      new UglifyJsPlugin({
        uglifyOptions: {
          warnings: true,
          mangle: false,
          output: {
            beautify: true,
            comments: true
          }
        }
      })
    ]
  }
};
  1. Run yarn webpack

The command output includes:

Dropping unused variable square [main.js:81,6]

And dist/main.js has contents:

/******/ !function(modules) {
    // webpackBootstrap
    // ...
/************************************************************************/
/******/ ([ 
/* 0 */
/***/ function(module, __webpack_exports__, __webpack_require__) {
    "use strict";
    __webpack_require__.r(__webpack_exports__);
    // CONCATENATED MODULE: ./src/index.js
    console.log((x => x * x * x)(3));
}
/******/ ]);
Contributor

edmorley commented Mar 13, 2018

This works now for me with webpack 4.

ie:

  1. yarn add webpack webpack-cli
  2. Create these files:
// src/index.js
import { cube } from './math';
console.log(cube(3));
// src/math.js
function getSquare() { return x => x * x; }
export const square = getSquare();
export const cube = x => x * x * x;
// webpack.config.js
const UglifyJsPlugin = require("uglifyjs-webpack-plugin");

module.exports = {
  mode: 'production',
  optimization: {
    minimizer: [
      new UglifyJsPlugin({
        uglifyOptions: {
          warnings: true,
          mangle: false,
          output: {
            beautify: true,
            comments: true
          }
        }
      })
    ]
  }
};
  1. Run yarn webpack

The command output includes:

Dropping unused variable square [main.js:81,6]

And dist/main.js has contents:

/******/ !function(modules) {
    // webpackBootstrap
    // ...
/************************************************************************/
/******/ ([ 
/* 0 */
/***/ function(module, __webpack_exports__, __webpack_require__) {
    "use strict";
    __webpack_require__.r(__webpack_exports__);
    // CONCATENATED MODULE: ./src/index.js
    console.log((x => x * x * x)(3));
}
/******/ ]);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment