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.86k stars 32.26k forks source link

Information: solving jss classnames production build conflict with external MUI dependent component #11628

Closed nihaux closed 6 years ago

nihaux commented 6 years ago

So I spend the all afternoon reading the related issues and debugging my code so I am posting this here in case it helps someone.
I can make a PR for the doc if you want but don't know where it should go as it is not directly a MUI issue.

Use case:

If you do this, you'll end up having jss classname conflicts in the production build of you final app.

why ?

cause @material-ui/core does not avoid @material-ui/core/List for instance to be included in your dist package.

and @material-ui/core/List will include withStyles which will cause jss class naming conflict in your final package !

So the solution is, you should put this in your rollup config:
external: id => /react|react-dom|material-ui\/.*/.test(id),

this way all material-ui components will be excluded from your build... lost some hair with this one :p .

oliviertassinari commented 6 years ago

@nihaux Bundling withStyles twice is definitely not a good idea. A less optimal alternative would have been to use <JssProvider /> to share the same class name generator. Do you have a reproduction example? Maybe we could think of a way to automatically warn about this issue.

nihaux commented 6 years ago

hello @oliviertassinari so I put up a minimal demo to reproduce.

https://github.com/nihaux/demo-double-bundle-withstyles
Is a material-ui component packaged as a library

import * as React from 'react';
import List from '@material-ui/core/List';
import ListItem from '@material-ui/core/ListItem';
import ListItemText from '@material-ui/core/ListItemText';

const DummyComponent = ({ items }: { items: string[] }) => (
  <List>
    {items.map(item => (
      <ListItem>
        <ListItemText primary={item} />
      </ListItem>
    ))}
  </List>
);

export default DummyComponent;

I put the faulty conf in the rollup config external: ['react', 'react-dom', '@material-ui/core'],

I published it on npm. I also commited the dist for the demo purpose.

Then this repo use it: https://github.com/nihaux/dummy-demo
It's a create-react-app with material-ui + the DummyComponent

I used the AppBar demo from the doc to show the css problem.

class ButtonAppBar extends React.Component {

  state = {
    showDummy: false,
  };

  componentDidMount() {
    setTimeout(() => this.setState({ showDummy: true }), 5000);
  }

  render() {
    const {classes} = this.props;
    return (
      <div className={classes.root}>
        <AppBar position="static">
          <Toolbar>
            <Typography variant="title" color="inherit" className={classes.flex}>
              Title
            </Typography>
            <Button color="inherit">Login</Button>
          </Toolbar>
        </AppBar>
        { this.state.showDummy && <DummyComponent items={['toto', 'titi', 'tata']}/>}
      </div>
    );
  }
}

Everything works great when you're in dev mode but when you build the project and serve it (using https://github.com/zeit/serve) you'll see all the css bugging as soon as the faulty comp is loaded.

nihaux commented 6 years ago

pushed a non uglified/minified etc version of the build to make it easier to read
https://github.com/nihaux/dummy-demo/blob/master/build/static/js/main.8086f9c5.js

mlindwall commented 6 years ago

Im having the same issue in production using v1.1.0 and webpack.

sli commented 6 years ago

Having a very similar issue in production with 1.0 and webpack. A view with a MUI-dependent component is failing to render properly under certain cases, and the culprit is (after much digging) that somehow the styles for the SvgIcon component are being applied to everything. If I start unchecking rules in the devtools, the page renders (mostly) properly.

In the inspector, I can see that everything I expect is indeed rendering, but all that I see on the page is a 24x24 Paper component. I found a class that was showing up in prod builds that did not have an equivalent named class in dev builds (in this case, jss1). Unchecking the width and height rules in that class fix the issue. Here is the relevant style included in the SvgIcon component:

  root: {
    userSelect: 'none',
    fontSize: 24,
    width: '1em',
    height: '1em',
    display: 'inline-block',
    fill: 'currentColor',
    flexShrink: 0,
    transition: theme.transitions.create('fill', {
      duration: theme.transitions.duration.shorter
    })
  }

These are indeed exactly the rules included in the jss1 class that's being added to (or simply effecting) prod builds and only prod builds. I have yet to find any glaring issues in my code, as nothing untoward happens in dev builds and it's affecting a component that has not been changed recently.

oliviertassinari commented 6 years ago

@nihaux The reproduction example is perfect. Thanks. Let's see how we can improve the warnings.

oliviertassinari commented 6 years ago

Also, I'm gonna add a not in the frequently asked question. People have been facing this issue for months: https://github.com/mui-org/material-ui/issues/8223#issuecomment-331625137

oliviertassinari commented 6 years ago

You should have a warning in the console next time.

marco-silva0000 commented 6 years ago

I have this exact issue, but I use webpack.

All my libs must not package with the material-ui libs, but the main app, which imports the libs, should have it bundled right?

Klabauterman commented 6 years ago

I have the same Issue, does some1 have a solution that does not involve uncommenting code from material-ui?

marton-levay commented 6 years ago

Ohai

If anybody is still looking for a solution with webpack (for us it was an ejected create-react-app with material-ui elements, shared as a library, and then used in a similar cra app with material elements): You have to externalize your whole material-ui dependency using regex in webpack config, otherwise various material scripts - withStyles, withTheme, etc... - will load twice (or more if you have more libs) which will cause the screen to fall apart.

https://webpack.js.org/guides/author-libraries/#external-limitations So in webpack.config.prod.js add something similar:

externals: [
    'react',
    'react-dom',
    //this will externalize everything which start with '@material-ui'
    /^@material-ui\/.+$/,
    'prop-types',
    'react-router-dom'
  ],

Of course this means you want to indicate in your /build/package json that these are peerDependecies. You have to have them in the app where you wish to use this lib.

ghost commented 4 years ago

So I spend the all afternoon reading the related issues and debugging my code so I am posting this here in case it helps someone. I can make a PR for the doc if you want but don't know where it should go as it is not directly a MUI issue.

Use case:

  • I have an external component "mui-tree-list" which use mui v1.
  • The lib use rollup to package the component.
  • in the rollup config I put external: [ 'react', '@material-ui/core', '@material-ui/icons', 'react-dom' ], WRONG ! ;p

If you do this, you'll end up having jss classname conflicts in the production build of you final app.

why ?

cause @material-ui/core does not avoid @material-ui/core/List for instance to be included in your dist package.

and @material-ui/core/List will include withStyles which will cause jss class naming conflict in your final package !

So the solution is, you should put this in your rollup config: external: id => /react|react-dom|material-ui\/.*/.test(id),

this way all material-ui components will be excluded from your build... lost some hair with this one :p .

Where exactly in the rollup.config.js should we put that ? please show a sample.

nckswt commented 4 years ago

If anybody is still looking for a solution with webpack (for us it was an ejected create-react-app with material-ui elements, shared as a library, and then used in a similar cra app with material elements): You have to externalize your whole material-ui dependency using regex in webpack config, otherwise various material scripts - withStyles, withTheme, etc... - will load twice (or more if you have more libs) which will cause the screen to fall apart.

https://webpack.js.org/guides/author-libraries/#external-limitations So in webpack.config.prod.js add something similar:

externals: [
    'react',
    'react-dom',
    //this will externalize everything which start with '@material-ui'
    /^@material-ui\/.+$/,
    'prop-types',
    'react-router-dom'
  ],

Of course this means you want to indicate in your /build/package json that these are peerDependecies. You have to have them in the app where you wish to use this lib.

This fix worked for me -- I'm surprised I haven't seen more people commenting! Thanks!