marmelab / react-admin

A frontend Framework for single-page applications on top of REST/GraphQL APIs, using TypeScript, React and Material Design
http://marmelab.com/react-admin
MIT License
24.83k stars 5.23k forks source link

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

Closed jpetitcolas closed 6 years ago

jpetitcolas commented 6 years ago

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:

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 commented 6 years ago

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.

in19farkt commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

The issue should now be fixed since https://github.com/marmelab/react-admin/commit/fa174b343599948ca5683f7aa3729cc6293dfee4#diff-47fb44765fe40371ceea73ac4cc821c6R6

fzaninotto commented 6 years ago

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

Kmaschta commented 6 years ago

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

"@material-ui/core": "~1.2.2",
"@material-ui/icons": "~1.1.0"
vascofg commented 6 years ago

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 commented 6 years ago

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

fzaninotto commented 6 years ago

2.1.0 is released, this should now be fixed.

AkselsLedins commented 6 years ago

@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 commented 6 years ago

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

chr15stevens commented 6 years ago

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 commented 6 years ago

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

Kmaschta commented 6 years ago

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

oliviertassinari commented 6 years ago

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? https://github.com/marmelab/react-admin/blob/dbaf3372da95b443497d7a6b04f9ffadb5688820/packages/ra-ui-materialui/package.json#L39-L40

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 commented 6 years ago

@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 commented 6 years ago

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 commented 5 years ago

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

djhi commented 5 years ago

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

irfanDC commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

straurob commented 4 years ago

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 commented 4 years ago

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 commented 2 years ago

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...!