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

Stylesheet class names conflict with production build (issue with JSS minification) #1782

Closed
jpetitcolas opened this issue Apr 25, 2018 · 26 comments

Comments

@jpetitcolas
Copy link
Contributor

We noticed a severe issue when passing our project in production. Our app seems to be empty:

image

Seems, because everything is correctly set up in the DOM:

image

The issue seems related to JSS, which has some troubles generating class names. Each time it see a withStyles, it increments a counter. Yet, it seems that MaterialUI and react-admin uses two distinct counters. Hence, when the classes are minified (removing the component display name), classes are renamed such as:

  • MaterialUI: jss1, jss2, etc.
  • React Admin: jss1, jss2, etc.

This duplication cause the issue mentioned above.

We temporarily fixed it on our project side, using a custom class name generator, using the same counter for every classes.

const escapeRegex = /([[\].#*$><+~=|^:(),"'`\s])/g;
let classCounter = 0;

// Heavily inspired of Material UI:
// @see https://github.com/mui-org/material-ui/blob/9cf73828e523451de456ba3dbf2ab15f87cf8504/src/styles/createGenerateClassName.js
// The issue with the MUI function is that is create a new index for each
// new `withStyles`, so we handle have to write our own counter
export const generateClassName = (rule, styleSheet) => {
    classCounter += 1;

    if (process.env.NODE_ENV === 'production') {
        return `c${classCounter}`;
    }

    if (styleSheet && styleSheet.options.classNamePrefix) {
        let prefix = styleSheet.options.classNamePrefix;
        // Sanitize the string as will be used to prefix the generated class name.
        prefix = prefix.replace(escapeRegex, '-');

        if (prefix.match(/^Mui/)) {
            return `${prefix}-${rule.key}`;
        }

        return `${prefix}-${rule.key}-${classCounter}`;
    }

    return `${rule.key}-${classCounter}`;
};

Then we wrapped our Admin component in:

import JssProvider from 'react-jss/lib/JssProvider';

export default () => (
    <JssProvider generateClassName={generateClassName}>
        <Admin />
    </JssProvider>
);

Should I open a PR with this hack-ish fix? It looks like a critical issue, as it prevents from deploying react-admin to production. Yet, I'm not convinced by this solution.

@Kmaschta
Copy link
Contributor

Kmaschta commented Apr 25, 2018

I can confirm that we had the same issue on our project.

On production, JSS minify class names with a counter: jss1, then jss2, etc.
The problem is that there is two stylesheets: Material-UI stylesheet (Paper, Button, etc) and the custom theme stylesheet provided by React Admin.

Both are minified in an isolated fashion, so the first class of the MUI stylesheet is jss1 and the first class of the custom them is jss1 too, and so on.
This cause a conflict in the class names and can have various and weird effects on prod (like a white page).

The workaround above, that I wrote, is just a trick to keep the counter sync for both stylesheets.
But it would be nice to find and fix the root cause.

@Kmaschta Kmaschta changed the title White screen when passed in production (issue with JSS minification) Stylesheet class names conflicts with production build (issue with JSS minification) Apr 25, 2018
@Kmaschta Kmaschta changed the title Stylesheet class names conflicts with production build (issue with JSS minification) Stylesheet class names conflict with production build (issue with JSS minification) Apr 25, 2018
@in19farkt
Copy link

It happened because material-ui is a dependency for react-admin, and if you use material-ui with another version, that you have two withStyles HOC.

I was helped to remove the material-ui from my package.json and reinstall react-admin.

@fzaninotto
Copy link
Member

So that's a matter of having a too rigid version dependency on material-ui. Unfortunately, until material-ui release a stable version, react-admin can't depend on a ~ version (even less on a ^ version), because they break BC at least once a week.

So I guess this problem can be fixed now by using exactly the same material-ui version as react-admin in your package.json, or waiting for a stable material-ui release (announced for may 17th), after which we can use a more relaxed version constraint.

@andyzaharia
Copy link

I can confirm that I had a similar problem with the stylesheets. After some investigation I had another @material-ui dependency besides the one that react-admin uses and somehow they started to conflict in a really weird way. Removing the "@material-ui" dependency fixed my stylesheet problems. I had to fallback to using the material-ui version that react-admin uses(was not a problem at all).

Thank you guys for all your work!

@Kmaschta
Copy link
Contributor

The issue should now be fixed since fa174b3#diff-47fb44765fe40371ceea73ac4cc821c6R6

@fzaninotto
Copy link
Member

Closing as the issue is normally fixed. Feel free to reopen if you still experience the problem.

@Kmaschta
Copy link
Contributor

I re-open the issue since it seems that it isn't fixed yet. See #1927

  • React-admin version: 2.0.3
  • React version: 16.4.1
"@material-ui/core": "~1.2.2",
"@material-ui/icons": "~1.1.0"

@Kmaschta Kmaschta reopened this Jun 19, 2018
@vascofg
Copy link
Contributor

vascofg commented Jun 21, 2018

Fix by downgrading material-ui in my package.json to "~1.0.0".
Note: I had to run npm dedupe to remove the extra dependency inside ra-ui-materialui.

@fzaninotto
Copy link
Member

This is fixed by #1909, to be released with 2.1.0.

@fzaninotto
Copy link
Member

2.1.0 is released, this should now be fixed.

@AkselsLedins
Copy link
Contributor

@fzaninotto It seems that this issue persist with the 2.1.0 version
hq_ laptop with mdpi screen
hq2_ laptop with mdpi screen

maybe @Kmaschta or @jpetitcolas can confirm

I'll stick with the @jpetitcolas solution for the moment

@fzaninotto
Copy link
Member

Did you remove your yarn.lock or composer.lock file before reinstalling the dependencies?

@chr15stevens
Copy link

Fixed in 2.2 by upgrading my material ui to match react-admin's:
"@material-ui/core": "^1.4.0", "@material-ui/icons": "^1.0.0",

@edouardmenayde
Copy link
Contributor

@chr15stevens You actually saved me hours of headache thank you a lot !

@Kmaschta
Copy link
Contributor

Kmaschta commented Sep 5, 2018

I opened an issue on the MUI repo for that. Feel free to add some precisions if needed: mui/material-ui#12781

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Sep 5, 2018

From what I can see on the repository, Material-UI core is set as a dependency. Any specific reason for not having it as a peer dependency?

"dependencies": {
"@material-ui/core": "^1.4.0",

We temporarily fixed it on our project side, using a custom class name generator, using the same counter for every classes.

@jpetitcolas Using a JssProvider component at the root should be enough to solve the problem. Do you really need a custom class name generator?

@Kmaschta
Copy link
Contributor

Kmaschta commented Sep 5, 2018

@fzaninotto and @djhi have the answers about the dependency choices.
But it would be a major breaking change to put it in peer dependency.

@oliviertassinari The idea behind the custom generateClassName was to have a singleton counter, and was the only solution that worked at the time (several months ago).

@oliviertassinari
Copy link
Contributor

oliviertassinari commented Sep 5, 2018

The idea behind the custom generateClassName was to have a singleton counter, and was the only solution that worked at the time (several months ago).

@Kmaschta Ok thank you for the context. ⚠️ This workaround can work client side but doesn't server side. I would love to have a simplified reproduction example to look into that. It's counter-intuitive.

@irfanDC
Copy link

irfanDC commented Oct 23, 2018

Hi, Can we use this with rest api's developed in laravel ?

@djhi
Copy link
Collaborator

djhi commented Oct 23, 2018

@irfanDC You're commenting on a very specific issue here. What are you referring to exactly ?

@irfanDC
Copy link

irfanDC commented Oct 23, 2018

@irfanDC You're commenting on a very specific issue here. What are you referring to exactly ?

No issue, Just asking can we integrate react-admin with laravel as restfull api with axios - npm ?

@Kmaschta
Copy link
Contributor

Hi @irfanDC , and thanks for your question. Of course, React Admin can work with any sort of API and you can plug any client you need. I can recommend the tutorial for a good start with this framework: https://marmelab.com/react-admin/

That said, as explained in the react-admin contributing guide, the right place to ask a "How To" question, get usage advice, or troubleshoot your own code, is Stack OverFlow.

This makes your question easy to find by the core team, and the developer community. Unlike GitHub, Stack Overflow has great SEO, gamification, voting, and reputation. That's why we chose it, and decided to keep GitHub issues only for bugs and feature requests.

So I'm closing this issue, and inviting you to ask your question at:

http://stackoverflow.com/questions/tagged/react-admin

Once you get a response, please continue to hang out on the react-admin channel in Stack Overflow. That way, you can help newcomers and share your expertise!

@irfanDC
Copy link

irfanDC commented Oct 23, 2018

Yes I understand, you can close the issue that is actually not, Thanks for quick response

dotexe0 pushed a commit to WiserSolutions/aor-codemirror that referenced this issue Nov 28, 2018
…versions of material-ui and creates conflicting withStyle HOC. See marmelab/react-admin#1782
jeevangelista added a commit to jeevangelista/signature-commons-ui that referenced this issue Jul 31, 2019
@straurob
Copy link

straurob commented Oct 25, 2019

I tried your workaround for my application which faces the identical issue. Unfortunately it didn't seem to have any affect.

I then noticed that the latest version of react-jss is not compatible anymore. Instead I used react-jss@^8.6.1 which then worked.

We temporarily fixed it on our project side, using a custom class name generator, using the same counter for every classes.

const escapeRegex = /([[\].#*$><+~=|^:(),"'`\s])/g;
let classCounter = 0;

// Heavily inspired of Material UI:
// @see https://github.com/mui-org/material-ui/blob/9cf73828e523451de456ba3dbf2ab15f87cf8504/src/styles/createGenerateClassName.js
// The issue with the MUI function is that is create a new index for each
// new `withStyles`, so we handle have to write our own counter
export const generateClassName = (rule, styleSheet) => {
    classCounter += 1;

    if (process.env.NODE_ENV === 'production') {
        return `c${classCounter}`;
    }

    if (styleSheet && styleSheet.options.classNamePrefix) {
        let prefix = styleSheet.options.classNamePrefix;
        // Sanitize the string as will be used to prefix the generated class name.
        prefix = prefix.replace(escapeRegex, '-');

        if (prefix.match(/^Mui/)) {
            return `${prefix}-${rule.key}`;
        }

        return `${prefix}-${rule.key}-${classCounter}`;
    }

    return `${rule.key}-${classCounter}`;
};

Then we wrapped our Admin component in:

import JssProvider from 'react-jss/lib/JssProvider';

export default () => (
    <JssProvider generateClassName={generateClassName}>
        <Admin />
    </JssProvider>
);

@Kmaschta
Copy link
Contributor

This issue gets old, and the jss API might have changed since then.
The best workaround to date is to sync the material UI versions with React Admin.

@MaffooBristol
Copy link

Many's mileage will vary, but for me, choosing @material-ui/styles over @mui/styles fixed the issue.

I don't know why material-ui has to make such a mess of versions and naming conventions and stuff...!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests