-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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] Allow deeper import of @mui/utils #38806
[core] Allow deeper import of @mui/utils #38806
Conversation
Netlify deploy previewhttps://deploy-preview-38806--material-ui.netlify.app/ Bundle size report |
164cdea
to
833cd63
Compare
833cd63
to
c7a17be
Compare
@@ -0,0 +1,15 @@ | |||
import PropTypes from 'prop-types'; | |||
|
|||
declare function integerPropType( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this related to the rest of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c7a17be
to
80f2978
Compare
80f2978
to
d03dc0a
Compare
70730f9
to
48cb743
Compare
This reverts commit c656dac.
// Begin block: Packages with files instead of packages in the top level | ||
// Importing from the top level pulls in CommonJS instead of ES modules | ||
// Allowing /icons as to reduce cold-start of dev builds significantly. | ||
// There's nothing to tree-shake when importing from /icons this way: | ||
// '@mui/icons-material/*/', | ||
'@mui/utils/*', | ||
// End block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the pattern we want to allow, removing.
// Allow deeper imports for TypeScript types. TODO? | ||
// Allow deeper imports for TypeScript types. TODO remove | ||
'@mui/*/*/*/*', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types should be exposed at the same level as the other modules.
@@ -3,6 +3,7 @@ | |||
// Actual .ts source files are transpiled via babel | |||
"extends": "./tsconfig.json", | |||
"compilerOptions": { | |||
"composite": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost the only package without this flag on https://www.typescriptlang.org/docs/handbook/project-references.html#composite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should aim to introduce this flag on all our projects (after we deal with circular references) to speed up the build times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I wrote a typo: with without.
@@ -0,0 +1,15 @@ | |||
import PropTypes from 'prop-types'; | |||
|
|||
declare function integerPropType( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should unlock to go through the codebase to update all the imports. At which point, I think we should add an eslint rule to prevent barrel level import, and benchmark the difference, see if there is shareable material to get developers excited about. |
A quick step to illustrate my point in mui/mui-x#10215.
It's also a step toward solving #35840.
Edit: oh wow ok, looking at the source, this PR is the first time we import from a folder of @mui/utils in MUI Core. Only MUI X has been doing this so far. I guess it's why I had to change a couple of code infra files.