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

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

Open
anthonyalayo opened this issue Jan 16, 2023 · 32 comments
Open

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

anthonyalayo opened this issue Jan 16, 2023 · 32 comments
Assignees
Labels
core Infrastructure work going on behind the scenes enhancement This is not a bug, nor a new feature performance priority: important This change can make a difference ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@anthonyalayo
Copy link

anthonyalayo commented Jan 16, 2023

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 #35457 (comment) 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 anthonyalayo added the RFC Request For Comments label Jan 16, 2023
@oliviertassinari oliviertassinari added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 17, 2023
@oliviertassinari oliviertassinari added the package: icons Specific to @mui/icons label Jan 17, 2023
@anthonyalayo
Copy link
Author

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

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

@flaviendelangle
Copy link
Member

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
Copy link
Author

@flaviendelangle that would be great!

@flaviendelangle
Copy link
Member

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
Copy link
Member

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?

@michaldudak michaldudak removed package: icons Specific to @mui/icons status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 19, 2023
@anthonyalayo
Copy link
Author

@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: vercel/next.js#12557 (comment)

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
Copy link
Member

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
Copy link
Author

@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
Copy link
Author

@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
Copy link
Author

@michaldudak bumping the above message in case it got missed

@michaldudak
Copy link
Member

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
Copy link
Author

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

@oliviertassinari oliviertassinari added the scope: code-infra Specific to the core-infra product label Jan 29, 2023
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 29, 2023

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
Copy link
Contributor

joshkel commented Feb 21, 2023

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 oliviertassinari added ready to take Help wanted. Guidance available. There is a high chance the change will be accepted and removed RFC Request For Comments labels Feb 21, 2023
@oliviertassinari
Copy link
Member

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
Copy link

Gu7z commented Jul 15, 2024

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" 😅

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Jul 16, 2024
@Janpot
Copy link
Member

Janpot commented Oct 2, 2024

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
Copy link
Contributor

jaydenseric commented Oct 21, 2024

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.
    • The consistency accross modules makes comparison easier, and reduces conflicts.

    • Can be enforced by one of these ESLint plugins:

    • Example of incorrect:

      import * as React from 'react';
      import { isFragment } from 'react-is';
      import PropTypes from 'prop-types';
      import clsx from 'clsx';
      import chainPropTypes from '@mui/utils/chainPropTypes';
      import composeClasses from '@mui/utils/composeClasses';
      import { styled } from '../zero-styled';
      import memoTheme from '../utils/memoTheme';
      import { useDefaultProps } from '../DefaultPropsProvider';
      import Collapse from '../Collapse';
      import Paper from '../Paper';
      import AccordionContext from './AccordionContext';
      import useControlled from '../utils/useControlled';
      import useSlot from '../utils/useSlot';
      import accordionClasses, { getAccordionUtilityClass } from './accordionClasses';

    • While rewriting imports to deep imports, the imports should not be re-sorted to minimise diff for review. But later, there should be another PR that implements the ESLint rule enforcing sorted imports and a project wide auto fix.

  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
Copy link
Member

flaviendelangle commented Oct 21, 2024

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
Copy link
Contributor

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
Copy link
Member

flaviendelangle commented Oct 21, 2024

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
Copy link
Member

oliviertassinari commented Oct 21, 2024

@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/[email protected]/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. [core] Run @mui/icon-material src:icons #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/[email protected]/index.js but it's signals lack of care.

@flaviendelangle
Copy link
Member

  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
Copy link
Member

oliviertassinari commented Oct 21, 2024

  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:

  • dependencies, e.g. react
  • local dependencies, e.g. @mui/utils
  • relative imports

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
Copy link
Contributor

jaydenseric commented Oct 22, 2024

@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:

{
"name": "@mui/utils",
"version": "6.1.4",
"private": false,
"author": "MUI Team",
"description": "Utility functions for React components.",
"main": "./src/index.ts",
"keywords": [
"react",
"react-component",
"mui",
"utils"
],
"repository": {
"type": "git",
"url": "git+https://github.com/mui/material-ui.git",
"directory": "packages/mui-utils"
},
"license": "MIT",
"bugs": {
"url": "https://github.com/mui/material-ui/issues"
},
"homepage": "private package",
"funding": {
"type": "opencollective",
"url": "https://opencollective.com/mui-org"
},
"scripts": {
"build": "pnpm build:modern && pnpm build:node && pnpm build:stable && pnpm build:types && pnpm build:copy-files",
"build:modern": "node ../../scripts/build.mjs modern",
"build:node": "node ../../scripts/build.mjs node",
"build:stable": "node ../../scripts/build.mjs stable",
"build:copy-files": "node ../../scripts/copyFiles.mjs",
"build:types": "node ../../scripts/buildTypes.mjs",
"prebuild": "rimraf build tsconfig.build.tsbuildinfo",
"release": "pnpm build && pnpm publish",
"test": "cd ../../ && cross-env NODE_ENV=test mocha 'packages/mui-utils/**/*.test.?(c|m)[jt]s?(x)'",
"typescript": "tsc -p tsconfig.json"
},
"dependencies": {
"@babel/runtime": "^7.25.7",
"@mui/types": "workspace:^",
"@types/prop-types": "^15.7.13",
"clsx": "^2.1.1",
"prop-types": "^15.8.1",
"react-is": "^18.3.1"
},
"devDependencies": {
"@mui/internal-test-utils": "workspace:^",
"@mui/types": "workspace:^",
"@types/chai": "^4.3.20",
"@types/mocha": "^10.0.9",
"@types/node": "^20.16.13",
"@types/react": "^18.3.11",
"@types/react-dom": "^18.3.1",
"@types/react-is": "^18.3.0",
"@types/sinon": "^17.0.3",
"chai": "^4.5.0",
"react": "^18.3.1",
"react-dom": "^18.3.1",
"sinon": "^18.0.1"
},
"peerDependencies": {
"@types/react": "^17.0.0 || ^18.0.0 || ^19.0.0",
"react": "^17.0.0 || ^18.0.0 || ^19.0.0"
},
"peerDependenciesMeta": {
"@types/react": {
"optional": true
}
},
"sideEffects": false,
"publishConfig": {
"access": "public",
"directory": "build"
},
"engines": {
"node": ">=14.0.0"
}
}

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:

  • You can not test a package's public API can be imported and used correctly without running npm pack to gather the published artefacts, install that tarball somewhere, then test importing from it. Your current approach will have passing tests even if that module isn't in the npm packages files, or if it's in the package .npmignore.
  • You are only testing one way the module can be imported, but MUI offers multiple; via deep import and via the package main index. It's possible for package exports to allow access one way, but not the other.
  • You are not testing the package exports several times using require, import, etc. because package exports can be different for each and it's possible for one to work and not the other.

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?

  • A lot of contributors (like me!) are used to the productivity gain of being able to alphanumerically scan down the list of imports to quickly find what you are interested in looking at.
  • When comparing two files side by side, when most of the imports are the same but they are ordered differently, it's harder to see what is different.
  • When different people are working on the same file, the current tooling allows them to add the same imports in different places. When the second PR gets merged, it will attempt to insert the same import again in the other location. This is a merge conflict that could have been avoided if the imports were sorted.

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes enhancement This is not a bug, nor a new feature performance priority: important This change can make a difference ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
Status: Selected
Development

No branches or pull requests