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 not working in 2.1.0-beta.25 #3092

Closed
leebenson opened this Issue Oct 3, 2016 · 30 comments

Comments

Projects
None yet
@leebenson
Copy link

leebenson commented Oct 3, 2016

I'm submitting a bug report

Webpack version:
2.1.0-beta.25

Please tell us about your environment:
macOS Sierra 10.12.1 Beta

Current behavior:

Tree shaking doesn't seem to be working. In the 901kb file that's generated in my live production (obviously more code than is shown below), a search on the generated bundle reveals the youshouldnevereverseethis string in the source code.

Expected/desired behavior:

Tree shaking should happen, and the exported test1 function should not be visible in source code.

Config

Language:

  • ES6 via babel-core 6.17.0
  • Babel loader config:
{
  test: /\.jsx?$/,
  loader: 'babel',
  query: {
    presets: [
      ['es2015', {
        modules: false,
      }],
      'react',
      'stage-1',
    ],
    plugins: [
      'transform-decorators-legacy',
      path.join(PATHS.app, '../plugins/relay'),
    ],
  },
},
@zhaoshengjun

This comment has been minimized.

Copy link
Contributor

zhaoshengjun commented Oct 4, 2016

I tried to replicate your issue, but there are so many missing pieces.
I recreated a repo based on the file-to-import.js and main.js, using a bare-bone webpack.config.js and it worked. You may check it here.

If you have a more complete repo to share, I can check it for you to see what's wrong.

@GianlucaGuarini

This comment has been minimized.

Copy link
Contributor

GianlucaGuarini commented Oct 6, 2016

I have a similar problem. This is my webpack config file and I realized it generates a bundle 3.2MB (1.3MB minified) big. I don't understand why in my output code there seem to be core node modules like Buffer or process (https://github.com/webpack/node-libs-browser) even if they never were required. This issue does not only appear in webpack@2.1.0-beta.25 but it was present also in webpack@2.1.0-beta.19. I am also on Sierra 10.12.
Side note: I was pinged on twitter by @TheLarkInn offering help on the issue

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 6, 2016

I'm pushing a repo that has my exact webpack config, that you can build locally and see that tree shaking isn't working. Will have it up in a few mins and post here.

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 6, 2016

Here it is: https://github.com/leebenson/wp-tree-shaking

Notes:

  • Run npm i then npm run build locally to build.
  • I've included the dist folder to show you the same output on my local machine. Search for youshouldnevereverseethis in the dist/public/bundle.js file, and you'll find it - this should never have landed because it's not referenced by src/main.js.
  • npm run build generates TWO bundles: A production client-side build (dist/public/bundle.js), and a build that targets node (dist/server.js). This doesn't make much sense in the context of the src folder provided, but in production, I do a lot of inline SASS and static asset imports and write universal React components, which run both through a web server and in the browser. I use webpack to bundle both.
  • There's a ton of plug-ins that aren't used here - SASS, resolve-url-loader, handling static assets, a file loader, a plug-in that copies files from my src/public folder to dist, etc. I've kept them intact to exactly match the webpack I'm using in my live codebase, even though src/main.js doesn't require the majority of them.
  • I've updated src/main.js to use an async/await pattern, which is common throughout my code. Just in case it's Babel that's transpiling and screwing things up.
@Jessidhia

This comment has been minimized.

Copy link
Member

Jessidhia commented Oct 6, 2016

@leebenson you are not giving ES6 modules to webpack; you are giving commonjs as transformed by babel. https://github.com/leebenson/wp-tree-shaking/blob/master/dist/server.js#L124-L126

There's also a leftover Object.defineProperty(n,"__esModule",{value:!0}) in the minified public bundle, in same module the leftover functions are defined; meaning that this was commonjs-ified by babel.

Try giving babelrc: false to the babel-loader settings; babel plugins are additive, and babel might be loading the commonjs transform from the "presets": [ "es2015" ] in your project root.

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 6, 2016

@Kovensky - that's not the problem (or shouldn't be the problem)

.babelrc is only there to transpile webpack.config.babel.js - there's a separate query for babel in this file that has the correct es2015 loader syntax.

Ignore the dist/server.js bundle too - I'm not trying to transpile the server down to es2015 (it's running through Node6 - hence the separate query on that one).

To clarify, all I care about is the output of dist/public/bundle.js, which is controlled by these files:

https://github.com/leebenson/wp-tree-shaking/blob/master/webpack/base.js

https://github.com/leebenson/wp-tree-shaking/blob/master/webpack/client.js

https://github.com/leebenson/wp-tree-shaking/blob/master/webpack/production.js

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 6, 2016

Try giving babelrc: false to the babel-loader settings; babel plugins are additive, and babel might be loading the commonjs transform from the "presets": [ "es2015" ] in your project root.

I saw your edit after posting. This actually fixed it. Genius! Thanks @Kovensky

@zhaoshengjun

This comment has been minimized.

Copy link
Contributor

zhaoshengjun commented Oct 6, 2016

Just learned a new trick 😄

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 6, 2016

Since we're on the topic of tree shaking, is there any way to convert CommonJS into ES6?

98% of my resulting bundle is pulled from older NPM modules that use module.exports, so the benefits of tree shaking are unfortunately limited to a few lines of code.

Rollup has this plugin; is there anything similar in the webpack world?

@maiis

This comment has been minimized.

Copy link

maiis commented Oct 6, 2016

@leebenson I was wondering the same thing, and found this issue this morning: #2372
With this trick my build contains 553 chunks instead 800+ earlier.

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 6, 2016

great tip, thanks @maiis - I'll give it a shot.

@BrainCrumbz

This comment has been minimized.

Copy link

BrainCrumbz commented Oct 24, 2016

@leebenson (or any one) could you please provide the "whole picture", I mean, what are the various items to check in order to enable tree-shaking? Thanks

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 25, 2016

@BrainCrumbz - basically, just make sure you're using the Babel es2015 plugin with modules set to false.

My loader config looks like this:

{
  test: /\.jsx?$/,
  exclude: /node_modules/,
  loader: 'babel',
  query: {
    babelrc: false,
    presets: [
      ['es2015', { modules: false }],
      'react',
      'stage-0',
    ],
    plugins: [
      'transform-decorators-legacy',
    ],
  },
},

Setting babelrc: false inside 'query' was required in my case, since I had an existing .babelrc file that I use via the CLI. Setting that field to false replaces it, rather than extends it, which guarantees you're using the exact query specified.

Tree shaking will only work if you're using ES6 import/exports, a la:

import { somethingSpecific } from 'some/package';

If you're using CommonJS (e.g. require()) or pulling everything in, like import * as whatever from 'wherever', then you won't get the benefit of tree-shaking-- you'll be pulling in everything that the module exports.

Tree shaking will start to make a lot of sense when third-party libraries adopt ES6 en masse. Some have started offering ES6 and CommonJS versions (usually by importing from a different file, or with their package.json file having a different entry point); use them where possible.

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 25, 2016

btw, I have exclude: /node_modules/ in there - this will assume any third-party packages have already gone through transpilation. You may get some extra juice dropping that (at the cost of increased transpilation times). In my config, it actually increased the overall bundle size, so I chose to ignore.

@Jessidhia

This comment has been minimized.

Copy link
Member

Jessidhia commented Oct 26, 2016

There are still some limitations with reexports, or internal dependencies only accessed from dead code; c.f. #1750.

It seems that packages' bodies are required by spec to be evaluated even if none of their exports are used. Feels like kind of an oversight to me; I believe only import 'package' should unconditionally run the body, but that's not how reality is :/

@leebenson

This comment has been minimized.

Copy link
Author

leebenson commented Oct 26, 2016

@Kovensky - I actually ran into this today. I have two exported functions - one being an immediate function that creates a scoped variable and returns a function that references it, that's designed to run in the browser. The second is a server-only function.

Examining output, I realised that despite import { getServer } ..., the getClient export was being invoked too, and breaking because of its dependency on the DOM.

I expected the export to be evaluated on import, but yeah, the whole body is executed, even if it's not being used. Seems a bit wasteful!

@BrainCrumbz

This comment has been minimized.

Copy link

BrainCrumbz commented Oct 27, 2016

@leebenson thanks for your hints. We're not using babel, and we do import only needed components already. But I cannot see a difference w.r.t. what we already have from AOT production build. Let's see how things evolve.

@mlwigdahl mlwigdahl referenced this issue Nov 13, 2016

Merged

Webpack 2 #314

@elyobo

This comment has been minimized.

Copy link

elyobo commented Jan 3, 2017

As @GianlucaGuarini mentioned, I'm seeing a lot of dependencies come in via node-browser-libs that weren't there before which is significantly increasing my build size. It seems to be pulling in many which I don't use at all as far as I can tell (e.g. bn.js via crypto-browserify) and weren't pulled in by webpack 1. Is this expected behaviour?

I can exclude these manually with NormalModuleReplacementPlugin, but it seems a bit flaky because perhaps some of these are required somewhere else in my dependencies. It would be better if those that really aren't needed simply weren't pulled in.

@m18

This comment has been minimized.

Copy link

m18 commented Feb 21, 2017

@elyobo had the exact same issue -- bn.js etc. being bundled together with the rest of the JS (in)effectively doubling the bundle's size.

Replacing the node-uuid package with uuid package helped.

@elyobo

This comment has been minimized.

Copy link

elyobo commented Feb 21, 2017

I ended up ignoring these for our application, as they're not being used. I guess the actual requirements will vary by app though.

const ignoredNodeBrowserLibs = [                                                                                                                            
  'commonjs-assert',                                                            
  'console-browserify',                                                         
  'constants-browserify',                                                       
  'domain-browser',                                                             
  'os-browserify',                                                              
  'node-process',                                                               
  'string_decoder',                                                             
  'timers-browserify',                                                          
  'tty-browserify',                                                             
  'vm-browserify',                                                              
  'browserify-zlib',                                                            
                                                                                
  // We do need crypto, but not much of it...                                   
  'browserify-cipher',                                                          
  'browserify-sign',                                                            
  'create-ecdh',                                                                
  'create-hmac',                                                                
  'diffie-hellman',                                                             
  'pbkdf2',                                                                     
  'public-encrypt',                                                             
  'randombytes',                                                                
];
@GianlucaGuarini

This comment has been minimized.

Copy link
Contributor

GianlucaGuarini commented Feb 21, 2017

I was able to solve my problem using https://webpack.github.io/analyse/ I realized that in the end it was not really a webpack fault but there where libraries requiring wrongly core node modules I hope this will help anyone else as well

@swenyang

This comment has been minimized.

Copy link

swenyang commented Mar 5, 2017

@m18 Could you show a little more how to remove bn.js? I found bn.js, buffer, elliptic, readable-stream sort of things in my bundle, which increase file size significantly.

@swenyang

This comment has been minimized.

Copy link

swenyang commented Mar 5, 2017

@m18 Sorry just ignore it, I found that they are all imported via sjcl.

@efortes

This comment has been minimized.

Copy link

efortes commented Mar 8, 2017

node-uuid replaced with package with uuid and the bundle size is reduced with 300kb.

@kafkahw

This comment has been minimized.

Copy link

kafkahw commented May 17, 2017

I got the same problem. bn.js is included in my bundle.js, however, I found that bn.js@4.11.6 is just a dependency of webpack@2.5.1 which is a dev dependency of my app. I'm curious how bn.js gets included in the final bundle.

@Benno007

This comment has been minimized.

Copy link

Benno007 commented May 20, 2017

Yeah, I've noticed this has increased our app.js by about 220kb minified. We aren't using node-uuid, they're dependencies of webpack. Does anyone know how to get rid of these properly?

PS #> yarn why bn.js
yarn why v0.24.4
[1/4] Why do we have the module "bn.js"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
info Reasons this module exists
   - "webpack#node-libs-browser#crypto-browserify#browserify-sign" depends on it
   - "webpack#node-libs-browser#crypto-browserify#create-ecdh" depends on it
   - "webpack#node-libs-browser#crypto-browserify#browserify-sign#elliptic" depends on it
   - "webpack#node-libs-browser#crypto-browserify#diffie-hellman#miller-rabin" depends on it
@michael-ciniawsky

This comment has been minimized.

Copy link
Member

michael-ciniawsky commented May 20, 2017

webpack.config.js

const config = {
  node: {
    crypto: false
  }
}

@Benno007 Untested if crypto is the correct name, but that's the interface to exclude node-libs-browser stuff to my knowledge

Node

@Benno007

This comment has been minimized.

Copy link

Benno007 commented May 20, 2017

Awesome thank you @michael-ciniawsky , that's worked!

@OliverJAsh

This comment has been minimized.

Copy link

OliverJAsh commented Oct 24, 2017

For anyone interested in how replacing node-uuid with uuid fixes this. Here is what I learnt. (Valid only at the time of writing!)

node-uuid and uuid used to be separate projects, but are now merged. The npm package node-uuid is deprecated. The new npm package is called uuid.

The old node-uuid package requires the crypto module, which in webpack's case will introduce node-libs-browser into the dependency graph. https://github.com/broofa/node-uuid/blob/309512573ec1c60143c257157479a20f7f1f51cd/uuid.js#L59

The new uuid package rewires module IDs for the browser to avoid requiring the crypto module: https://github.com/kelektiv/node-uuid/blob/bba940235d5c4778ec289f9e65dae0cd526038ef/package.json#L25. In place of crypto, it will use the browser crypto API if available and fallback to using Math.random: https://github.com/kelektiv/node-uuid/blob/bba940235d5c4778ec289f9e65dae0cd526038ef/lib/rng-browser.js#L1.

@edmorley

This comment has been minimized.

Copy link
Member

edmorley commented Mar 12, 2018

The STR in the original post no longer reproduce with webpack 4 (note main.js also has a typo; test() should be test2() otherwise neither function gets bundled).

ie: the generated output (using webpack --mode production) only includes the string 'but you should see this' and not 'youshouldnevereverseethis'.

The other comments in this thread are relating to packages depending on built-ins, which are unrelated.

As such, it seems like this issue can now be closed?

@leebenson leebenson closed this Mar 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.