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

Bundle size with ES modules #11281

Closed
1 task done
crsanti opened this issue May 8, 2018 · 27 comments
Closed
1 task done

Bundle size with ES modules #11281

crsanti opened this issue May 8, 2018 · 27 comments

Comments

@crsanti
Copy link

crsanti commented May 8, 2018

When creating bundle with webpack (4.8.1) using ES6 syntax + Babel transpilation bundle size (aplying tree shaking) is huge and sweeps along lots of components I've not imported.

  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

Bundle size should be smaller and keep only imported components.

Current Behavior

My current project setup uses 2 steps transpilation: TypeScript --> ES6 & Babel --> ES5.

In my tsconfig.js I tell to TypeScript to transpile to ES6:

{
  "compilerOptions": {
    "target": "es6",
    "module": "es6",
    ...
  }
}

Then I tell to Babel to not convert ES6 modules to CommonJS:

{
  "presets": [
    [
      "env",
      {
        "modules": false
      }
    ]
  ]
}

Inside my app I'm importing components I need:

import { AppBar, Toolbar, Typography } from "material-ui";

I've read about how to reduce bundle size importing directly components from files instead of using barrel but since my setup involves ES6 it should apply tree shaking on components I'm not using since I'm working with ES6 modules and this library has a proper ES build (exposing all components under es directory)

So the current bundle size is next for material-ui:

tree shaking not applied

From Webpack docs "material-ui" is resolved reading from package.json properties in next order when target is set to web (or unspecified): ["browser", "module", "main"]. Currently material-ui has module and main:

"main": "./index.js",
"module": "./index.es.js"

So when I import some components from material-ui using barrel it should be using "index.es.js". If I open index.es.js I get:

/** @license Material-UI v1.0.0-beta.45
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */
export { default as AppBar } from './AppBar';
export { default as Avatar } from './Avatar';
export { default as Badge } from './Badge';
...

The real issue here is that inside index.es.js all components are imported from compiled files instead of "es" folder.

If I make a quick replace and point components to es folder:

/** @license Material-UI v1.0.0-beta.45
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 */
export { default as AppBar } from './es/AppBar';
export { default as Avatar } from './es/Avatar';
export { default as Badge } from './es/Badge';
...

The bundle and build size is next:

tree shaking applied

From a consumer lib side I know I can use an alias inside webpack config to point to es folder like so:

module.exports = {
  resolve: {
    extensions: ['.js', '.ts', '.tsx'],
    alias: {
      'material-ui': 'material-ui/es',
    }
  },
  ...
};

But I think this bundle size issue can be solved from inside this lib 👍 pointing components to es folder on index.es.js.

Steps to Reproduce (for bugs)

  1. Clone branch "issue-tree-shaking" from this repo:
git clone -b "issue-tree-shaking" https://github.com/Lemoncode/treeshaking-samples.git
  1. cd into typescript/03\ barrels
  2. make npm install
  3. make npm run build and see material-ui size (It should be same as first posted image)
  4. Uncomment alias in webpack.config.js and make an npm run build (material-ui bundle should be reduced since it is pointing to es folder and result should be like second posted image)
  5. Comment out alias in webpack.config.js and edit node_modules/material-ui/index.es.js pointing each export to es folder as described above.
  6. Make another npm run build. Size should be same as step 5.

Context

Bundle size is aumented when it shouldn't.

Your Environment

Tech Version
Material-UI v1.0.0-beta.45
React 16.3.2
browser -
etc -
@fjcalzado
Copy link

fjcalzado commented May 8, 2018

Good job @crsanti ! Exactly, I also believe that pointing to commonJS modules from index.es.js barrel makes no sense. That index.es.js should be aimed to be the entry point for ES6 modules imports. Are we missing something here or is it a bug? Thanks for your great contribution guys.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 8, 2018

Thanks for opening this issue. It's definitely not the first time we get this feedback. But for the first time, I see tree shaking working 👏 . Related issues:

I also believe that pointing to commonJS modules from index.es.js barrel makes no sense.

@fjcalzado The whole point is to avoid bundle bloat. By publishing a second version of the Material-UI modules, we take the risk that they get loaded twice in people bundle (it's what is already happening to people with lodash, lodash/x and lodash.x: 3x). Material-UI total size is 100 kB gzipped. We definitely don't want people to end up with 200 kB gzipped in their bundle. This can happen very quickly, all you need is one third-party library that doesn't follow the convention and loads the wrong version. The list of third-party libraries based on Material-UI grows quickly.
Most third party libraries do import TextField from 'material-ui/TextField'; over import { TextField } from 'material-ui';. For instance:

But I think this bundle size issue can be solved from inside this lib 👍 pointing components to es folder on index.es.js.

@crsanti ⚠️⚠️⚠️ The /es folder is not what you think it's. It wasn't designed for solving the tree shaking problem. It solves the older browser support problem. It only works with evergreen browsers. UglifyJS will fail to minify it. For instance, the /es folder you will find on react-router is different.
We warn about the limitations of the /es folder: https://material-ui.com/guides/minimizing-bundle-size/#ecmascript.

Regarding going forward

We will soon flatten the import path #9532. This will allow the Babel plugins referenced in https://material-ui.com/guides/minimizing-bundle-size/#option-2 to work.
Regarding publishing a third version of our modules to natively support webpack's tree shaking, like you are suggesting, I don't know.

We have a much smaller module duplication tolerance to projects like react-router. Both because the size of the material-ui package is pretty large and because the no-config mode of the CSS-in-JSS solution relies on a singleton. If we move forward in this direction, we have to move the whole ecosystem. It's much harder.
On the other hand, we could encourage people to use the Babel plugin, and work with webpack to see what we/they can do for solving the ES module -> CommonJS tree-shaking deoptimization issue.

@TrySound
Copy link
Contributor

TrySound commented May 8, 2018

I can help to solve treeshakability problem if you are interested.

@abnersajr
Copy link
Contributor

abnersajr commented May 9, 2018

I had saw this in my project too. The way that I solving is using the default import like @oliviertassinari wrote in that comment:
import TextField from 'material-ui/TextField';
Using this way you can achieve what you expect. For sure would be great to also have an way to use like you wrote, but I don't see a big issue using the way referenced above.

@fjcalzado
Copy link

@oliviertassinari I think I get your point now. Just let me explain you to confirm. You mean I can end up with an scenario where a 3rd party library I depend on is bringing MUI CJS modules while I intentionally use ES variant modules of MUI, and thus, duplicating same components (with different module formats) in my bundle. Am I right? And by making index.es.js point to CJS modules, at least, it could relieve that potential issue as webpack or any other bundler could handle them as common chunks of code. Right?

@brauliodiez
Copy link

@oliviertassinari About option 2, I have tried using babel-plugin-import, it works fine on a simple scenario (single bundle file), but as soon you have a vendor chunk it does not apply treeshaking. Checking on their github main page they are ware of this limitation:

babel-plugin-import will not work properly if you add the library to the webpack config vendor.

Should material ui minimizing bundle size guide warn about this limitation and discourage developers using this approach on a real project? I haven't tried with the two other mentioned plugins, will give a try and check if they have the same issue.

@oliviertassinari
Copy link
Member

oliviertassinari commented May 10, 2018

I can help to solve treeshakability problem if you are interested.

@TrySound I have pinged you on Gitter so we can talk about it :).

I haven't tried with the two other mentioned plugins, will give a try and check if they have the same issue.

@crsanti I haven't tried them out yet. Yes, please :). I have a long-term goal of using the "barrel" approach at the office but I never had the opportunity to prioritize it over the other topics.
Not everybody is using webpack. But the fact it's not working with a vendor is surprising :/.

@arialpew
Copy link

arialpew commented May 17, 2018

Note : A barrel is a single file who re-export things from other modules.

There's no standard of how we can ship modern JS (see https://github.com/renchap/modern-js-in-browsers/).

WebPack 4 resolve module via "package.json" fields in this order :

  • browser (if target is set to "web" or "webworker" or unspecified).
  • module
  • main

You probably know the "main" field, the standard entrypoint, mostly for CJS module but you can use any format.

The new "browser" field aim to target browser only. For universal package, you should not use it (Material-ui is universal, they don't use "browser" field).

The new "module" field aim to help tree-shaking and Node.js experimental new module system (mjs), it should point to ES module.

Other fields like "jsnext:main" (Rollup) or "source" (Parcel) are not supported by WebPack.

Material-ui "package.json" :

"main": "./index.js",
"module": "./index.es.js"

The issue say "index.es.js" re-export CJS module who use "require" to load module, instead of ES module who use "import/export". So we can't take advantage of tree-shaking, it's true.

To enable tree-shaking with WebPack and "babel-plugin-import", we have to rewrite import path :

{
   "plugins": [
      [
         "babel-plugin-import",
         {
            "libraryName": "@material-ui/core",
            "libraryDirectory": "",
            "camel2DashComponentName": false
         },
         "tree-shaking-mui-core"
      ],
      [
         "babel-plugin-import",
         {
            "libraryName": "@material-ui/core/styles",
            "libraryDirectory": "",
            "camel2DashComponentName": false
         },
         "tree-shaking-mui-styles"
      ],
      [
         "babel-plugin-import",
         {
            "libraryName": "@material-ui/core/colors",
            "libraryDirectory": "",
            "camel2DashComponentName": false
         },
         "tree-shaking-mui-colors"
      ],
      [
         "babel-plugin-import",
         {
            "libraryName": "@material-ui/icons",
            "libraryDirectory": "",
            "camel2DashComponentName": false
         },
         "tree-shaking-mui-icons"
      ]
   ]
}

Babel 7 doesn't support an array of object as option so you have to duplicate the plugin and name all instances.

Then, you can use all Material-ui barrel, plugins take care of import path rewrite :

import { TextField } from '@material-ui/core';
import { createMuiTheme } from '@material-ui/core/styles';
import { blue } from '@material-ui/core/colors';
import { DataUsage } from '@material/icons';

Without "babel-plugin-import" (tree-shaking off) : A

With "babel-plugin-import" (tree-shaking on) : B

"babel-plugin-import will not work properly if you add the library to the webpack config vendor" is not true for latest version of WebPack. I use this strategy (you can see the vendor/chunk distribution above) :

{
    optimization: {
        splitChunks: {
            chunks: 'all'
        },
        namedModules: true
    }
}

If you want to use Material-ui evergreen (ES instead of CJS), remove the previous "babel-plugin-import" configuration and use WebPack aliasing. Instead of ES5, you will now get ES6 version.

{
      resolve: {
          alias: {
            '@material-ui/core': '@material-ui/core/es',
            '@material-ui/icons': '@material-ui/icons/es'
          }
      }
}

Be carefull, it come with a cost :

  • The alias works on WebPack side, not on Babel side, Jest and others tools can not use it.
  • Less browser support (evergreen version).
  • Possible Uglifyjs error (but i think it's ok, Material-ui do a small transpilation on source to produce ES - JSX, ... - and Uglifyjs3 should support ES6 syntax now ? => there's new alternative like Terser who aim to support ES6+ syntax). If you want to get ride of that, you should transpile "node_modules" like create-react-app/next (see Enable babel-preset-env for node_modules that target newer Node versions facebook/create-react-app#1125) with preset-env.

With ES instead of CJS, Material-ui is smaller than ReactDOM :) :

C

I think it's worth to do not use WebPack aliasing + ES and stay with "babel-plugin-import" + CJS for the moment, unlike you really do not want to support old browser and you know what you are doing.

@dantman
Copy link
Contributor

dantman commented May 22, 2018

I haven't upgraded to WebPack 4 yet. But I'll note that with WebPack aliasing (though I use NormalModuleReplacementPlugin instead of resolve.alias) plus a separate babel-loader config to transpile node_modules with env, even though I am transpiling with IE11 as a target, the bundle size is still smaller when I use es/ and this technique instead of importing the ES5 version of material-ui.

i.e. es/ is still advantageous even when you still have to support old browsers.

@TrySound
Copy link
Contributor

@kMeillet jsnext:main is deprecated. rollup-plugin-node-resolve supports module field.

@jeffvandyke
Copy link

jeffvandyke commented May 24, 2018

Just thought I'd chime in with a note on how the bundle size is affected as of version 1. I started with create-react-app, and analyzed bundle size before and after the Button component from Material UI was used.

Parsed Gzipped
Initial create-react-app: 114.87 KB 36.71 KB
After Material UI Button: 241.60 KB 70.75 KB

(This issue really helped using Webpack Bundle Analyzer with create-react-app)

Code in src/App.jsx:

import Button from '@material-ui/core/Button';
...
    <Button variant="raised" color="default">Default</Button> {' '}
    <Button variant="raised" color="inherit">Inherit</Button> {' '}
    <Button variant="raised" color="primary">Primary</Button> {' '}
    <Button variant="raised" color="secondary">Secondary</Button>

This is a size increase of 126.73 KB, 34.04 KB Gzipped, but the docs on minimizing bundle size said this difference should be ~20 KB Gzipped. I'm investigating using Material UI in an embedded environment where size really matters, and serving Gzipped is difficult at the moment, so I'm still looking for ways to make this smaller, or to trim out some fat. I see similar metrics in my own project that upgraded to Webpack 4 with default production mode, though it's worth mentioning that create-react-app is still on Webpack 3.

@oliviertassinari
Copy link
Member

This is a size increase of 126.73 KB, 34.04 KB Gzipped, but the docs on minimizing bundle size said this difference should be ~20 KB Gzipped.

@jeffvandyke It's getting better with #11492 and some other changes in v1.1.0.

@sesam
Copy link

sesam commented Jul 25, 2018

@jeffvandyke maybe do "two-stage delivery" so the embedded is just a jump-off point and data-source, but gets its libraries from elsewhere (CDN.js etc, webworkers). Or support only evergreen browsers. Or see if parcel is better at tree-shaking (hoisting) than webpack.

@alex996
Copy link

alex996 commented Sep 3, 2018

@oliviertassinari From Minimizing Bundle Size

Important note: Both of these options should be temporary until you add tree shaking capabilities to your project.

I find the last part misleading. It implies that the end user is supposed to tweak their config to enable tree shaking. However, both index.js and index.es.js barrels in MUI point to CJS exports, and from what I found, it's practically impossible to tree shake CJS. So, in reality, MUI does not support tree-shaking, and the most we can do is sugar-code our named imports with babel-plugin-import, so we can at least trim a dozen or two kB.

Regarding /es exports, they indirectly benefit from tree-shaking because of ESM syntax. But if you import those with @dantman's handy path rewrite, you'd need to transpile MUI with env preset to cover older browsers. But why not have a version in MUI akin to /es that is also transpiled with env internally, and have the library export it by default?

This way we'll have ESM exports that can be tree shaken. Most people use a bundler anyway. I find it strange that the footprint of /es manually transpiled for IE11 is still smaller than that of ES5. Assuming they are ran through the same plugins (except env), is it purely because of CJS vs ESM? I guess CJS is preferred because of Node, but perhaps we could work something out with mjs extension instead? Or maybe preload esm for Node environments until ES modules are fully supported?

I wish the docs were a bit clearer on that. From what I see, there's nothing that can be done, except opting in for /es. Unless I'm missing something. Thanks!

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 4, 2018

it's practically impossible to tree shake CJS

@alex996 Parcel recently announced that it was supporting it. I have never tried it.

The issue say "index.es.js" re-export CJS module who use "require" to load module, instead of ES module who use "import/export". So we can't take advantage of tree-shaking, it's true.

@kMeillet I start to believe that we could be leveraging the alias feature of the bundlers to improve the current solution with third-party libraries using Material-UI.
What about publishing a /modules folder under @material-ui/core package?
We could change the package.json to:

"main": "./index.js",
"module": "./index.modules.js",

This way, I hope we can match webpack requirements to have a working tree-shaking. Once we release this change, we will most likely have a module duplicate with third-party not releasing a module version pointing to ours. It will break many people code. But hopefully, people should be able to dodge the issue with an alias and third-party components will update to follow the standard.

@rosskevin
Copy link
Member

@oliviertassinari I agree that we should be publishing the package.json module as something that points to an entry point and source behind it that preserves import and export statements. This should allow for proper tree shaking.

I am unclear on why the module points to index.es.js but this in turn just points to cjs code. It doesn't make sense to me but I never dug into it.

@TrySound
Copy link
Contributor

TrySound commented Sep 4, 2018

Let's call it esm like esm.

@rosskevin The reason is less verbose interop with cjs reexports.

@eps1lon
Copy link
Member

eps1lon commented Oct 24, 2018

So I did some experimenting with webpack aliases and had some success to workaround the current "module" entry point: https://github.com/eps1lon/material-ui-treeshaking/blob/master/config/webpack.config.current-manual-shakeable.js
This is basically what @kMeillet proposed as a workaround but that solution will put es6 code into your bundle even if you want to target es5 because you usually ignore files in node_modules

The main offender is that module points to a file with esm modules but that file points to cjs files. The other point is that our published /es is quite confusing. Usually packages publish files with esm under es.

I also tried putting .mjs files next to the .js files so that users could still do @material-ui/core/* and get full tree shaking capability. But this requires additional bundler configuration at the moment and everyone that wants full tree shaking should be doing import {} from '@material-ui/core' anyway.

So continuing from @TrySound change build:es2015modules to build:esm and use build/esm as the out-dir instead of a single file. Let module point to esm/index.js.

The biggest issue with this solution is that 3rd party libraries use @material-ui/core/Component imports which would be duplicated in the bundle as @oliviertassinari pointed out: #11281 (comment). I think if we go forward with this we should patch those 3rd party libraries. At least so that they point to @material-ui/core/esm/Component.

I like the idea of es which is basically just the source from our components without debug related logic. But I would just name it src to avoid confusion with es modules.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 24, 2018

@eps1lon I'm all 💯 for the plan 👍 . Renaming es to src is a breaking change. We can do it in v4.0.0. Do you want to take the lead on this topic?

@TrySound
Copy link
Contributor

Take a look at this guys
https://unpkg.com/date-fns@2.0.0-alpha.24/format/package.json

@eps1lon
Copy link
Member

eps1lon commented Oct 24, 2018

@TrySound They are basically adding a package.json to every folder? That's probably the best idea.

@oliviertassinari
Copy link
Member

They are basically adding a package.json to every folder? That's probably the best idea.

How does it work 🤔, is this something standard?

@TrySound
Copy link
Contributor

Yep. Node resolve algorithm is able to resolve this.

@eps1lon
Copy link
Member

eps1lon commented Oct 25, 2018

https://nodejs.org/api/modules.html#modules_folders_as_modules

The good thing is that even if some bundler(version) does not implement this behavior then it won't cause any breakage. It's entirely optional. If your bundler looks at package.json in folders then you will get full tree shaking otherwise everything will function as normal. I'll have a look.

@TrySound
Copy link
Contributor

It's not treeshaking. It's splitting code by files or path imports. Treeshaking is program analyses of what should be included in bundle. With path imports users choose this not program.

@eps1lon
Copy link
Member

eps1lon commented Oct 25, 2018

It's not treeshaking. It's splitting code by files or path imports. Treeshaking is program analyses of what should be included in bundle. With path imports users choose this not program.

Sure but that file should also be shakeable which is only possible with the package.json approach (unless the bundler can actually shake commonJS which webpack at least can't).

@eps1lon
Copy link
Member

eps1lon commented Oct 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests