mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.75k stars 32.24k forks source link

[core] Flatten imports to speed up webpack build & node resolution #35840

Open anthonyalayo opened 1 year ago

anthonyalayo commented 1 year ago

What's the problem? πŸ€”

Background

While playing around with Next.js, I installed a package that was using @mui. I immediately noticed huge delays running next dev and the large module count (10K+) listed in the terminal. Searching for an answer, I landed on a long list of requests for help:

But none of those had an actual answer with an actual solution, just guesses and workarounds. One of the popular answers was: https://mui.com/material-ui/guides/minimizing-bundle-size/

After attempting to fix the library I pulled in by following the first option on that guide: https://mui.com/material-ui/guides/minimizing-bundle-size/#option-one-use-path-imports

I noticed that the module count was still in the thousands. Why?

After reading everything on webpack, modules, tree shaking, etc, I made a demo application using CRA and a single import to @mui to showcase the problem.

Problem

Reproduction Steps

  1. Run npx create-react-app cra-test to get the latest with Webpack 5
  2. Ejected from CRA so that I could modify the webpack config for more metrics
  3. Using webpack dev server, I modified this to see what modules traversed in development
  4. Using webpack prod builds, I modified this to see modules traversed in production

I used the following configurations for stats.toJson() to balance the verbosity of the output. I tested with and without default imports for a single icon and component.

  1. With default import {assets: false, chunks: false, modulesSpace: 6, nestedModules: false, reasonsSpace: 10}
  2. With path import: {assets: false, chunks: false})

Demo App 1 - Default Import Icon

This is the scenario that we are told to avoid when using @mui, and for good reason. I created this as a baseline to see what webpack had to traverse.

import { AbcRounded } from '@mui/icons-material'

function App() {
  return (
      <div>
        <AbcRounded />
      </div>
  );
}
export default App;

Demo App 1 - Webpack Metrics Output

As expected, using the default import for AbcRounded ended up pulling in all icons.

Webpack Module Summary

orphan modules 6.28 MiB [orphan] 10860 modules
runtime modules 28.5 KiB 14 modules

image

Demo App 2 - Path Import Icon

Following the docs, you would expect an import like this to be lightweight. It isn't.

import AbcRounded from '@mui/icons-material/AbcRounded'

function App() {
  return (
      <div>
        <AbcRounded />
      </div>
  );
}
export default App;

Demo App 2 - Webpack Metrics Output

This is the problem. Importing a single icon resulted in:

  1. Importing ./utils/createSvgIcon
  2. Importing @mui/material/utils
  3. Importing a lot more...(doesn't fit in a single screenshot)

Webpack Module Summary

orphan modules 534 KiB [orphan] 274 modules
runtime modules 28.5 KiB 14 modules

image

Demo App 3 - Path Import Component

This problem isn't unique to @mui/icons-material either. Here's performing a path import of a button.

import Button from '@mui/material/Button'

function App() {
  return (
      <div>
          <Button>Hello World</Button>
      </div>
  );
}
export default App;

Demo App 3 - Webpack Metrics Output

Again, the problem. Importing a single button resulted in:

  1. Importing ./buttonClasses
  2. Importing @mui/utils
  3. Importing a lot more...(doesn't fit in a single screenshot)

Webpack Module Summary

orphan modules 573 KiB [orphan] 291 modules
runtime modules 28.5 KiB 14 modules

image

We should not be importing hundreds of modules from a single icon or button.

But... Tree Shaking? Side Effects?

This was confusing for me too, and I had to go into the details to find the answer.

  1. Webpack does not tree shake at the code level, it only tree shakes at the module level.
  2. Terser performs dead code elimination when minifying, but webpack still has to traverse all those imports!
  3. This happens whether you are building for development or production.
  4. We "cue up" our code for deletion by using ESM, but it isn't done until the minification happens.

Yes the bundle will still be minimized successfully when following ESM practices, but thousands of modules being traversed bloats memory and slows down development servers.

What are the requirements? ❓

Importing from @mui should always result in a minimal dependency graph for that particular import.

What are our options? πŸ’‘

Option 1 - Proposal

Apply transformations to the @mui build process to ensure a minimal dependency graph for all files.

Option 2 - Alternative

Remove all barrel files from @mui. This option isn't great as the developer experience that they provide is desired by both library maintainers and library users alike.

Proposed solution 🟒

  1. Apply import transformations within @mui packages.

Showcased above, even when importing a component or icon directly, thousands of downstream modules get pulled in. This happens because within @mui itself, barrel files are being utilized for their developer experience. In that case, why not follow the same recommendation that @mui gives, and add these transforms to the build process?

  1. Make "modularize imports" work for all @mui packages

In [docs] Modularize Imports for Nextjs, the comment https://github.com/mui/material-ui/pull/35457#issuecomment-1375499537 requested that the docs don't include @mui/material for import transformations via babel or swc, since there are outliers that cause the transformation to fail.

Instead of backing off here, the work should be put in to fix it. The same barrel files that @mui is using internally for better developer experience is what users of the library need as well. By fixing point 1, this will come for free.

Resources and benchmarks πŸ”—

Below are the webpack metrics I collected for the applications in the background statement: mui-default-import-icon-truncated-metrics.txt mui-path-import-button-metrics.txt mui-path-import-icon-metrics.txt

anthonyalayo commented 1 year ago

@oliviertassinari this issue isn't specific to icons, please see "Demo App 3":

This problem isn't unique to @mui/icons-material either.

flaviendelangle commented 1 year ago

We could enforce a few rules to improve the amount of dependencies listed. On of them being to never import from the root of @mui/utils and @mui/system.

anthonyalayo commented 1 year ago

@flaviendelangle that would be great!

flaviendelangle commented 1 year ago

I think Olivier added the icon label because it is the scenario with the most obvious gains. An icon is a super small component and we are looking through a full package (@mui/system at least). For regular component, I don't think you should expect the list of files visited to be small, but we can definitely make improvements.

By the way, on the X components (data grid and pickers), we are probably even worse than that. But the components being big by themselves, it's less notceable. We can still improve though.

michaldudak commented 1 year ago

Thanks for such a detailed report, @anthonyalayo! I agree with @flaviendelangle, we can start with creating an eslint rule to disallow imports from barrel files. It could be better than introducing a build transform, as anyone reading the source code will see the proper way of importing other modules.

@anthonyalayo, would you be interested in working on this?

anthonyalayo commented 1 year ago

@michaldudak thanks for going through it πŸ˜„ sure I'm interested, but I wanted to hit on that point you just mentioned:

we can start with creating an eslint rule to disallow imports from barrel files. It could be better than introducing a build transform, as anyone reading the source code will see the proper way of importing other modules.

I considered this (as it's the easier way to go code change wise), but I also noted in the proposed solution that the developer experience would be affected. A tangential discussion happened on a Next.js issue, and the audience was quite in favor of barrel files: https://github.com/vercel/next.js/issues/12557#issuecomment-1309240211

On top of that, Next.js recently released modularizeImports for SWC, since babel-plugin-transform-imports was quite nice for developer experience.

While bundlers understand these barrel files and can remove unused re-exports (called "dead code elimination"), this process involves parsing/compiling all re-exported files. In case of published libraries some npm packages ship barrel files that have thousands of modules re-exported, which slows down compile times. These libraries recommended babel-plugin-transform-imports to avoid this issue, but for those using SWC, there was no previous support. We've added a new SWC transform built into Next.js called modularizeImports.

Leveraging this transform with @mui/icons-material or lodash allows skipping compilation of unused files.

In the absence of these features, I think the only solution would be an eslint rule to reject barrel file usage. But since we already are somewhat across the goal post for good DX and performance, why not take it all the way?

michaldudak commented 1 year ago

By "taking it all the way" you mean fixing the issues that prevent babel and modularizeImports transforms to work, right? I haven't looked into this much, but I fear we won't be able to change the structure of our packages without introducing breaking changes. We can certainly look into it for the next release, as we're going to change how things are imported anyway (by introducing ESM and import maps)

anthonyalayo commented 1 year ago

@michaldudak agreed, it would be a breaking change (since some import locations would move to be consistent), so I think looking into it for the next release sounds reasonable to me.

With that being said, I can definitely do the eslint rule and import fixes associated with it. I'll make a PR for it an attach it to this issue.

anthonyalayo commented 1 year ago

@michaldudak I did an initial attempt, but there's quite a bit of eslint configs/overrides setup already with 'no-restricted-imports': https://github.com/mui/material-ui/blob/master/.eslintrc.js

I also noticed that paths doesn't support wildcards, so it would have to look something like this (unless someone chimes in with a better solution):

    'no-restricted-imports': [
      'error',
      {
        "paths": ['@mui/material', '@mui/material/utils', '@mui/styles', '@mui/base', '@mui/utils'],
      }
    ],

Is there anyone at @mui that could join in the conversation for how they would like it ideally?

anthonyalayo commented 1 year ago

@michaldudak bumping the above message in case it got missed

michaldudak commented 1 year ago

Unfortunately, "patterns": ["@mui/*"] doesn't seem to do the trick here. Explicitly listing all the forbidden paths seems reasonable.

cc @mui/code-infra for visibility and perhaps other opinions

anthonyalayo commented 1 year ago

Sounds good, If no other opinions from @mui/code-infra i'll do that then

oliviertassinari commented 1 year ago

This problem isn't unique to @mui/icons-material either. Here's performing a path import of a button.

@anthonyalayo I think that the number of modules isn't this relevant. We should focus more on the metrics that directly impact developers:

  1. built time in dev mode
  2. load time in dev mode

I have added the icon's label because so far, I don't think that it was ever proven that the problem goes beyond icons. https://mui.com/material-ui/guides/minimizing-bundle-size/#development-environment mentions a build time of x6 for icons, but for a button, back then, it was like 50%, mostly negligible.

It could be great to measure again, it was a long time ago.

On of them being to never import from the root of @mui/utils and @mui/system.

πŸ‘ agree, to keep doing it (I think that we started doing this a long time ago).

joshkel commented 1 year ago

I've been looking into what I think is the same root issue: I'm trying to speed up our Jest test suite. Jest by default executes each of its suites in an isolated Node context, which means all of the tests' dependencies have to be re-loaded, parsed, and executed for every test module. Tree-shaking doesn't apply, and Babel typically isn't run on node_modules for Jest (so babel-plugin-import or babel-plugin-direct-import won't help), and the costs are incurred on every test execution (in watch mode, every time a file is saved, versus just when webpack-dev-server is starting up). MUI packages' use of barrel imports from other MUI packages seems to have a noticeable impact here, so I'm interested in this area of work as well.

Should @mui/base be added to the list of "don't do a root import" packages, especially as more of @mui/material and @mui/joy start using it? (In some crude local testing, replacing barrel imports of @mui/base speeds up a single test module by 2.5% - not much, but measurable, for an process that's run all the time during development.)

If this isn't the same issue or would be better tracked separately, please let me know.

oliviertassinari commented 1 year ago

I'm trying to speed up our Jest test suite.

@joshkel If you didn't read it before, check this out: https://blog.bitsrc.io/why-is-my-jest-suite-so-slow-2a4859bb9ac0, and search for "Barrel files".

Barrel files Now that we have the inspector hooked up, we can immediately see the problem β€” almost all of our time loading the test file is spent loading the @mui/material library. Instead of loading only the button component we need, Jest is processing the entire library.

But yeah, the solution they propose is already applied by the developers that are landing on this issue.


I have added the good to take GitHub label, the problem, and solution are clear: the community can help. We need to flatten our internal imports, especially because competitive libraries that use one npm package per component are forced to flatten, and hence get a hedge. In our case, we can have the same forcing constraint with eslint.

A closing note, let's please benchmark, so we can have a nice Twitter/release note mention about the actual win this brings to the community. I'm sure developers are always happy to hear about efficiency wins 😁

oliviertassinari commented 1 year ago

Removing assignments as it could be confused into this is something the person is working on. I assume nobody is working on it right now.

joacub commented 1 year ago

Removing assignments as it could be confused into this is something the person is working on. I assume nobody is working on it right now.

is this going to be done by mui or are you asking developers to help on this ??, we are getting really slow development with nextjs 13, really really slow.

kevcmk commented 1 year ago

As of right now, I would urge anyone considering using NextJS 13 to avoid Material UI v5

oliviertassinari commented 1 year ago

As of right now, I would urge anyone considering using NextJS 13 to avoid Material UI v5

@kevcmk Which issue are you facing?

kevcmk commented 1 year ago

Material UI documentation will send you down the complete wrong path if you're using next.js.

@oliviertassinari After looking through your posts on other code reviews, I tracked down this code (specific to Next.js), which finally did the trick. Thank you for this ↓

const nextConfig = {
...
modularizeImports: {
    "@mui/material": {
      transform: "@mui/material/{{member}}",
    },
    "@mui/icons-material": {
      transform: "@mui/icons-material/{{member}}",
    },
  },
}

module.exports = nextConfig

and, if you're like me, you ran into issues with, for example

import { useTheme } from "@mui/material"; # Will fail given the above next.config modification.

Use this instead

import { useTheme } from "@mui/material/styles";

Also, thank you for your work on this project and your effort on the issues board.

oliviertassinari commented 1 year ago

This issue is up for grabs for the community. It should be relatively easy to make progress. It's a matter of have deeper imports, avoiding barrel index files.

Gu7z commented 3 months ago

We could enforce a few rules to improve the amount of dependencies listed. On of them being to never import from the root of @mui/utils and @mui/system.

Has the change to avoid importing directly from the root of @mui/utils and @mui/system been implemented yet?

I'm experiencing issues with exports as mentioned in the issue description. By merely importing an icon with `import Abc from '@mui/icons-material/Abc', the build time in a simple React + Webpack app increased from 3 seconds to 17 because Material adds roughly 500 internal packages.

"@emotion/react": "11.10.6", "@emotion/styled": "11.10.6", "@mui/icons-material": "5.11.16", "@mui/material": "5.11.16", "@mui/styles": "5.11.16",

Stuck at "react": "17.0.2" πŸ˜…

Janpot commented 3 weeks ago

Is there anyone at https://github.com/mui that could join in the conversation for how they would like it ideally?

@anthonyalayo or if anyone is still interested in tackling this. I believe a good starting point would be to update .eslintrc.js with

diff --git a/.eslintrc.js b/.eslintrc.js
index 038be57d26..c178890cb3 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -457,7 +457,17 @@ module.exports = {
         'no-restricted-imports': [
           'error',
           {
-            paths: NO_RESTRICTED_IMPORTS_PATHS_TOP_LEVEL_PACKAGES,
+            paths: [
+              ...NO_RESTRICTED_IMPORTS_PATHS_TOP_LEVEL_PACKAGES,
+              {
+                name: '@mui/system',
+                message: OneLevelImportMessage,
+              },
+              {
+                name: '@mui/utils',
+                message: OneLevelImportMessage,
+              },
+            ],
           },
         ],
         // TODO: Consider setting back to `ignoreExternal: true` when the expected behavior is fixed:

And then address the ~250 warnings this generates. Most of them will be about rewriting things from e.g.

import { getDisplayName } from '@mui/utils';

to

import getDisplayName from '@mui/utils/getDisplayName';

Some of them may be a bit more challenging such as

import { Interpolation } from '@mui/system';

There we may first have to push those re-exports deeper in @mui/system so that we can import them as

import { Interpolation } from '@mui/system/styled';
jaydenseric commented 5 days ago

I'm working on this, but firstly, it's good to establish the ideal end state. The imports in the MUI codebase are a hot mess of anti-patterns…

  1. Type imports/exports should use the TypeScript type syntax.

  2. Internal imports within a package should be relative (not using the packages' own name).

  3. Cross-package imports should use the package name (no relative imports across packages).
  4. Cross-package imports should be direct from the source, and not via a re-export.

  5. Import paths should be fully specified (including the file extension, .js).

  6. Imports should be sorted.

  7. Imports should be deep; never via a barrel module.

  8. There should not be multiple import statements when importing from the same module.

Some of the above anti-patterns I have listed how tooling can be used to enforce good patterns, but others I still have to figure out the right ESLint/TS config.

flaviendelangle commented 4 days ago

Internal imports within a package should be relative (not using the packages' own name).

This is a test file, it's not inside the package

I agree with some of the proposals (sorting the imports is something I would love to for quite some time but never took the effort to do so.

For others I have by doubts tbh

jaydenseric commented 4 days ago

This is a test file, it's not inside the package

It's a module within a package boundary; it just happens to not be published.

I agree with some of the proposals (sorting the imports is something I would love to for quite some time but never took the effort to do so.

For others I have by doubts tbh

Sorting imports has the least functional improvement of all the recommendations. What do you doubt about the other points? Do you just doubt some, but not all points?

flaviendelangle commented 4 days ago

It's a module within a package boundary; it just happens to not be published.

Then I'm not following what is the point of your whole post. If it's to improve the life our the developers using the MUI packages, then how do it matter how the test files are importing what they need?

Sorting imports has the least functional improvement of all the recommendations. What do you doubt about the other points? Do you just doubt some, but not all points?

I have doubts about the viability of something like There should not be multiple import statements when importing from the same module. in a codebase with as many exports as the MUI packages. For @mui/utils I'm sure it's viable (and most exports already only have one element). But for the main packages I think it would make the end DX worse. When we have two elements that are always used together, exporting them from the same file makes that clear. We always export the component and the method to build classes for this component together for example and I think we should continue to do so.

oliviertassinari commented 4 days ago

@jaydenseric Thanks for looking into it!

On the labeling for "hot mess of anti-patterns", for each point:

  1. Mixed feelings: "some projects use transpilers such as Babel, SWC, or Vite that don't have access to type information" https://typescript-eslint.io/blog/consistent-type-imports-and-exports-why-and-how/. I can see an example of those, https://unpkg.com/browse/@mui/joy@5.0.0-beta.48/Divider/index.js with DividerProps. What we do here is wrong, we should import type. However, doing this systematically sounds like a distraction. Ignoring this and using a TypeScript aware transpiler sounds better.
  2. Disagree, this is by design. It guarantees that those test can't test private APIs.
  3. Example?
  4. Agree, we started to reorganize the code but we never finished the work.
  5. Agree for what is publish on npm, e.g. https://github.com/mui/material-ui/pull/44097. For the source, not clear it's better https://devblogs.microsoft.com/typescript/announcing-typescript-5-7-beta/#path-rewriting-for-relative-paths, sounds like a distraction.
  6. Disagree, distraction
  7. βœ… Agree, what this issue is about πŸ‘ (though it's not about removing barrel index, but not using them everywhere possible)
  8. Agree, it looks like it won't make a difference https://unpkg.com/browse/@mui/utils@6.1.4/index.js but it's signals lack of care.
flaviendelangle commented 4 days ago
  1. Disagree, distraction

I tend to disagree on that one, I find that codebase with organized imports are more readable and maintainable, and this is easily handled by ESLint or even Prettier.

But for me it's totally out of the scope of this issue though

oliviertassinari commented 4 days ago
  1. Disagree, distraction

@flaviendelangle we used to sort all the imports on the codebase. I think that we should continue to do it.

We would sort them in 3 buckets:

However, under "hot mess of anti-patterns" I disagree, I think it would be a distraction to work on this first. I also fail to see how its not already sorted in the example provided. Meaning, why would it be better with a different sorting?

But for me it's totally out of the scope of this issue though

πŸ‘

jaydenseric commented 4 days ago

@oliviertassinari

Regarding (1):

It's very easy to autofix this project wide with ESLint, it improves quality so I don't see why not to enforce it. It makes code much easier to read and understand what imports are types and what represents runtime code.

I don't feel a sense of urgency about actioning this because it can be done by anyone pretty easily; I only have a week right now to contribute my expertise for optimal module design to hopefully solve a bunch of problems we have been experiencing with MUI for some time now at work. We want it to work in native Node.js ESM projects, and we have been having a lot of issues with absurd bundle sizes for deep imports of specific components due to all the suboptimal internal imports via barrel files, etc. We have unit tests in our design system that assert the esbuild bundle size per component our UI library exports, and due to the way MUI is structured, regularly they expand for seemingly no reason because of how much unrelated code gets sucked in.

  1. Disagree, this is by design. It guarantees that those test can't test private APIs.

Firstly, the MUI codebase sometimes relative imports the main thing being tested, other times it uses the package name. There is no tooling enforcing consistency.

Secondly, any modules authored in a Node.js project should conform to the Node.js resolution rules. Here are the rules for self referencing package imports:

https://nodejs.org/api/packages.html#self-referencing-a-package-using-its-name

For ESM, it's invalid unless the package has an exports field and what is being imported is exported.

At this time, the packages do not have an exports field and as such the style of imports you are insisting on are actually invalid:

https://github.com/mui/material-ui/blob/1c7f61f25bdd6411e5c6fa765720c0ea126045c9/packages/mui-utils/package.json#L1-L80

Thirdly, even if in the future the packages were to have an exports field, the motivation for using self-referencing package name imports being to test the public API works is flawed:

Finally, it's best for unit tests to be co-located next to the module being tested, with a relative import of the sibling module. You can move those modules together to a different directory or even package and you don't need to rewrite the import path. You can decide to change if the module is public or private via the package exports field without having to make code changes to module imports within the package. Conceptually, a unit test should only be testing that unit. What you are trying to do, is test the package itself, which composes such units. As such, you should have package level tests for that.

Regarding (5):

For the source, not clear it's better

It's always better to write valid ESM instead of using exotic tooling configurations to allow you to write invalid code in source modules. It's better for contributors to understand (and enforce with tooling) one set of rules for published code and source code.

Regarding (6):

Disagree, distraction

We can agree to disagree on this, and have it your way because it doesn't cause misery in MUI user's projects like the other issues do.

I also fail to see how its not already sorted in the example provided. Meaning, why would it be better with a different sorting?

I don't massively care how the imports are deterministically ordered (although I have personal preferences which align with the default for eslint-plugin-simple-import-sort); the main thing is that they are deterministically ordered (and not just grouped).

Again, I don't feel urgency to push this like the other issues, and it could be actioned later very easily with a big ESLint autofix :)

oliviertassinari commented 4 days ago

I will be curious to get code-infra view on this.

We want it to work in native Node.js ESM projects, and we have been having a lot of issues with absurd bundle sizes for deep imports of specific components due to all the suboptimal internal imports via barrel files, etc. We have unit tests in our design system that assert the esbuild bundle size per component our UI library exports, and due to the way MUI is structured, regularly they expand for seemingly no reason because of how much unrelated code gets sucked in.

@jaydenseric Ok, so more about #43938

  1. It makes code much easier to read and understand what imports are types and what represents runtime code.

I don't know. It also feels that it's extra work for maintainers without clear DX value. The UX value of smaller bundle export makes sense, but it's more about being type aware when transpiling.

  1. the MUI codebase sometimes relative imports the main thing being tested, other times it uses the package name.

We didn't complet this migration yet.

  1. any modules authored in a Node.js project should conform to the Node.js resolution rules. Here are the rules for self referencing package imports:

We don't use Node.js resolution directly nor did we try to. Babel operates in between and rewrite those imports.

  1. to test the public API works is flawed

It definitely has its limits, but npm pack issue is workedaround by following stronge conventions, same for the exports package.js field. As for the different import ways, we only recommend the one level import, so only uses this one. Both are aliased. The goal is to know that if the test works, we know we don't test a private API. It seems good enough for this.

  1. It's always better to write valid ESM

I don't know about this. If Node.js default resolution is poor DX, and we have to transpile the source anyway, then I don't see the value.

  1. alphanumerically scan down

It sound like how material-ui-pickers source was configured. I disabled his rule, it was a lot of busy work. Eslint is not an acceptable solution for this IMHO. It would at least need to be Prettier based so it's automated, but even then, it seems not important enough for developers to have to be distracted by this.