[RFR] Fix import errors when tree-shaking @material-ui #4983
Conversation
| @@ -2,7 +2,8 @@ import { render, cleanup } from '@testing-library/react'; | |||
| import * as React from 'react'; | |||
| import expect from 'expect'; | |||
| import { TestContext } from 'ra-core'; | |||
| import { createMuiTheme, ThemeProvider } from '@material-ui/core'; | |||
| import { ThemeProvider } from '@material-ui/core'; | |||
There was a problem hiding this comment.
Shouldn't this be imported from @material-ui/core/styles as well? Same for all other ThemeProvider imports
There was a problem hiding this comment.
That's an interesting case: ThemeProvider normally lives in @material-ui/styles, which means it's re-exported from @material-ui/core and is in theory subject to the same problem we are talking about here. However, I've failed to confirm this so far, still looking into the source code. It doesn't seem to hurt compilation anyway, at least not for a minimal project, but I'll keep digging.
There was a problem hiding this comment.
OK, found it. @material-ui/core/styles exports a couple of exports from @material-ui/styles, while @material-ui/core re-exports everything from @material-ui/core/styles. Now to find out why this works and e.g. makeStyles doesn't.
There was a problem hiding this comment.
I don't think I get it fully, but it seems that ThemeProvider is resolved correctly with the babel rules because at the end of the import chain it's not a part of @material-ui/core but of @material-ui/styles, which is not getting transformed by babel. In theory, everything under core/styles should probably be part of the rule:
colorManipulator.js
createBreakpoints.js
createMixins.js
createMuiStrictModeTheme.js
createMuiTheme.js
createPalette.js
createSpacing.js
createStyles.js
createTypography.js
cssUtils.js
defaultTheme.js
makeStyles.js
MuiThemeProvider.js
responsiveFontSizes.js
shadows.js
shape.js
styled.js
transitions.js
useTheme.js
withStyles.js
withTheme.js
zIndex.js
But the question is probably how to keep up with changes. I think createMuiTheme and makeStyles could be enough for starters and we'll have to see what happens with MUI v5, this problem might disappear in the process.
Technically, you are right, however I think making an exception in the ESLint rule will probably be more complicated than using the right imports in the tests. |
|
Sorry we merged something else. Do you mind rebasing? |
|
@djhi done, thanks! |
|
@djhi sorry for nagging, but it seems this PR is the last one for 3.6.3, is there anything blocking this from being merged, can I help with that in any way? |
|
Sorry for the delay. There were significant changes in the |
|
Thanks for the update, I'll keep an eye on releases and will rebase when 3.6.3 is done. |
|
Or should I just rebase on top of |
|
I've rebased this PR on |
@material-ui documents some tree-shaking performance optimizations at https://material-ui.com/guides/minimizing-bundle-size/. Any downstream project using both Material-UI and react-admin implementing this recommendations will encounter errors like Cannot find module: '@material-ui/core/esm/createMuiTheme'. Make sure this package is installed. This is apparently due to the way @material-ui re-exports styling exports via @material-ui/core. The official recommendation is to import via @material-ui/core/styles, which is what this commit is doing for makeStyles and createMuiTheme. Also included: ESLint configuration forbidding these two imports.
|
Thanks! |
Material-UI documents some tree-shaking performance optimizations at https://material-ui.com/guides/minimizing-bundle-size/. Any downstream project using both Material-UI and react-admin implementing these recommendations will encounter errors like
Cannot find module: '@material-ui/core/esm/createMuiTheme'. Make sure this package is installed.This is apparently due to the way Material-UI re-exports styling exports via
@material-ui/core. The official recommendation is to import via@material-ui/core/styles, whichreact-adminis not doing throughout. This PR is making suremakeStylesandcreateMuiThemeare imported correctly. This is obviously incomplete: any re-exported import from @material-ui/core/styles would probably be a potential problem, however, just includingmakeStylesandcreateMuiThemehas been sufficient to make a minimal example project with the tree-shaking optimization include compilable.Also included: ESLint configuration forbidding these two imports.