Material-ui-pickers: This library is not tree-shaking friendly

[klis87]

Environment

| Tech | Version |
| -------------------- | ------- |
| @material-ui/pickers | 3.1.1 |
| material-ui | 4.1.1 |
| React | 16.8.6 |
| Browser | any |
| Peer library | date-fns 2.0.0-alpha.34 | |

Steps to reproduce

Tree-shaking with code splitting is problematic due to unnecessary bundling by rollup for es and cjs targets

Expected behavior

Pickers should be packaged like material-ui core, especially pickers are under mui organisation. I am referring to

{
  "main": "./index.js",
  "module": "./esm/index.js",
}

they have in package.json. Notice, they give path to not bundled index.js files. However, for pickers paths target bundled library by rollup.

Rollup is nice for UMD target, but for cjs and es this is really redundant, people using bundlers on app level will bundle vendor files anyway. Prebundling hurts tree-shaking and does not give any advantage what so ever. Webpack tree shaking is based on unused reexports and prebundling library won't have those but a flat structure.

Also, we cannot even do import like import DatePicker from @material-ui/pickers/DatePicker which would target a component with es module, because the only es file is bundled. This is problematic because of Webpack issue https://github.com/webpack/webpack/issues/7782 , so you cannot tree shake a given library across multiple chunks. The remedy is to import components like import DatePicker from @material-ui/pickers/DatePicker, but because this is not of es type, it leads to disaster because material-ui/core components are not imported as es modules, so actually they will be duplicated in date picker chunk.

So please, do not bundle for es and common js targets. And if you really want to bundle for those, do it additionally, but don't link those in main and module in package.json

Actual behavior

As above

Live example

n/a

[dmtrKovalenko]

@TrySound it's your turn :)

[klis87]

Btw, other issues which arises are from #662 for instance. I also had problem with missing context issue due to mixed imports. But my suggestion would solve such issues no matter how you mix imports.

For instance, without bundling, when you import { Datepicker } from '@material-ui/pickers, you really import the same as import Datepicker from '@material-ui/pickers/Datepicker, because 1st import would target index.js:
export { default as Datepicker } from './Datepicker'

But right now, 2nd import will target different file than 1st, again, due to unnecessary prebundling. Even worse, people could have duplicated components instances in their app without even realizing, which would hurt bundle size even more. At least duplicated context issue throwed so issue was obvious :)

[dmtrKovalenko]

Yeah @material-ui/pickers/Datepicker are deprected. They are left ... I don't know why they are here
But it needs to make a new major release to remove them

[klis87]

those imports should be allowed but exactly in the way material-ui/core does it , again, as a consequence of not bundling the app :)

[dmtrKovalenko]

No, we will not allow them. It is to hard to maintain 2 version of builds.
We providing you tree-shakable esm bundle and also cjs for server side rendering and some other cases

[dmtrKovalenko]

I suppose it is quite deprecated way, that will be removed in the nearest major release

[klis87]

But then you make peoples life miserable. Please read arguments I made above. If you don't wanna provide alternative builds, just don't bundle for es/cjs targets. Not bundled esm is tree-shakeable and not bundled cjs files can also be run on server (why to bundle for server usage anyway?)

Please give me at least one argument for bundling for es/cjs with rollup. I gave you tons of serious disadvantages. Plus, you are under @material-ui org and they allow such imports, thanks to which they are friendly library to make optimisations, and I believe you should conform to their way, as pickers are now somehow official mui components.

From maintanance point of view not bundling is simpler than bundling, you just need to bundle for UMD and thats it.

[TrySound]

From maintanance point of view not bundling is simpler than bundling, you just need to bundle for UMD and thats it.

Actually providing esm/cjs support for path imports (like @material-ui/pickers/DatePicker) is not really trivial thing. Bundling and forbidding using internals is much easier for maintenance.

Please give me at least one argument for bundling for es/cjs with rollup.

For node it's startup time. Node resolve algorithm is quite slow. Bundled packages do not require node to resolve all internals. As a result startup time is much smaller. Try to run webpack and rollup cli.

For bundlers it's also less stuff to resolve. So build/rebuild time becomes better.

Another good reason is hiding internals. Material-ui users even now consume a lot of commonjs by importing more deeper than allowed. For example importing internal FormControlContext does not work because inputs consume esm version which is different context.

[dmtrKovalenko]

Yeah, there is a lot of pain with react-context and different imports.
Moreover we will not win a lot by splitting code in several files. We are reusing 60-70% of code for each component. So you will win 1-2kb

[klis87]

@TrySound

Actually providing esm/cjs support for path imports (like @material-ui/pickers/DatePicker) is not really trivial thing. Bundling and forbidding using internals is much easier for maintenance.

I understand hiding internals argument, but you don't need to even officially support those imports, you can even discourage those in docs, but please don't prevent them because tree shaking for Webpack just doesnt work. I just checked and it is even worse than i thought, even when I import DatePicker and provider, without any code splitting, the whole library is bundled. So tree-shaking for webpack users does not work at all. What I suggest is just publishing files with file structure you have in src, and reexport everything from index.js according to official API. Also use babel(or Typescript with cjs target) to replace import with require to provide cjs build, again, not bundled. Anyway, material-ui/core does it correctly so it is a good place to look at.

For node it's startup time. Node resolve algorithm is quite slow. Bundled packages do not require node to resolve all internals. As a result startup time is much smaller. Try to run webpack and rollup cli.

I don't think it will make such a big difference, if material ui doesnt bundle and it is fast, for much smaller library it is just so called prematured optimisation, and not even runtime one.

For bundlers it's also less stuff to resolve. So build/rebuild time becomes better.

Runtime optimisations are much more important than build ones, plus webpack has its own way to cache vendor builds so next time they are faster, in my opinion it is not library author position to worry about webpack optimisations

Another good reason is hiding internals. Material-ui users even now consume a lot of commonjs by importing more deeper than allowed. For example importing internal FormControlContext does not work because inputs consume esm version which is different context.

With great power comes great responsibility, using deeper imports can be always discouraged in docs. But when you use .index.js of es build, and then you will import a deeper component from its subdir, you won't have any issues, as index.js just reexport deeper stuff anyway.

@dmtrKovalenko

We providing you tree-shakable esm bundle and also cjs for server side rendering and some other cases

Probably u are referring rollup, but for Webpack tree-shaking does not work at all, and it is Webpack should be 1st concern as it is much more popular, especially for apps, webpack tree-shaking is based on unused reexports, which u remore by bundling

Moreover we will not win a lot by splitting code in several files. We are reusing 60-70% of code for each component. So you will win 1-2kb

40% of 16.7 is 6.7kb, so I guess it will be much more. Even if not, please take account that big apps can have many dependencies, and each kbs counts. If all libraries had similar approach, suddenly we could talk about 100kbs+ for application initial bundles

All in all, the only argument I buy for bundling es/cjs is hiding internals, but this affects developers only who can anyway be warned in docs

However, we, developers, write programs not for ourselves, but for people, and my arguments clearly show that customers of our apps will suffer. Runtime optimisations should be our focus, not hiding internals or build optimisations.

[TrySound]

And how did you check this? Provide a repro please

[dmtrKovalenko]

Goog example of this library tree-shaking - our docs.
Next.js automatically splits the bundle with a help of webpack.

[klis87]

@TrySound just:

import {
  MuiPickersUtilsProvider,
  DatePicker,
  TimePicker,
  DateTimePicker,
} from '@material-ui/pickers';

vs

import {
  MuiPickersUtilsProvider,
  DatePicker,
} from '@material-ui/pickers';

is giving me the same bundle size. Plus, in my dist I can see time picker related stuff even though I import only DatePicker

But I am not surprised it does not work for Webpack, I admit, Rollup is better at tree-shaking, Webpack just decorates unused reexports with some comments if a package has sideEffects: false and then uglifyJs strips those parts.

I doubled check this and you can trust me, tree-shaking does not work. I respect your time and I would not waste it. But if you prefer to see a repro for reference, I could provide it, but I could do this only tomorrow, as I won't have enough time today.

@dmtrKovalenko Next.js uses webpack under the hood, so if I cannot tree-shake even without code splitting with Webpack, next.js also cannot. Sadly, webpack has some specific requirements to make tree-shaking work, which I explained above. With code splitting this is a different story, you can code split a library if the whole library goes to one chunk, but if you need to have some parts in one chunk, and other one in second chunk, Webpack currently does not support it. The only way to achieve it is to have imports like import DatePicker from '@material-ui/pickers/DatePicker; Not bundling the app would solve those 2 issues at the same time.

Anyway, I appreciate your work on this library, without it material-ui would not be the same, pickers are everywhere ;) That's why I care so much about making build process correct here, because apart from tree-shaking it is so great otherwise :+1:

[klis87]

@TrySound here u are https://github.com/klis87/webpack-tree-shaking

Proof that tree-shaking works for core components and does not work for pickers

[stunaz]

@TrySound ping!

[benmccann]

I'm using material-ui in my project and it pulls in 44.7 KB (uncompressed) worth of components. When I add this library it pulls in 167.2 KB worth of material-ui components in addition to 48.27 KB worth of pickers components.

Using only components required by my project:

After adding pickers:

It should not be necessary to pull in every single material-ui component in order to use the date picker. I generated these images using source-map-explorer

[klis87]

@benmccann it seems that not the whole mui is bundled, just components used by pickers. For instance I cannot see Table component. However, your example is even bigger proof that making pickers treesheakable is crucial, because now u will end up with those mui components bundled no matter what picker you use, cause all pickers and its dependencies will be bundled, again, due to using rollup for es target instead of using just babel like material-ui itself does it

[dmtrKovalenko]

Moving to babel export till defaultProps will not be removed from this project

[klis87]

@dmtrKovalenko big thanks, I confirm tree-shaking works now!

I am not sure how it is related to defaultProps though. Is that what you think prevents rollup esm build to be tree-sheakable?

I did research of my own and I discovered that sometimes esm with rollup does work. For example for Redux - https://github.com/klis87/webpack-tree-shaking/tree/redux

I did not get too deep into this, but I read that webpack has several optimisations allowing tree-shaking. The ultimate power is unused reexports, but this is not the only one. There are more, but they have issues for instance with higher order components. Hopefully Webpack will be smarter and smarter so using Rollup for esm won't cause any tree-shaking problems anymore in the future.

[dmtrKovalenko]

@klis87 I hope it will work. For some reason, webpack treats defaultProps as side-effect and disables treeshaking of the whole file.

[benmccann]

Thanks for this! I can confirm it worked on my project and shaved 90 kb off my bundle