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

[v5] CSS injection order is wrong #24109

Closed
goffioul opened this issue Dec 24, 2020 · 59 comments
Closed

[v5] CSS injection order is wrong #24109

goffioul opened this issue Dec 24, 2020 · 59 comments
Labels
package: styled-engine Specific to @mui/styled-engine v5.x migration waiting for 👍 Waiting for upvotes

Comments

@goffioul
Copy link
Contributor

goffioul commented Dec 24, 2020

I'm using the following pattern in my code, but it doesn't work anymore in v5 (tested with 5.0.0-alpha.20). My custom styles get overwritten by other styles (likely coming from the new styling mechanism):

const styles = theme => ({
    title: {
        [theme.breakpoints.down('sm')]: theme.typography.body1
    }
})

function App({ classes }) {
    return <Typography variant="h5" classes={{h5:classes.title}}>Hello</Typography>;
}

const StyledApp = withStyles(styles)(App);
@goffioul goffioul added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 24, 2020
@mnajdova
Copy link
Member

mnajdova commented Dec 24, 2020

@goffioul now that the Typography, Button, etc components are migrated to emotion, you need to use the StylesProvider exported from '@material-ui/core' with the injectFirst option, in order for the CSS injection order to be correct between emotion and JSS. It is explained here: https://next.material-ui.com/guides/interoperability/#css-injection-order.

This is only required for the time of the migration (v5-alpha phase). Once we do no longer depend on JSS, the order should be correct.

Here is a codesandbox with a working example - https://codesandbox.io/s/devtools-material-demo-forked-ethyg?file=/index.js You will notice that the <Demo /> is wrapped with the StylesProvider component.

import * as React from "react";
import ReactDOM from "react-dom";
import StylesEngineProvider from "@material-ui/core/StylesEngineProvider";
import Demo from "./demo";

ReactDOM.render(
  <StyledEngineProvider injectFirst>
    <Demo />
  </StyledEngineProvider>,
  document.querySelector("#root")
);

@mnajdova mnajdova added component: Typography The React component. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 24, 2020
@oliviertassinari oliviertassinari added the support: question Community support but can be turned into an improvement label Dec 24, 2020
@goffioul
Copy link
Contributor Author

I was aware of injectFirst attribute, but it didn't work in my app. After some testing with the codesandbox, it appears the problem come from importing StylesProvider from @material-ui/core/styles (v4 code, which I'm currently migrating) instead of @material-ui/core. You can verify that in your codesandbox.

@goffioul
Copy link
Contributor Author

goffioul commented Dec 24, 2020

It's actually a bit more complicated than that, and I actually need to use the 2 styles providers, like the following:

import { render } from 'react-dom';
import { StylesProvider, jssPreset } from '@material-ui/core/styles';
import StyledEngineProvider from '@material-ui/core/StyledEngineProvider';
import { create } from 'jss';
import rtl from 'jss-rtl';

const jss = create({ plugins: [...jssPreset().plugins, rtl()] });

render(
    <StyledEngineProvider injectFirst>
        <StylesProvider jss={jss}>
            <App />
        </StylesProvider>
    </StyledEngineProvider>,
    document.getElementById('app')
);

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 24, 2020

it appears the problem come from importing StylesProvider from

@goffioul Thanks for raising it. We are still looking for a proper solution to this problem. In v5, we have changed the default import.

What if the best answer was?

import { StylesProvider, jssPreset } from '@material-ui/styles';
import StyledEngineProvider from '@material-ui/core/StyledEngineProvider';

Notice how:

  • It imports from a different package, the one we will try to progressively remove @material-ui/styles.
  • We import the new style provider from a nested folder, solving Module not found @material-ui/core/ThemeProvider when minimizing bundle size in create react app #23208
  • The module has a different name. For the styled and ThemeProvider it's not really an issue to import one or the other, but I think Marija was right, I shouldn't have encouraged using the same name. Using StylesProvider vs. StyledEngineProvider is an important distinction.

@oliviertassinari oliviertassinari added package: styled-engine Specific to @mui/styled-engine and removed component: Typography The React component. support: question Community support but can be turned into an improvement labels Dec 24, 2020
@goffioul
Copy link
Contributor Author

As a developer using material-ui (not developing it), initially I totally missed the different import. Using the same name obviously adds to the confusion, because old code is already importing StylesProvider and you don't realize you have to import it from another module to get a different implementation. It's also not obvious that when migrating code that uses JSS, you actually need to use both providers to manage the import order properly (or is there a better alternative?). Yes, the doc says you need to use injectFirst in your StylesProvider, but it doesn't tell you explicitly which one (in my case, I used injectFirst on the StylesProvider I already imported in my code, but it was the old one, and that didn't have the wanted effect).

If the target is to have a drop-in replacement for the v4 styling engine, I'd say using the same name is a good option. The problem I'm having is just a transient situation in alpha-grade code, I can live with that. Maybe a warning when using the old StylesProvider would help detect such scenario. If a drop-in replacement is not on the table, then I'd go with a different name.

@mainfraame
Copy link

The 5.0.0 alpha seems to awash with duplicate imports. I was using StyleProvider via @material-ui/styled-engine. Is that the correct one?

Ideally, the import of the StyleProvider should be consistent; possibly as a wrapper around both JSS and emotion StyleProviders. That way, legacy components still using JSS are still supported, while the newer implementations using Emotion will work seamlessly. Once JSS is fully removed, it would cause any breaking changes.

@oliviertassinari
Copy link
Member

@mainfraame See my proposal in #24109 (comment). We need two different style providers as they are two different components with different APIs, they might even be required at the same time when both JSS and emotion/sc are used at the same time in the page.

@mainfraame
Copy link

mainfraame commented Jan 2, 2021

@oliviertassinari thanks for your reply. I do understand that for the time being both providers will need to be supported. I agree with your point about distinct naming and think that would be the best option for reducing complexity for devs. It seems to me like just using StyledEngineProvider via core should solve my problem.

Can anyone comment on which styled should be used? I see the legacy styled is using jss still and experimental styled is using emotion. Would you recommend just using experimental styled for all override styling, or would u suggest to continue to use the jss implementation?

@oliviertassinari
Copy link
Member

@mainfraame Prefer using modules coming from @material-ui/styled-engine (emotion/sc) and avoid modules from @material-ui/styles (JSS)

@Philipp91
Copy link
Contributor

The solution in #24109 (comment) worked for me. For others who might be lacking the <StylesProvider/>, I'll just note that one very salient phenomenon is that all components that were previously round (like Fab, DatePicker days, etc.) are rectangular (because the border-radius: 0 takes precedence). Just writing this down so that others can find this issue faster through GitHub search.

@mainfraame
Copy link

@oliviertassinari any plans to add more to the docs regarding the styled/experimentalStyled hooks?

@tomas-c
Copy link

tomas-c commented Jan 4, 2021

Because emotion styles now have to be injected before JSS styles, emotion's css prop can no longer be used to override styles of components that are still using JSS internally unless !important is used. It seems that the only solution is to migrate from emotion to JSS for those components, and only once MUI fully converts to emotion - migrate back. Is there a better way?

@mbrevda
Copy link

mbrevda commented Sep 30, 2021

I'd be happy to share a recording provided it can be privatly

@mbrevda
Copy link

mbrevda commented Oct 3, 2021

@mnajdova can I share privately? Where should I send it? (note: as above, this won't be a minimal reproduction unfortunately)

@Andarist
Copy link
Contributor

Andarist commented Oct 3, 2021

As an exception - I can help you if you send me a replay of the problem privately and describe the issue you are facing in detail (both what happens and what is your expected result)

@mbrevda
Copy link

mbrevda commented Oct 5, 2021

Found the issue was on my end. Apparently, the codemod missed some of the imports (and there was a "rouge" peerDep for v4 (courtesy of mdi-material-ui), allowing the import to almost work). Updating these to v5 imports resolved the issue I was having. Thanks so much @Andarist for pointing this out!

@Andarist
Copy link
Contributor

Andarist commented Oct 5, 2021

Wouldn't be possible without a tip from @mnajdova - if you see styles coming from a "static" class name (like MuiButtonBase-root) and if those styles are not under your control, but generated by JSS, then it's most likely that it's some rogue v4 in your codebase.

@wallzero
Copy link

wallzero commented Oct 5, 2021

Correct me if I'm wrong, but with v4 I had to setup an insertion point so the meta, base, and title elements didn't become buried. I can't find why this was an issue; maybe scrapers only reading the first N elements within <head>? I did something like:

const jss = create({
	...jssPreset(),
	insertionPoint: 'jss'
});

And my SPA index.html:

<!DOCTYPE html>
<html lang="en">
	<head>
		<meta charset="UTF-8">
		<base href="<%= htmlWebpackPlugin.options.baseHref %>" target="_blank">
		<meta name="viewport" content="initial-scale=1, maximum-scale=5">
		<meta http-equiv="X-UA-compatible" content="IE=Edge">
		<title><%= htmlWebpackPlugin.options.title %></title>
		<!-- jss -->
	</head>
	<body>
		<div id="app"></div>
	</body>
</html>

So now I am migrating a toy project and I have to use <StyledEngineProvider injectFirst> in order for the JSS and CSS Module custom rules to have priority. The JSS/makeStyles I'll migrate eventually but plan to continue using CSS Modules in certain cases and <StyledEngineProvider injectFirst> will remain. If this is the case it'd be nice to provide something similar to <!-- emotion --> in order to inject styles below other <head> elements.

Chrome is showing 713 DOM nodes inside <head> after upgrading to v5 and most of that is Emotion styles. With injectFirst the remaining elements are pushed to the bottom. Is it not a problem that meta, base, and title are hundreds of lines down in <head>?

@leland-kwong
Copy link

if you're using @mui/x-data-grid make sure to use the peer dependency of @material-ui/core version ^5.0.0-beta.0. For me, it was the last remaining legacy 4.x module that was inject jss.

@crshumate
Copy link

crshumate commented Oct 20, 2021

Why is use of <StylesProvider injectFirst> not implemented in the NextJS MUI example docs?

Starting a new app and wanted do things the "right" way instead of using deprecated JSS. However while using v5 with emotion we had to come here to learn about <StylesProvider> to avoid littering our codebase with !important

Once we added in this secret super power Component in _app.js our styles were finally applying correctly.

UPDATE: Didn't realize it had changed from @material-ui to @mui. Double checked my deps in package.json and saw entries for @material-ui and @mui. Reinstalled with just @mui and updated import paths properly fixed this issue.

@paullaros
Copy link
Contributor

Tried my best to isolate the CSS injection order issue. You can re-create the issue by clicking the "Go to the about page" link, and hit the refresh icon.

Link: https://codesandbox.io/s/flamboyant-hooks-hdq1b?file=/pages/about.tsx
Based on: https://github.com/mui-org/material-ui/tree/master/examples/nextjs-with-styled-components-typescript

/pages/about.tsx

const CustomButton = styled(Button)`
  background: pink;
  color: black;
`;

/pages/_app.tsx

export default function MyApp(props: AppProps) {
  const { Component, pageProps } = props;
  return (
    <React.Fragment>
      ...
      <StyledEngineProvider injectFirst>
        <ThemeProvider theme={theme}>
          <CssBaseline />
          <Component {...pageProps} />
        </ThemeProvider>
      </StyledEngineProvider>
    </React.Fragment>
  );
}

Before refresh:
image

After refresh:
image

@Andarist
Copy link
Contributor

@paullaros I'm unsure why this happens - I'm not familiar enough with Styled Components. It seems though that the final component has more than a single SC-based class name and, in fact, the injection order is different in both situations - leading to a different visual appearance.

I've verified that the "pink classname" is being passed here within other to the ButtonRoot. That ButtonRoot looks like a regular Styled Component - so it seems that it somehow doesn't handle this situation correctly, perhaps because Button is not a styled component itself.

Just a note - this situation would not happen with Emotion. We have explicit logic to decode "registered class names" from the received className prop.

@mnajdova
Copy link
Member

mnajdova commented Dec 6, 2021

There hasn't been any activity on the issue for over a month and a fix was provided for the initial issue. I am closing it.

@mnajdova mnajdova closed this as completed Dec 6, 2021
@inf3rnus
Copy link

inf3rnus commented Jan 29, 2022

FYI if anyone else runs into an issue with styling, injection order got messed up by my import of import ToggleIcon from "material-ui-toggle-icon";

So, peer dep issue, it uses MUI v4 (at a maximum).

I do not know the exact reason why this occurs, but my theory w/ very little knowledge of CSS engines is that the loading of its module may perform some side effect as the import chain is traversed and an insertion to the styles in <head> overrides other styles

@onzag
Copy link

onzag commented Feb 9, 2022

doesn't work well with SSR if you are having issues with SSR and the emotion cache giving nothing (which may not be obvious) it's because of the StyledEngineProvider.

On the other hand without StyledEngineProvider then non-SSR mechanism don't work well.

There was nothing wrong with JSS, but emotion and JSS "DO NOT" play nice with each other.

I've even had situations where the render changes unexpectedly as if hitting some race conditions.

For anyone considering migrating to v5 if you are happy with v4 it's not worth it. It's been 2 weeks for me, and I think I have to rewrite everything to drop JSS because emotion and JSS refuse to work together, I've tried every trick in the book, it's not reliable.

@mnajdova
Copy link
Member

mnajdova commented Mar 2, 2022

@onzag do you have a reproduction that you can share? Otherwise we cannot act on a statement without any prove/reproduction.

There was nothing wrong with JSS, but emotion and JSS "DO NOT" play nice with each other.

Yes they do, you just have to configure them properly. We used them on our documentation while migration all components, and we didn't have issues.

it's because of the StyledEngineProvider.

On the contrary, this is required for making sure that the styles coming from JSS comes after the styles generated from emotion.

For anyone considering migrating to v5 if you are happy with v4 it's not worth it. It's been 2 weeks for me, and I think I have to rewrite everything to drop JSS because emotion and JSS refuse to work together, I've tried every trick in the book, it's not reliable.

Again, create a small reproduction and you are likely going to find the issue, or we can take a look into it. It's a bit misleading to say upgrading is not worth if you haven't actually use v5 :) The migration will take some time, and we've tried to create codemod for most of the things we thought we could automate, but we believe it is indeed worth migrating.

@pkaufi
Copy link

pkaufi commented Mar 3, 2022

doesn't work well with SSR if you are having issues with SSR and the emotion cache giving nothing (which may not be obvious) it's because of the StyledEngineProvider.

I can confirm this. If you have SSR and add injectFirst to StyledEngineProvider, the <style data-emotion="css "> tag inside the head stays empty. However, the emotion-styles will be added in front of the corresponding html tags (div, span and so on).
As soon as you remove injectFirst, the style tag inside the head will be filled.
I don't think that the empty style tag is a coincidence, as the styles will be put inside the body afterwards. This is just a wild guess from my side though, but I did not find any issues with that approach. SSR works properly in my case.

@mnajdova
Copy link
Member

mnajdova commented Mar 3, 2022

@pkaufi we have several examples to showcase how SSR should be configured, for example using nextjs, remix etc. You can find more on this here - https://mui.com/getting-started/example-projects/#official-examples

@mnajdova
Copy link
Member

mnajdova commented Mar 3, 2022

Here is how our docs is configured on using both emotion and JSS - https://github.com/mui/material-ui/blob/master/docs/pages/_document.js

@pkaufi
Copy link

pkaufi commented Mar 4, 2022

@mnajdova Thank you for your response. I actually used one of your examples and created a reproduction repository of what I was talking about above: https://github.com/pkaufi/ssr-reproduction

As you can see, injectFirst changed the way it behaves on SSR (broken in my eyes), and I didn't find anything regarding this in the documentation. It would be great if someone could shed some light on why this happens and if this is intended.

@mnajdova
Copy link
Member

mnajdova commented Mar 4, 2022

Ah ok, I see the confusion now. The examples use custom emotion cache, which means that it is enough toa add prepend: true as an option and it would do the same as the StyledEngineProvider with injectFirst would do. We have that as an explanation on the docs:

image

Reference: https://mui.com/guides/interoperability/#main-content

If you want to use both emotion and JSS, you should configure and add the styles in the correct order for SSR, this is how we do it for our documentation: https://github.com/mui/material-ui/blob/master/docs/pages/_document.js

@pkaufi
Copy link

pkaufi commented Mar 4, 2022

Thank you very much for the explanation, this helped a lot. I think I got confused with all the possibilities and namings. It's much more clear now :)

@mnajdova
Copy link
Member

mnajdova commented Mar 4, 2022

Glad that we resolved the confusion @pkaufi :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styled-engine Specific to @mui/styled-engine v5.x migration waiting for 👍 Waiting for upvotes
Projects
None yet
Development

No branches or pull requests