Fix resolution of @mui/material-ui in docs #43108
Conversation
Netlify deploy previewhttps://deploy-preview-43108--material-ui.netlify.app/ Bundle size report |
| @@ -1,8 +1,9 @@ | |||
| import * as React from 'react'; | |||
| import { createTheme, ThemeProvider } from '@mui/material/styles'; | |||
| import { blue } from '@mui/material/colors'; | |||
| import { Box, Stack } from '@mui/system'; | |||
There was a problem hiding this comment.
Also replacing this import from @mui/system to @mui/material. This didn't seem intentional, but correct me if I'm wrong
There was a problem hiding this comment.
Hey @Janpot!
- Is the top-level
@mui/materialimport change something users should do as well? Or does it only break our repo configuration? - Should we add an eslint rule to avoid future imports from
@mui/material?
| @@ -98,8 +98,14 @@ export default withDocsInfra({ | |||
| ...config.resolve.alias, | |||
|
|
|||
| // for 3rd party packages with dependencies in this repository | |||
| '@mui/material$': path.resolve(workspaceRoot, 'packages/mui-material/src/index.js'), | |||
There was a problem hiding this comment.
I'm confused:
'@mui/material$' seems like is added to handle top-level imports from @mui/material, but we're also removing those imports in this PR? 🤔
There was a problem hiding this comment.
but we're also removing those imports in this PR?
Yes, but they're supposed to work, so I left this fix in
There was a problem hiding this comment.
So the fix actually works but this PR includes the changed imports as they're recommended, is that correct?
There was a problem hiding this comment.
Actually, just remember I didn't remove all instances of the top-level import, only the ones in public demos as that is recommended indeed.
We're recommending it. These days, some frameworks automatically do this for you in their compilation. I'm not aware of any discussions around it, but I could see us some day even deprecate this usage.
Yes, ideally. |
Should we include it in this PR or is it out of scope? |
I didn't remove all the top-level imports in the docs, only the ones in public demos. We can expand this override to the docs, but then we must fix all imports. Perhaps we can keep it out of scope? |
Sure 👌🏼 |
Pulling some fixes from #43068
First problem encountered when removing
esmExternalsis around resolution of top level@mui/material. This was fixed by adding a new@mui/material$alias which specifically targets this import and point it to the intended source file.As part of this PR I'm also correcting some imports in the demos and docs modules that import the top level package
There will likely be a lot more work to get this flag removed but I'm going one-by-one. bisecting this takes a lot of time.