[core] Improve tree-shakeability

[eps1lon]

As discussed #11281 and per suggestion of @TrySound via #11281 (comment)

Put the build output in version control and detected no breakage. I would recommend that we release this in a pre-release to get some feedback from e.g. parcel people just in case that additional package.json causes issues with other bundlers.

Overall I would recommend anyone going down that rabbit hole to not try to hard. Tree shaking is only really usefull for barrel files or if you import from large files and as said in #11281 (comment) you can always encounter gzip deoptimizations with small changes to the original files.

Savings displayed: /~https://github.com/mui-org/material-ui/pull/13391/files#diff-0b6b5aecf13927e3ba1fc7c40204d7c8

Closes #11281

[eps1lon]

This was used in the cli with --ignore **/*.test.js. Most shells already expand those. If you want to pass a glob to a node script you have to escape it with ". I've made that mistake myself and it's quite common. Only prettier documents this well.

[eps1lon]

@oliviertassinari Are you sure about sideEffects: false? withStyles does have a side effect on evaluation with /~https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/styles/withStyles.js#L25. Especially considering a possible class mismatch between client and server?

[oliviertassinari]

@eps1lon I believe the sideEffects option should be true when performing global mutations. We don't do that.

[oliviertassinari]

What's the advantage of the nested package.json solution?

[eps1lon]

@eps1lon I believe the sideEffects option should be true when performing global mutations. We don't do that.

I thought so too but in the context of webpack it means basically that that file only re-exports and doesn't e.g. call anything

A "side effect" is defined as code that performs a special behavior when imported, other than exposing one or more exports. An example of this are polyfills, which affect the global scope and usually do not provide an export.

--- https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free

But I think setting it to true only affects the index file not every individual file.

What's the advantage of the nested package.json solution?

Previously if we did something like import { onlyOneHelper } from './someBigHelperBarrel'. We would include everything from someBigHelperBarrel. With this change webpack will shake someBigBarrel and only pull in onlyOneHelper. (Barrel files are files that only re-export; mostly index.js)

In the experiment I did I used Typography which uses import { capitalize } from '../utils/helpers';. Before the complete helper.js would be bundled. Now it won't.

But these are all only very small reductions since we already split out functionality across multiple files and rarely do named imports. It's only interesting if we import small functionality from bigger files. The biggest reductions would probably be achieved if we import only little from 3rd party libraries. Something like import { someUtil } from 'lodash'.

[TrySound]

@oliviertassinari Nested package.json will allow to provide esm for "path imports" style like in preferred in material-ui. esm is bundler more effectively then commonjs. For example colors reexports cannot be analyzed and they are easily wrapped with commonjs iife to handle its dynamism. This adds a lot of bytes and prevents treeshaking.

Also hooks. They allow to use only functions which are perfect for treeshaking. Commonjs will still prevent it because of dynamism and unpredictability.

Nested package.json is less opinionated than mjs which has a lot more problems with interop.

[TrySound]

sideEffects is not predictable. And it doesn't work with rollup which is my bundler for production. So it doesn't help a lot. It broke react-virtualized styles a few months ago.

[oliviertassinari]

I thought so too but in the context of webpack it means basically that that file only re-exports and doesn't e.g. call anything

@eps1lon Creating a className generator isn't an issue on its own. The issue is about how it's used. I think that it's fine.

It's only interesting if we import small functionality from bigger files. The biggest reductions would probably be achieved if we import only little from 3rd party libraries. Something like import { someUtil } from 'lodash'.

Thank you for provide this level of details. One thing I'm unsure, by "we" you mean a Material-UI user right?

Nested package.json will allow to provide esm for "path imports" style like in preferred in material-ui.

@TrySound "Path imports" style is only preferred in Material-UI because tree-shaking isn't always working. I would rather do import { Button } from '@material-ui/core'; if tree-shaking is reliable!

For example colors reexports cannot be analyzed and they are easily wrapped with commonjs iife to handle its dynamism.

This is a good one.

[TrySound]

Treeshaking would work if we rewrite the whole material-ui with only hooks so all components could be functions which can be treeshaked just fine.

[TrySound]

This PR will let me remove these lines from my rollup config which is great!

        '@material-ui/core/styles': [
          'createGenerateClassName',
          'createMuiTheme',
          'jssPreset',
          'MuiThemeProvider',
          'createStyles',
          'withStyles',
          'withTheme',
        ],
        '@material-ui/core/colors': [
          'amber',
          'blue',
          'blueGrey',
          'brown',
          'common',
          'cyan',
          'deepOrange',
          'deepPurple',
          'green',
          'grey',
          'indigo',
          'lightBlue',
          'lightGreen',
          'lime',
          'orange',
          'pink',
          'purple',
          'red',
          'teal',
          'yellow',
        ],

[eps1lon]

[oliviertassinari]

Wow, this looks pretty nice! 🤩 Can we rebase to have the latest size-limit from next?

[oliviertassinari]

Do we need to clone the array?

[eps1lon]

Not really. I guess this is just left from reducing the plugins to a single group. Will fix it later.

[eps1lon]

I need to resolve the conflicts anyway before merging. We will see exactly how much KB we save with this.

[eps1lon]

Merged. Special thanks to @TrySound for the nested package.json solution.

[oliviertassinari]

I can't wait to try it out :)

[eps1lon]

I can't wait to try it out :)

@oliviertassinari I'm in favour of doing frequent alpha releases. As far as I understand it it takes quite some time so maybe once per week so that users can track progress or incrementally migrate.

[oliviertassinari]

@eps1lon I agree. Once per week, as we have been doing it over the last 2 years, sounds great to me.

[mbrowne]

Even after refactoring to all function components so that tree-shaking works perfectly, I hope that this library continues to use the nested package.json approach (or I suppose mjs files would work too, if that becomes more viable). Why? Because it makes dynamic imports (import()) efficient.

Suppose you have a page that's being generated by a CMS tool that allows the user to select display options that affect which React component should be used to render a particular section. For a completely dynamic scenario like that, it's great that it's possible to do import('@material-ui/core/' + componentId) (for example). If only import('@material-ui/core') were possible, you'd be forced to import the entire package even if you only needed one or two components.

In addition to putting in my two cents about the future of material-ui, I am considering the package.json approach for a shared library of UI components at my company (inspired by material-ui's implementation). So if there is some equally good or better way to accomplish dynamic importing of components with code splitting, I welcome and appreciate any corrections to what I said above.

[mbrowne]

P.S. It turns out that import('@material-ui/core/' + window.testComponentId) doesn't actually work for the couple of material-ui components I tested (I get an error "Can't resolve './props'"). But something like import('@material-ui/core/Button') does work, and I'm guessing it probably loads less code than trying to do tree-shaking with dynamic imports. I could open a new issue about the dynamic imports problem, but I'm not sure if supporting fully dynamic imports is a goal of this library.

[eps1lon]

I could open a new issue about the dynamic imports problem, but I'm not sure if supporting fully dynamic imports is a goal of this library.

That would help a lot. I haven't tested how dynamic imports work right now so I don't know what's working and what is not. I would be careful with wildcard chunks in webpack since not every @material-ui/component/* is a component. Especially core/test-utils might cause some issues.

But let's continue this discussion in a separate issue.

[mbrowne]

Sure, I opened a new issue about it (#17314)...after further investigation, it looks like there are probably some issues with webpack itself...

BTW (totally unrelated), I noticed that you all have been debating about possibly switching to styled-components. I really like styled-components, but I found recently that it introduces a number of issues that make it difficult to make tree-shaking work consistently (this is one of the issues; there are others). Just thought you might be interested to know.

[joshwooding]

@mbrowne Thanks for the valuable feedback about styled-components, do you mind adding it to #6115 ?