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

production build - classnames conflicts #8223

Closed
1 task done
darkowic opened this issue Sep 15, 2017 · 62 comments
Closed
1 task done

production build - classnames conflicts #8223

darkowic opened this issue Sep 15, 2017 · 62 comments
Labels
bug 🐛 Something doesn't work

Comments

@darkowic
Copy link
Contributor

darkowic commented Sep 15, 2017

The css class names definitions are duplicated for some components - in my case it is duplicated (I guess from my debugging) for MuiIconButton and MuiModal - check current behavior

  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

The class names should not be duplicated across components.

Current Behavior

My button styles:
classnames_conflict
The class is duplicated.
Styles definition:
classnames_conflit_2

It is working in development mode:
My buttons styles:
development_class

and found the definitions:
mui-icon-button-dev

and from modal:
mui-modal-dev

Steps to Reproduce (for bugs)

I can try to prepare the environment to reproduce the problem but right now I just wanted to report it here.

Context

I'm trying to release my application with the production environment.

Your Environment

Tech Version
Material-UI 1.0.0-beta.10
React 15.6.1
browser any
webpack ^3.3.0

I need some hints where may be the problem. I'm not using withStyles solution anywhere - I use styled components for styles overriding.

@oliviertassinari
Copy link
Member

I have already seen some issue around this problem. It was always linked to duplicated className generator instances. I can't help more without a reproduction.

@oliviertassinari oliviertassinari added v1 waiting for 👍 Waiting for upvotes labels Sep 15, 2017
@oliviertassinari
Copy link
Member

I'm closing the issue for now as not actionable. Let me know if you have a reproduction example.

@darkowic
Copy link
Contributor Author

@oliviertassinari I was able to reproduce the problem. Here is the webpack bin - https://www.webpackbin.com/bins/-KuO6ia3h-JDpBOJncZ3

In my case, I have 2 application roots which are mounted independently to 2 different div. Both use the same material-ui ThemeProvider wrapped with JssProvider to override insertionPoint from jss.

generate_classnames_counter

Based on createGenerateClassName function, you use counter to have unique class names

export default function createGenerateClassName(): generateClassName {
  let ruleCounter = 0;

  return (rule: Rule, sheet?: StyleSheet): string => {
    ruleCounter += 1;
    warning(
      ruleCounter < 1e10,
      [
        'Material-UI: you might have a memory leak.',
        'The ruleCounter is not supposed to grow that much.',
      ].join(''),
    );

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

    if (sheet && sheet.options.meta) {
      return `${sheet.options.meta}-${rule.key}-${ruleCounter}`;
    }

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

And my screenshot confirms that for some reason the counter is duplicated.

I need help. I don't know what I'm doing wrong right now.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 19, 2017

@darkowic You need to share the jss instance between the different react trees. You should be good with such change.

@darkowic
Copy link
Contributor Author

@oliviertassinari I think I'm doing it using my custom ThemeProvider

  <JssProvider registry={context.sheetsRegistry} jss={context.jss}>
    <MuiThemeProvider
      theme={context.theme}
      sheetManage={context.sheetsManager}
      {...props}
    />
  </JssProvider>

I wrap my every react tree with this.

@darkowic
Copy link
Contributor Author

This issue should be opened.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 21, 2017

Sure, let's sum up what we know so far. This issue arise as soon as you are using multiple react rendering tree. The jss provider is going to create two class name generators, one for each tree. Hence we end up with class name conflicts.
@kof Should we extend the JssProvider from react-jss to accept a class name generator?

@oliviertassinari oliviertassinari added core and removed waiting for 👍 Waiting for upvotes labels Sep 21, 2017
@oliviertassinari oliviertassinari changed the title V1 production build - classnames conflicts production build - classnames conflicts Sep 21, 2017
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Sep 21, 2017
@darkowic
Copy link
Contributor Author

darkowic commented Sep 21, 2017

WORKAROUND for client-side application: You can create your own createGenerateClassName and move ruleCounter outside the wrapper function

import warning from 'warning';

// Returns a function which generates unique class names based on counters.
// When new generator function is created, rule counter is reset.
// We need to reset the rule counter for SSR for each request.
//
// It's an improved version of
// https://github.com/cssinjs/jss/blob/4e6a05dd3f7b6572fdd3ab216861d9e446c20331/src/utils/createGenerateClassName.js
//
// Copied from material-ui due to issue https://github.com/callemall/material-ui/issues/8223

// This counter is moved outside from `createGenerateClassName` to solve the issue
let ruleCounter = 0;

export default function createGenerateClassName() {
  return (rule, sheet) => {
    ruleCounter += 1;
    warning(
      ruleCounter < 1e10,
      [
        'Material-UI: you might have a memory leak.',
        'The ruleCounter is not supposed to grow that much.'
      ].join('')
    );

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

    if (sheet && sheet.options.meta) {
      return `${sheet.options.meta}-${rule.key}-${ruleCounter}`;
    }

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

@kof
Copy link
Contributor

kof commented Sep 21, 2017

Nice one, we had such a use case on the server and I was already planing to for a bold hint in the documentation see #5

But now you actually found a good reason to support 2 parallel running providers.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 21, 2017

We need to research if there are use cases for a strong need of multiple parallel JssProviders. If there are, we need to think of something to support it.

@kof You have just found a use case for a multiple parallel JssProviders on the client side. And I think the proposed solution is simple to implement :).

@kof
Copy link
Contributor

kof commented Sep 21, 2017

move ruleCounter outside the wrapper function

This will mean that on the server, ruleCounter will never get reseted. We can't do that.

@oliviertassinari
Copy link
Member

On the server side, the JssProviders definitely needs to support concurrent async rendering of a react tree (strong need). But the current implementation makes it already supported :)

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 21, 2017

@darkowic Proposed a workaround having no access to the underlying stack. We do. We can do better with this extra flexibility: accepting a class name generator instance.

@kof
Copy link
Contributor

kof commented Sep 21, 2017

Also request to the same endpoint should always respond with same class names.

@kof
Copy link
Contributor

kof commented Sep 21, 2017

@kof Should we extend the JssProvider from react-jss to accept a class name generator?

  1. Yeah one possible way would be to allow JssProvider accept a class name generator like this:
import {createGenerateClassName} from 'react-jss'

const generateClassName = createGenerateClassName()

<JssProvider generateClassName={generateClassName}>
  <App1 />
</JssProvider>

<JssProvider generateClassName={generateClassName}>
  <App2 />
</JssProvider>
  1. Another potential option would be to provide some prefix, like an application name. This could work if we assume we don't have unpredictable amount of applications.
<JssProvider classNamePrefix="app-1">
  <App1 />
</JssProvider>

<JssProvider classNamePrefix="app-2">
  <App2 />
</JssProvider>

Pro 1:

  • User doesn't need to provide the prefix

Pro 2:

  • User can have something meaningful in the DOM class names that identifies the app during development
  • User have more control about the prefix.
  • It is relatively easy to have a non-conflicting prefix. Either you have just a few subtrees and naming is already easy enough or you can add you own number prefix to those names and reset them on each request.

@kof
Copy link
Contributor

kof commented Sep 22, 2017

Actually it makes sense to have both props classNamePrefix and generateClassName. First for debugging, second for automated uniqueness of classnames.

@tlvince
Copy link

tlvince commented Sep 22, 2017

I've ran into this issue via facebook/create-react-app#3173 (and the linked reduced test case).

In my case, a material UI-dependant component (v1.0.0-beta.11) was included into an app that also uses material-ui (same versions). In development, this works fine, but in production the layout is broken due to conflicting class names.

I'm not sure if this qualifies as "multiple react rendering trees" and moving var ruleCounter = 0; before createGenerateClassName() did not workaround the issue, but commenting out the following, did work:

https://github.com/callemall/material-ui/blob/2361339fd6df9bfbb85ed2ee4da9d42ee6fee71c/src/styles/createGenerateClassName.js#L26-L28

@robophil
Copy link

robophil commented Sep 22, 2017

Sorry, I couldn't provide more info at the time I opened #7855.😊
This comment basically covers the issue I faced when running production build.

A workaround for this in the pipeline?

@kof
Copy link
Contributor

kof commented Sep 23, 2017

Created a PR which should fix this in react-jss https://github.com/cssinjs/react-jss/pull/155

@oliviertassinari
Copy link
Member

So let's sum up.

@oliviertassinari
Copy link
Member

I'm experiencing this issue with latest v3.9.0

@w3nda This is too generic, there is a million different reason for this problem to happen.

should I migrate now to mui/styes or wait to v4 release. (large scale app)

We might revert the classname hashing logic. While it greatly improves the performance on the server, it has a significant cost on the client. Maybe we will provide a flag to enable it instead. So no, I think that the best option is to solve the core issue. Did you try this page https://material-ui.com/guides/server-rendering/#troubleshooting?

@w3nda
Copy link

w3nda commented Jan 22, 2019

@oliviertassinari Thanks for the fast response, I really don't know how to debug the problem and the link you mentioned isn't relevant to me because I serve it as a static site..
Tho the comment I referenced helped me, shouldn't it tell the cause of this issue in my case?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 22, 2019

@w3nda Static website has the same issue. You need to generate the HTML, so it will use the server-side rendering API. If the index counter leaks between two pages, the class name won't be correct. Well, I think that a slower class name generator is a good tradeoff compared to the pain it is to debug such a problem (& how frequent it is). So, yes, you can upgrade to @material-ui/styles, it's a simple escape hatch.

@leolozes
Copy link

leolozes commented Feb 5, 2019

I just had a similar problem and it was an old material-ui import that was in one of our libraries. It worked fine in development mode but was broken in production. I'm pretty sure this was working before, I don't know if a warning could be issued in this case, even if it's clearly our fault.
I updated the versions to match them between projects and everything worked again.

@kopax
Copy link

kopax commented Apr 11, 2019

Hi, I am using material just for a single input, button and form on my site and I had to use a <JssProvider /> following this comment #8223 (comment)

It's annoying to need this jss provider, is there another fix we can do that would not increase the final build size?

@oliviertassinari
Copy link
Member

@kopax What are you using JssProvider for?

@kopax
Copy link

kopax commented Apr 11, 2019

Hi @oliviertassinari, In production before visiting another route (we use react-router):

image

In production after visiting a route or in development

image

Something similar here happens with a form (weird box shadows), we need to visit another page to have the proper css, that happens only in production:

image

Both issues are fixed if we add JssProvider. (fixed: we have the same css in production than in development)

@oliviertassinari
Copy link
Member

I have no idea. I can't help with the provided information.

@andrewkslv
Copy link

andrewkslv commented Apr 11, 2019

Everything is broken too. I observe wrong order jssXX classes in production and as result the wrong styling. It happens after refreshing the page.

Can't find the reason yet.

@kopax
Copy link

kopax commented Apr 12, 2019

@oliviertassinari perhaps those version number used by react-admin@2.6.4 we are using would help you tell us what's going on:

        "@material-ui/core": "^1.4.0",
        "@material-ui/icons": "^1.0.0",
        "material-ui-chip-input": "1.0.0-beta.6 - 1.0.0-beta.8",

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 12, 2019

Could you make sure that Material-UI is only bundled once? Using two versions at the same time can create the same buggy behavior.

@andrewkslv
Copy link

Well. It's possible. We're using react-admin which has recommended version ~1.5.

I solved the bug on production by adding JssProvider. Now I can refresh the page normally.

import React from "react";
import { Admin, Resource } from "react-admin";
import JssProvider from "react-jss/lib/JssProvider";

const App = () => (
  <JssProvider>
    <Admin dataProvider={dataProvider}>
      <Resource name="trip" list={RequestsList} className="resourceItem" />
    </Admin>
  </JssProvider>
);

export default App;

@kopax
Copy link

kopax commented Apr 12, 2019

@andrewkslv this is exactly what we are trying to avoid, we'd prefer to use it without JssProvider because we only use one Input and Button component from @material-ui, instead we prefer to use for the rest another ui for react-admin.

@oliviertassinari I've just checked and I did have some dependencies issues. I fixed it but still got the errors with the following npm ls @material-ui/core:

├─┬ @bootstrap-styled/ra-ui@1.0.23
│ └── @material-ui/core@1.5.1 
├── @material-ui/core@1.5.1 
└─┬ ra-ui-materialui@2.6.4
  └── @material-ui/core@1.5.1 

After doing:

rm -rf node_modules/@bootstrap-styled/ra-ui/node_modules/@material-ui/
rm -rf node_modules/ra-ui-materialui/node_modules/@material-ui/
npm ls @material-ui/core
├─┬ @bootstrap-styled/ra-ui@1.0.23
│ └── UNMET DEPENDENCY @material-ui/core@1.5.1 
├── @material-ui/core@1.5.1 
└─┬ ra-ui-materialui@2.6.4
  └── UNMET DEPENDENCY @material-ui/core@1.5.1 

Then now it does work (no css issue in production). I guess this is not what I want...

Related project @material-ui dependencies:

Any idea what to do?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 13, 2019

@kopax It's hard to tell without something I can debug. I'm happy to hear it's working now.

I have noticed you are using styled-components with Material-UI. If you have some time, I would love to chat about the integration on Gitter.

@kopax
Copy link

kopax commented Apr 13, 2019

The working solution isn't natural. Which means it involve task that cannot be run with npm. I will not use it, i gave this as a hint.

We'll have the chance on monday, I'll join the mui gitter channel.

@roytz
Copy link

roytz commented Apr 22, 2019

Hi @kopax, did you manage to find a solution?

@kopax
Copy link

kopax commented Apr 22, 2019

No not yet. Not without the provider. @oliviertassinari I am on gitter.

@griiettner
Copy link

@andrewkslv Your solution really worked for me. I'm also using react-admin and AWS Amplify. Anytime I would deploy my react application to a S3 bucket, the style would be all broken, and your solution really saved me.

@koutsenko
Copy link

Same issue here. Why adding JssProvider helps?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 25, 2019

I have added a warning in v4 to warn when duplicated style instances are used: #15422.

@andrewkslv
Copy link

I don't know. I raised this issue in react-admin when they were implementing a new version of material ui to the framework but it was ignored.

marmelab/react-admin#3102 (comment)

@beran24
Copy link

beran24 commented Oct 8, 2019

Where i can find the solution about production build - classnames conflicts #8223 ?

Thanks,

@KannugoPrithvi
Copy link

KannugoPrithvi commented Nov 20, 2019

@oliviertassinari Facing this issue again, even though i have followed all the instructions. Since it is working for others, i guess i might be missing something basic.

https://stackoverflow.com/questions/58938080/jssprovider-not-generating-class-prefix-with-classnameprefix

I'm using following versions of the packages.

"@material-ui/core": "^4.6.1",
"@material-ui/icons": "^4.5.1",
"react": "^16.12.0",
"react-dom": "^16.12.0",
"react-jss": "^10.0.0",
"react-scripts": "3.2.0"

Update: Issue got resolved. Sorry for not going through documentation thoroughly. This part of the documentation solved my issue.

https://material-ui.com/styles/api/#creategenerateclassname-options-class-name-generator

But somehow, the JSSProvider solution which was working for all others, was not working for me. Anyways, thank you :)

@appli-intramuros
Copy link

Thanks @KannugoPrithvi , this is the good solution ! https://material-ui.com/styles/api/#creategenerateclassname-options-class-name-generator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
Development

No branches or pull requests