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

Tree shaking completely broken? #2867

Closed
vladshcherbin opened this issue Aug 13, 2016 · 163 comments
Closed

Tree shaking completely broken? #2867

vladshcherbin opened this issue Aug 13, 2016 · 163 comments

Comments

@vladshcherbin
Copy link

vladshcherbin commented Aug 13, 2016

updated with the latest versions (29.06.2017)
Webpack - 3.0.0
Babel-preset-env - 1.5.2
Node - 8.1.2
OS X - 10.12.5

Current

Tree shaking is not removing imports, not used in the app.

Expected

Tree shaking, removing not used imports.


The test case is a simple app with React & React Router v4. Link component is not used, but the whole router package is bundled.

import React from 'react'
import ReactDOM from 'react-dom'
import { Link } from 'react-router-dom'

ReactDOM.render(
  <p>Hey</p>,
  document.getElementById('app')
)

Webpack & Rollup test repo

@TheLarkInn
Copy link
Member

If you were to use this with babel-es2015-webpack does this still work. I think its worth verifying as ["es2015", {"loose": true, "modules": false}]] is a relatively new change to babel.

@vladshcherbin
Copy link
Author

@TheLarkInn yes, I tried both babel-preset-es2015-webpack and babel-preset-es2015-loose-native-modules presets.

Unfortunately, the result is the same.

@TheLarkInn
Copy link
Member

What version did you jump from where it was working?

@vladshcherbin
Copy link
Author

@TheLarkInn I'm using it in a fresh project.

Got curious about the production file size after adding some packages and discovered this. I've seen previously issues and topics about the size, latest in create-react-app with no file size reduction.

I'll check another versions of webpack, maybe there will be difference.

@Jessidhia
Copy link
Member

This seems to be a problem only in combination with reexports...

@vladshcherbin
Copy link
Author

vladshcherbin commented Aug 15, 2016

Here is my small investigation. Everything was tested with the same config as above:

  • Webpack - 2.1.0-beta.20
  • Babel es2015 preset - 6.13.2
  • OS X 10.11

Simple react app:

import React from 'react'
import ReactDOM from 'react-dom'

ReactDOM.render(
  <p>Hey</p>,
  document.getElementById('app')
)

The production build is 146 kb.

Time for tests:

  1. Lets add some simple functions like this:
function fA() {
  console.log('I am a string')
}

We import some of them, but use only one:

import React from 'react'
import ReactDOM from 'react-dom'
import { fA, fB, fC, fD, fE, fF, fG } from './func'

fA()

ReactDOM.render(
  <p>Hey</p>,
  document.getElementById('app')
)

The size is still 146 kb as the function is pretty small. If we use all of them - the size is 150 kb.

Tree shaking is working.

  1. Lets make the same test with react stuff. Simple components:
import React from 'react'

const A = () => (
  <div>
    <p>Hey, I am a message</p>
  </div>
)

Lets use one component:

import React from 'react'
import ReactDOM from 'react-dom'
import { A, B, C, D, E, F, G } from './comp'

ReactDOM.render(
  <div>
    <A />
  </div>,
  document.getElementById('app')
)

The size is now 147 kb. With all components - 152 kb.

Tree shaking is working.

  1. Now we reexport the components. First, we create a file with named exports:
export { A, B, C, D, E, F, G } from './comp'

The same example as above with one component still gives us 147 kb.

Tree shaking is working.

  1. Same as previous, but with a *
export * from './comp'

Now the size with one component is 152 kb.

Tree shaking is not working (exports with *). Issue is here.

  1. Final test. We make two simple components and use them (as we know, tree shaking is working).
import React from 'react'
import ReactDOM from 'react-dom'
import { A } from './compA'
import { B } from './compB'

ReactDOM.render(
  <div>
    <A />
    <B />
  </div>,
  document.getElementById('app')
)

The size with one component - 147 kb, with both - 149 kb. Now, we take random library and import it in one of the components (lets take B):

import React from 'react'
import Select from 'react-select'

const B = () => (
  <div>
    <p>Hey, I am a message</p>
  </div>
)

We want to use only one component:

import React from 'react'
import ReactDOM from 'react-dom'
import { A } from './compA'
import { B } from './compB'

ReactDOM.render(
  <div>
    <A />
  </div>,
  document.getElementById('app')
)

Unfortunately, now the size is 185 kb.


So, the tree shaking actually removes the component, but some of the imports are included in the build (even if they were not used). Same thing happens with combined / reexported / nested files.

Any ideas why this happens with only some of the components and how to solve this?

@nylen
Copy link

nylen commented Aug 16, 2016

I believe I'm having the same issue - tree shaking doesn't seem to be doing much of anything. The project is https://github.com/redmountainmakers/kilntroller-ui. I haven't narrowed it down to a specific bug but here is an example of a dependency I am definitely not using anywhere:

(image from the excellent [Webpack Visualizer](https://chrisbateman.github.io/webpack-visualizer/) tool)

@sokra
Copy link
Member

sokra commented Aug 19, 2016

you could try it again with webpack beta 21, as I fixed an issue with reexports (exports * from '...')

@nylen
Copy link

nylen commented Aug 20, 2016

No change in bundle size for the master branch of https://github.com/redmountainmakers/kilntroller-ui (444,561 bytes with both beta 20 and 21).

nylen added a commit to redmountainmakers/kilntroller-ui that referenced this issue Aug 20, 2016
@sonicoder86
Copy link

@nylen @vladshcherbin have you tried turning on warnings for UglifyJS? it can tell a lot of useful information about skipped cleanup in unused classes and functions.

I have a very similar ticket with Typescript, where the generated class definitions considered as code with side effects.
#2899

@vladshcherbin
Copy link
Author

@sokra @BlackSonic I actually have a very simple example repo here with webpack and rollup configs.

The index file imports Link from react-router, but never uses it. I expect it be removed from the production build, but it is still there (the whole react-router library I guess).

I'll give it a try with the new versions and warnings on and will post the result.

@sonicoder86
Copy link

Created an issue in UglifyJS, i think it is the source of the problem.
mishoo/UglifyJS#1261

@vladshcherbin
Copy link
Author

Yep, I've tested the latest versions of both webpack/rollup - no changes with tree shaking. There are indeed uglify warnings, maybe this can help.

@sonicoder86
Copy link

There we go, lot of lines like this one

WARN: Side effects in initialization of unused variable replaceLocation [0:24087,4]

These variables remain in the minified version.

@nylen
Copy link

nylen commented Aug 22, 2016

I also have a ton of warnings from uglify (over 1000 of various kinds, including some about side effects). It looks like there's not an easy solution for these issues unfortunately. In https://github.com/redmountainmakers/kilntroller-ui/tree/try/closure-compiler I tried switching to closure-compiler instead - the build was a good bit smaller but I got a bunch of errors.

@drcmda
Copy link

drcmda commented Sep 1, 2016

Can someone explain in simple terms? I am trying to make sense of the informations present but i have no idea what's going on.

I am trying to get an outcome from three.js latest build, which finally features modules.

import * as THREE from 'three/src/Three.js';
let v = new THREE.Vector3();
console.log(v)

It seems to pretty much pull the entire lib, half a MB when it should be a few KB.

@modosc
Copy link

modosc commented Sep 1, 2016

i think webpack2 needs that to be:

import {Vector3} from "three/src/Three.js"

@Jessidhia
Copy link
Member

No, webpack 2 should be able to do tree shaking with namespace imports, as
long as the namespace export doesn't escape.

The important part is that the imported file must use ES6 exports.

On Fri, 2 Sep 2016 at 0:47 jonathan schatz notifications@github.com wrote:

i think webpack2 needs that to be:
'''
import {Vector3} from "three/src/Three.js"
'''


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2867 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEdfaHY5722UN-TDX1OJCwnHQS4yFQqks5qlvOZgaJpZM4JjvHI
.

@arian
Copy link

arian commented Sep 15, 2016

I've hit the same problem:

index.js

import {sudoMakeMeASandwich} from './re-export';
console.log(sudoMakeMeASandwich); // make sure this one is used.

re-export.js

export {makeMeASandwich, sudoMakeMeASandwich} from './helpers';
export {unusedHelper} from './unused-helper';

helpers.js

export const makeMeASandwich = () => 'make sandwich: operation not permitted';
export const sudoMakeMeASandwich = () => 'one open faced club sandwich coming right up';

unused-helper.js

import * as unused2 from './unused-helper-2';
export const unusedHelper = unused2;

unused-helper-2.js

export const FOO = 'FOO';

Expected output

The expected output is a bundle with only the following modules: index, re-export and helpers. The unused modules should not be included, as those are only re-exported by re-export.js, but this export is never used.

Actual output

/******/ (function(modules) { // webpackBootstrap

/************************************************************************/
/******/ ([
/* 0 */
/***/ function(module, exports, __webpack_require__) {

"use strict";
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0__helpers__ = __webpack_require__(1);
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_1__unused_helper__ = __webpack_require__(3);
/* unused harmony reexport makeMeASandwich */
/* harmony reexport (binding) */ __webpack_require__.d(exports, "a", function() { return __WEBPACK_IMPORTED_MODULE_0__helpers__["a"]; });
/* unused harmony reexport unusedHelper */



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

"use strict";
/* unused harmony export makeMeASandwich */
/* harmony export (binding) */ __webpack_require__.d(exports, "a", function() { return sudoMakeMeASandwich; });
var makeMeASandwich = function makeMeASandwich() {
  return 'make sandwich: operation not permitted';
};
var sudoMakeMeASandwich = function sudoMakeMeASandwich() {
  return 'one open faced club sandwich coming right up';
};

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

"use strict";
/* harmony export (binding) */ __webpack_require__.d(exports, "FOO", function() { return FOO; });
var FOO = 'FOO';

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

"use strict";
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0__unused_helper_2__ = __webpack_require__(2);
/* unused harmony export unusedHelper */

var unusedHelper = __WEBPACK_IMPORTED_MODULE_0__unused_helper_2__;

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

"use strict";
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0__re_export__ = __webpack_require__(0);

console.log(__webpack_require__.i(__WEBPACK_IMPORTED_MODULE_0__re_export__["a" /* sudoMakeMeASandwich */])());

/***/ }
/******/ ]);

Rollup

As a small comparison, rollup outputs only:

const sudoMakeMeASandwich = () => 'one open faced club sandwich coming right up';
console.log(sudoMakeMeASandwich());

@weisk
Copy link

weisk commented Nov 6, 2017

@bkniffler why would you need it for lodash? In lodash you can import the micromodules directly, I dont think you should import it globally

@darrenscerri
Copy link

@Vanuan Unused named exports should be removed by tree-shaking (or rather, marked as unused so that Uglify can omit it) - https://webpack.js.org/guides/tree-shaking/

I think the problem here is re-exporting.

@bkniffler
Copy link

@weisk If you import like import {throttle} from "lodash" it still imports the whole thing (last time I checked).

@weisk
Copy link

weisk commented Nov 6, 2017

@bkniffler
Thats not the way to use lodash micromodules, you can do it like this:

import throttle from 'lodash/throttle'

@noppa
Copy link

noppa commented Nov 6, 2017

You could also use babel-plugin-lodash to automatically transform this

import { throttle } from "lodash";

to something equivalent to

import throttle from "lodash/throttle";

@bkniffler
Copy link

Yes @weisk, but if you use lodash excessively, this gets bothering pretty fast, especially since there is alternate ways like @noppa mentioned.

@dhardtke
Copy link

Ran into the same issue with the latest rxjs version when importing like this:
import { concat, map, merge, share, switchMap, take, toArray } from 'rxjs/operators';
instead of separate imports :(

@dmitriid
Copy link

For those who follow this issue, or are subscribed to it, please note that it is now documented as "Caveats" in main documentation: https://webpack.js.org/guides/tree-shaking/#caveats

@xiaoqiang730730
Copy link

xiaoqiang730730 commented Dec 21, 2017

the same to me.
I use the example on the doc;

// no ModuleConcatenationPlugin
"use strict";
Object.defineProperty(__webpack_exports__, "__esModule", { value: true });
/* harmony import */ var __WEBPACK_IMPORTED_MODULE_0__maths_js__ = __webpack_require__(1);


function component() {
    var element = document.createElement('pre');
    element.innerHTML = ['Hello webpack!', '5 cubed is equal to ' + Object(__WEBPACK_IMPORTED_MODULE_0__maths_js__["a" /* cube */])(5)].join('\n\n');

    return element;
}

document.body.appendChild(component());

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

"use strict";
/* unused harmony export square */
/* harmony export (immutable) */ __webpack_exports__["a"] = cube;

function square(x) {
    return x * x;
}

function cube(x) {
    return x * x * x;
}

/***/ })
//use ModuleConcatenationPlugin
"use strict";
Object.defineProperty(__webpack_exports__, "__esModule", { value: true });

// CONCATENATED MODULE: ./demo/maths.js

function square(x) {
    return x * x;
}

function cube(x) {
    return x * x * x;
}
// CONCATENATED MODULE: ./demo/main.js


function component() {
    var element = document.createElement('pre');
    element.innerHTML = ['Hello webpack!', '5 cubed is equal to ' + cube(5)].join('\n\n');

    return element;
}

document.body.appendChild(component());

/***/ })

@sokra
Copy link
Member

sokra commented Dec 23, 2017

webpack 4 allows you to opt-in into more aggressive tree shaking.

@green-pickle
Copy link

@sokra show a working example please for proof. Similar to the example in the first message of the issue, where an external package is used.

@sokra
Copy link
Member

sokra commented Dec 23, 2017

https://github.com/sokra/react-rollup-webpack-test

Note that instead of adding the sideEffects flag to the config, it could also be added to the package.json of react-router-dom.

@green-pickle
Copy link

@sokra very interesting, indeed. 👍

Is there any guide/article about added include block with packages in webpack config and sideEffects flag for developers and library creators?

@sokra
Copy link
Member

sokra commented Dec 26, 2017

Documentation is in progress. It's still in alpha...

TxHawks added a commit to TxHawks/react-powerplug that referenced this issue Jan 30, 2018
Because of webpack/webpack#2867, webpack doesn't remove all unused code
when a user imports a single component from the library.
'date-fns'

Webpack 4 fixes that for libraries that explicitly anounce thmeselves
to be side effect free:
https://github.com/webpack/webpack/tree/next/examples/side-effects.
TxHawks added a commit to TxHawks/react-fns that referenced this issue Jan 30, 2018
Because of webpack/webpack#2867, webpack doesn't always remove all
unused code when a user imports a single component from the library.

Webpack 4 fixes that for libraries that explicitly anounce thmeselves
to be side effect free:
https://github.com/webpack/webpack/tree/next/examples/side-effects.
renatorib pushed a commit to renatorib/react-powerplug that referenced this issue Jan 31, 2018
Because of webpack/webpack#2867, webpack doesn't remove all unused code
when a user imports a single component from the library.
'date-fns'

Webpack 4 fixes that for libraries that explicitly anounce thmeselves
to be side effect free:
https://github.com/webpack/webpack/tree/next/examples/side-effects.
@hisapy
Copy link

hisapy commented Feb 5, 2018

I'm using webpack@3.10.0 and correctly configured babel-loader to avoid modules.

I've trying different webpack config combinations and what I discovered is that tree-shaking does NOT work when using CommonsChunkPlugin.

My 2 big libs that won't tree-shake are rxjs and react-md. If I remove all CommonsChunkPlugin from my plugins list in webpack config then tree-shaking works but I get a lot of repeated code so now I'm analyzing pros vs cons of commons chunks vs tree-shaking

@TheLarkInn
Copy link
Member

Since this has been implemented in webpack 4 (#next branch), and landed, I'm going to close and lock this issue. Please submit a new issue, with a reproducible case and repo included if you believe there is something specifically that is not getting shaken by webpack. Otherwise please submit issues to relative projects (uglify, etc.), where appropriate.

@webpack webpack locked as resolved and limited conversation to collaborators Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests