[build] Bundling with Rollup

[NMinhNguyen]

• I have searched the issues of this repository and believe that this is not a duplicate. I believe the only relevant thing is this comment Cannot bundle MuiThemeProvider.js with rollup #8100 (comment):

  we would love to know if you can bundle the v1-beta branch with rollup.

Expected Behavior

Bundling with Rollup should work as long as MUI provides ES modules (https://github.com/rollup/rollup/wiki/ES6-modules#es6-friendly-libraries).

Current Behavior

Currently, there is an index.es.js file which is an ES module itself, but it points at CommonJS modules (https://unpkg.com/[email protected]/index.es.js). This then causes Rollup to fail to build:

[!] Error: 'BottomNavigationAction' is not exported by node_modules/material-ui/BottomNavigation/index.js
https://github.com/rollup/rollup/wiki/Troubleshooting#name-is-not-exported-by-module
node_modules/material-ui/index.es.js (9:38)
 7: export { default as Avatar } from './Avatar';
 8: export { default as Badge } from './Badge';
 9: export { default as BottomNavigation, BottomNavigationAction } from './BottomNavigation';
                                          ^
10: export { default as Button } from './Button';
11: export { default as ButtonBase } from './ButtonBase';

Presumably, this is because https://github.com/rollup/rollup-plugin-commonjs cannot fully understand the CommonJS exports (unless you manually specify them via namedExports option).

Steps to Reproduce (for bugs)

I have a simple repro available here: https://github.com/NMinhNguyen/material-ui-rollup-example

It also contains a branch where I modified yarn.lock to point at a temporary fork of MUI that publishes ES modules (source), and bundling with Rollup works without any other changes. I am willing to raise a PR with this change (I'll probably also replace the current build:es2015modules script as I believe my change supersedes it, but not sure if removing index.es.js could be considered a breaking change?), but I thought I'd raise an issue first (this is also what the contributing guide suggests) 🙂

I have also created this branch where I committed the current build output and how it changes with my changes so you can more easily review them as well as spot any possible regressions. And here you can find the actual ES5 modules.

By the way, the reason I would rather not use the es folder is because that folder is ESNext (2015+), so I'd have to transpile it via Babel myself. This would mean that I'd additionally have to include these plugins to ensure I get the same Babel transformations applied: https://github.com/mui-org/material-ui/blob/8497cd976d74cfd102c6e8afb932348b366dd5b3/.babelrc#L70-L83

Context

We are using Rollup to bundle our code.

Your Environment

Tech	Version
Material-UI	1.0.0-beta.32
React	16.2.0
Rollup	0.55.3
rollup-plugin-commonjs	8.3.0

[oliviertassinari]

This would mean that I'd additionally have to include these plugins to ensure I get the same Babel transformations applied:

@NMinhNguyen The es folder doesn't aim at solving the tree shaking problem. It's a version of Material-UI that is up-to-date with all the currently supported features of the ECMAScript specification. Which mean we will only transpile stage features and apply our custom code transformations. So, it will only work with evergreen browsers.

I thought we were already applying these transforms for the es folder. We should. Good catch. However, I have one concern with this es folder: it's module duplication. I start to think that the best path going forward is to simply remove it.

Bundling with Rollup should work as long as MUI provides ES modules

Same concern than above. I don't think it's wise to publish different versions of our components. It's gonna bloat users bundle when using third party libraries based on Material-UI. You can already experience the issue with lodash (lodash/x, lodash-es/x, lodash.x).

Error: 'BottomNavigationAction' is not exported by node_modules/material-ui/BottomNavigation/index.js

Could it be a Rollup issue?

[NMinhNguyen]

I don't think it's wise to publish different versions of our components

@oliviertassinari Could you please elaborate on what you meant? Correct me if I'm wrong, but I think the analogy with lodash doesn't necessarily apply here because with the current version 4 of lodash, they actually publish different packages (lodash, lodash-es, lodash.x etc). My proposal is to add a different module format to the same published package, and point the module field in package.json at ./es5/index.js (or similar). This is what libraries such as React Router and Recompose do. Power users could then use plugins such as babel-plugin-lodash to turn import { Button } from 'material-ui' into import Button from 'material-ui/es5/Button'. As for the es5 folder name, since es was taken, I drew inspiration from RxJS who publish ES2015 modules and ES5 modules under _esm2015 and _esm5 folders, respectively.

It's gonna bloat users bundle when using third party libraries based on Material-UI.

Do you mean when the user imports a CJS module, but then a library they use imports an ES module?

Could it be a Rollup issue?

So Rollup is primarily an ES module-aware bundler. There is rollup-plugin-commonjs which is used to

Convert CommonJS modules to ES6, so they can be included in a Rollup bundle

But it is not able to figure out the named exports in all cases, which is when you need to specify the namedExports option, but with such a big (as in, containing many modules) library as Material UI, this can get tedious very quickly 😅 I mean, perhaps there is something that can be done on Rollup's side to make this detection smarter (see related issues 1, 2), but a lot of libraries do publish ES modules where the code is transpiled down to ES5, but the ES imports/exports are left as is.

[oliviertassinari]

they actually publish different packages

Modules don't need to be in different packages to introduce duplication in users bundle. Simply the es non es folder can lead to it. I have been giving more thought about it. I'm happy keeping the es folder as long as third party libraries don't use it and people are aware of the duplication cost it can have.

This is what libraries such as React Router and Recompose do

They are much smaller libraries. Duplication tolerance is higher for them.

Power users could then use plugins such as babel-plugin-lodash

Babel doesn't traverse the node_modules by default. And you don't want him to do it for performance consideration. Babel can't be the answer for solving the third party duplication problem.
Given tree shaking is brittle, I think that we need to ask third party libraries to do an explicit path import:
import Button from 'material-ui/Button'.
This prevents us from following react-router or recompose approach.

Do you mean when the user imports a CJS module, but then a library they use imports an ES module?

Yes, or the other way around.

I mean, perhaps there is something that can be done on Rollup's side

I believe it's working with webpack.

[NMinhNguyen]

Babel don't traverse the node_modules by default. And you don't want him to do it for performance consideration. Babel can't be the answer for solving the third party duplication problem.
Given tree shaking is brittle, I think that we need to ask third party libraries to do an explicit path import.

@oliviertassinari Sorry, I wasn't very clear. I was actually referring to other libraries writing import { Button } from 'material-ui' in their own library source code, and then relying on Babel to transform that into material-ui/Button for CJS output and material-ui/es/Button for ES output, for example. It's what React Router does in react-router-dom.

• Source:

  // Written in this round about way for babel-transform-imports
  import { Router } from "react-router";
  export default Router;

• CJS:

  'use strict';
  
  exports.__esModule = true;
  
  var _Router = require('react-router/Router');
  
  var _Router2 = _interopRequireDefault(_Router);
  
  function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }
  
  exports.default = _Router2.default; // Written in this round about way for babel-transform-imports

• ES:

  // Written in this round about way for babel-transform-imports
  import Router from 'react-router/es/Router';
  
  export default Router;

I still have a feeling that if a library ensures that its ES output only imports ES modules, then the risk of module duplication is significantly reduced? Happy to be proved wrong though.

I guess given your response, we can close out this issue? And to solve the problem of other libraries having to apply the same production-mode Babel transforms on material-ui/es/*, would you be open to me raising a smaller PR that does just that? As a library author, I think that I would still have to transform material-ui/es with Babel for UMD, but maybe for ES modules that I publish, instead of referencing material-ui/es/x (I was trying to ensure that I directly referenced ES modules where possible because tree shaking is so brittle), I'll just have to reference the CJS material-ui/x.

[oliviertassinari]

I still have a feeling that if a library ensures that its ES output only imports ES modules, then the risk of module duplication is significantly reduced?

@NMinhNguyen Yes, I agree with this point at the condition all the third party libraries you want to use follow this pattern. Hard to enforce.

would you be open to me raising a smaller PR that does just that?

Yes, please! I thought it was already done. It can be considered a bug on our end.

[dantman]

@NMinhNguyen The es folder doesn't aim at solving the tree shaking problem. It's a version of Material-UI that is up-to-date with all the currently supported features of the ECMAScript specification. Which mean we will only transpile stage features and apply our custom code transformations. So, it will only work with evergreen browsers.

If the es/ folder's intent is to provide ES6 code for evergreen browsers, could we explicitly support it as such? Perhaps with documentation. There are probably a few users that only support evergreen browsers that would love to use that. Personally I'm working on a project that still supports IE11, however I am considering making separate bundles for modern browsers and switching between them for other reasons (the code size cost of regenerator when 90% of browsers support native generators; and the fact that native Custom Elements implementations require ES6 classes and break with ES5 transpiled code unless you include an extra adapter in modern browsers). If I succeed in that, I'd love to include the full ES6 code in my modern bundle (and instead of switching to the non-es version, I'd just let babel transpile material-ui so that my env preset can take care of transpiling to ES5 for the legacy bundle and handling the possibility that the browsers I consider "modern" may still need a few transforms even if they support class syntax and most of ES6).

I thought we were already applying these transforms for the es folder. We should. Good catch. However, I have one concern with this es folder: it's module duplication. I start to think that the best path going forward is to simply remove it.

Bundling with Rollup should work as long as MUI provides ES modules

Same concern than above. I don't think it's wise to publish different versions of our components. It's gonna bloat users bundle when using third party libraries based on Material-UI. You can already experience the issue with lodash (lodash/x, lodash-es/x, lodash.x).

they actually publish different packages

Modules don't need to be in different packages to introduce duplication in users bundle. Simply the es non es folder can lead to it. I have been giving more thought about it. I'm happy keeping the es folder as long as third party libraries don't use it and people are aware of the duplication cost it can have.

Node.js is going with a .mjs file extension for supporting ES modules natively in node. It's already implemented and is slated for release next month.

This may actually also solve the duplication issue with libraries. Because if you import material-ui/TextField/TextField a require, pre-ESM version of node, and WebPack without special configuration will import the CommonJS code from material-ui/TextField/TextField.js. And if you import from an ES module in node it will first see if there is an ES module from material-ui/TextField/TextField.mjs. As long as the extension is not included an import from the application and a library will use the same import for both CommonJS and future Tree Shaking/ES6 users.

Would you be happy with including a .mjs variant alongside the .js if we convinced WebPack, etc to encourage that?

The only caveat I can think of is that we'd need to check and make sure all our modules are valid ESM modules and won't break when run server side in Node 10.0.

[dantman]

I would definitely like to see es/ officially documented as a power user feature.

I shave 20% of the bundle size of Material-UI, and 35% more off material-ui-icons, just by using an alias to tell WebPack to import the /es variants of MUI and letting babel-loader transpile the /es variant to it to ES5 that works in IE11, instead of having it transpile the pre-compiled ES5 version. Note that when I use the precompiled version without transpiling it, the result is actually 775.08kb. It's probably more efficient for me to run babel myself because I can configure extra optimizations.

[oliviertassinari]

I would definitely like to see es/ officially documented as a power user feature.

@dantman I agree, it would be great.

[dantman]

Also the Minimizing Bundle Size explicitly says:

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

Which is why I never bothered setting it up and made a bug report when things weren't tree shaking correctly. If the consensus is that tree shaking doesn't actually work, even when we do everything right, and the response to any bugs about tree shaking is to try the alternatives. Then the documentation should be updated to recommend users use the alternatives instead of dissuading users who are happy to configure tree shaking from doing so.

[estaub]

fwiw, here's a different experience and motivation:

When using dynamic bundle loading, initial page-load gains can be very much larger than those @dantman's showing above. My initial load mui stat size for about half of my pages is about 300K (30K gzipped). The other half need another bundle, with another 300K (22K gzipped) of mui. A few rarely-used components are sprinkled elsewhere in bundles where they are used. I'm just telling webpack 4 to split things up as it deems best; I'm not hand-tweaking all of this.

I'm trying to keep the app viable for use on slow devices with low bandwidth.