Class precedence

[garronej]

Class precedence

[janus-reith]

I tried adding a custom cache just for the makeStyles Wrapper:

const { makeStyles } = createMakeStyles({
  useTheme,
  cache: createCache({ key: "my-prefix-key" })
});

While the global default one just keeps the key "css" and is set up like defined in the MUIv5 nextjs example.

That way, the custom classes there actually have priority over the MUI ones however it still breaks the precedence of a className over the other classes.

Before in MUIv4 with makeStyles:
I could pass a root class for a Button, e.g. containing background: "red", within classes.
Now if I also pass a className to that Button, with the class also being created using makeStyles, which e.g. has background: "blue", that one would have priority over what is defined within classes.

Edit: So I just verified, that only comes up when using a different cache. Using the same cache, I have the original error of classes not taking priority over MUI ones, but the className works as expected and also correctly has priority over the MUI classes.

[janus-reith]

Hey @garronej, thank you for the reply, yes that is basically what I'm doing with a few exceptions:

• I dont create the cache in my makeStyles wrapper
• _document.tsx has a custom getInitialProps in my case, however I think I wrapped things properly in a way createDocument would too
• I dont use a the Cache provider or ThemeProvider per page, but have it in _app.js. Also, I don't use getCache there, but directly import the same shared createEmotionCache function. Just switched to getCache to be sure, but it doesn't make a difference.

I will try to create a minimal example on codesandbox to demonstrate the issue.
I will use the examples you gave above, since I expect the issue to be the same.

[janus-reith]

I just started wondering, since Material UI internally uses @emotion/styled instead of @emotion/react, if that somehow would take priority.

import { makeStyles } from "../shared/makeStyles";
import styled from '@emotion/styled'

const useStyles = makeStyles()(() => ({
    otherText: {
        color: "orange"
    }
}));

const Basic = ({ className }) => {
    const { classes } = useStyles();

    return (
        <div className={`${className} ${classes.otherText}`}>Some text</div>
    )
}

const SomeStyled = styled(Basic)`
  color: hotpink;
`

However, this doesn't seem to be the case, since the text would properly render in orange,.
So since that got me curious, I just tried a static classname:

<Button
    classes={{
        root: "SOMESTATICNAME"
    }}
>
    text
</Button>

And realized that indeed SOMESTATICNAME would be at the start of the classes in the resulting element, not at the end.
So this problem seems to be about Material UI v5 in general, and not specific to this library, since the same issues would occur using static css.

Feel free to close this, or maybe keep it open since this would be the first starting point for users coming from v4 and expecting to be able to continue styling MUI components the same way they did before. I will search for any mention of that change in behavior in the MUI repo.

[janus-reith]

Submitted a new issue about this here: mui/material-ui#27945

Update: I got on the wrong track here, order of css classes was not the issue.
Now If don't get things wrong, it would boil down to this:

The emotion styles I define in some component would need to come AFTER the ones generated from the MUI component rendered INSIDE that component in the created CSS if I'm not mistaken.

Although, simply using a separate emotion cache and making all classes there come after the MUI ones / have priority over them does not seem to be the full solution either, since as described above in #17 (comment) that could still break order, with a custom classname not being prioritized over these classes.

[garronej]

Ok I thee where where a lot of back and forth. Sorry I wasn't able to to go through tss-react issues the past few days. I will have a carfull look at this as soon as I can.

[garronej]

I, however, opened another issue on the material-ui repo following up yours.

[janus-reith]

Hey @garronej, thanks, yes it seems to be the solution that I also found in the meantime and described above.
#17 (comment)

That one issue however still remains unsolved if I'm not mistaken:

Before in MUIv4 with makeStyles:
I could pass a root class for a Button, e.g. containing background: "red", within classes.
Now if I also pass a className to that Button, with the class also being created using makeStyles, which e.g. has background: "blue", that one would have priority over what is defined within classes.

Edit: So I just verified, that only comes up when using a different cache. Using the same cache, I have the original error of classes not taking priority over MUI ones, but the className works as expected and also correctly has priority over the MUI classes.

Since emotion will derive the priority of styles based on where in the three they are defined, that still doesn't work.
Here is a simple example:
My component, lets call it MyCustomButton, is a wrapper around the MUI Button and passed some classes, like a root one that defines that the Button by default is "green".
That custom component MyCustomButton just passes down all other props to the Button.

Now, I have some outer component that actually uses this Button, lets call it MyCustomPage.
MyCustomPage will make use of MyCustomButton, but instead of the default green, wants it to be blue.
So it defines a class with styles for a blue Button, and passes it: <MyCustomButton className={classes.blueButton}.

In v4 that would work fine.
But now, the resulting button will still be green - A class definition with styles for the original green button comes later, since first the Outer styles of MyCustomPage are defined, afterwards the inner ones of MyCustomButton.

While easy to work around this in a new project, this can make things quite hard with an existing codebase and is a dealbreaker for a simple transition from v4 to v5 if that pattern was used before.

One workaround that can help: On the outer class defined in MyCustomPage, add "&&" so it gets more priority:

myClassName: {
 "&&": {
      backgroundColor: "blue"
   }
}

Looks a bit weird, but probably better than using !important on each declaration.
It would end up in the resulting css as myClassName.myClassName

In a bigger project, the workflow for this can be like this:

1. Spot all Components that use MUI components and pass classes
2. Check each of them if they also add className, if yes add "&&".
3. Check each use of these components that wrap MUI Components, within order Components, if they pass props, maybe including className, and also add "&&" there.

This could cover usual usecases, but certainly not all of them. There could be situations where you have composition of multiple levels of nested styles, this could probably be covered using "&&&", "&&&&" and so on, you get the idea.

[garronej]

Sorry if I am missing the point, I haven't read your message in detail. I will do if the following code do not solve your problem:

makeStyles.ts

export const cache= createCache({ key: "my-prefix-key" })

const { makeStyles } = createMakeStyles({
  useTheme,
  cache
});

page/document.tsx

import { createDocument } from "tss-react/nextJs";
import { getCache } from "tss-react/cache";
import { cache } from "../shared/makeStyles";

const { Document } = createDocument({ "caches": [ getCache(),  cache ] });


export default Document;

pages/index.tsx

import { getCache } from "tss-react/cache";

export default function Home() {
    return (
        <>
            <Head>
                //...
            </Head>
            <CacheProvider value={getCache()}>
                <MuiThemeProvider theme={theme}>
                    <CssBaseline />
                    <Root />
                </MuiThemeProvider>
            </CacheProvider>,
        </>
    );
}

[garronej]

Thank you, sorry it didn't do it for you.

You can start from the test app located at src/test/ssr in this repo.

you can run it by doing:

git clone https://github.com/garronej/tss-react
cd tss-react
yarn
yarn build
yarn start_ssr

You can fork, modify the example in order to show me what isn't working as expected.

[janus-reith]

Oh, Im seeing this a bit too late, I already crated a quick example here:
https://stackblitz.com/edit/nextjs-4wchjr (Stackblitz since Codesandbox decides to act crazy again)

[garronej]

Ok thanks for taking the time to create a sandbox,

I see that:

1. It isn't working as expected with multiple caches.
2. It isn't working as expected when we use the mui classes props.

I have to fix that!

For now this is the best workaround I can offer: https://stackblitz.com/edit/nextjs-5bjvvn?file=src%2Fcomponents%2FsomeButton.tsx

[garronej]

If you feel you can do it, PR are welcome.

[janus-reith]

Thanks for the quick response!
Simply switching to className sadly is not option, since in my projects classes is used to customize various classes of a MUI component.

Happy to submit a PR once I find out how to control the order of styles here :)

[garronej]

Ok I think it's solved, let me publish a fix

[garronej]

Hi @janus-reith,
I think everything is fixed now, I didn't change anything in the code but I updated the instruction on how to set things up. See the updated readme.

Do not esitate to reopen if you still have issues but it have been carefully tested

[garronej]

See the diff of the README to know how to update your configuration to make things work as expected. Thank you for reporting.

[garronej]

Hi @janus-reith,
Just a small notice to inform you that I updated the instructions on how to setup tss with mui yet again and I also made some small api changes.
Sorry for the instability of the API. Everything will be freezed once mui release.
See new instructions

[garronej]

Tanks for rising this concern.

this can make things quite hard with an existing codebase and is a dealbreaker for a simple transition from v4 to v5 if that pattern was used before.

I agree, this is a problem. We want to make the migration as smooth as possible.

Maybe if material-ui devs address this issue we could, by moving back to using a unique cache, make it work like it used to but I don't see any other way from where I stand.

What I would recommend doing is using tss's cx function that allow to give priority of one class over an other:

export type Props = {
    className?: string;
    children: ReactNode;
};

export function MyButton(props: Props){

    const { className, children } = props;
   
    const { classes, cx }= useStyles();

    return (
        <MuiButton classes={{
            ...classes,
            "root": cx(classes.root, classeName)
        }}>
            {children}
        <MuiButton>
    );
    
}

I don't know how common is the pattern you describe. Maybe I should add some instruction in the readme about that.
What do you think ?

[garronej]

Proof

[janus-reith]

Maybe if material-ui devs address this issue we could, by moving back to using a unique cache, make it work like it used to but I don't see any other way from where I stand.

Yes, thats the same impression I got.

What I would recommend doing is using tss's cx function that allow to give priority of one class over an other

Interesting, I wasn't aware of that. I wonder if it could be wrapped in some clever way, to avoid needing to do this per component, but so far none comes to mind.

I don't know how common is the pattern you describe. Maybe I should add some instruction in the readme about that.
What do you think ?

That could be helpful - While I'm not sure how many people do it like that usually, passing a className as a prop doesn't seem to uncommon.

[garronej]

Before in MUIv4 with makeStyles:
I could pass a root class for a Button, e.g. containing background: "red", within classes.
Now if I also pass a className to that Button, with the class also being created using makeStyles, which e.g. has background: "blue", that one would have priority over what is defined within classes.

Hi @janus-reith, correcty me if I am wrong but this doesn't seem to be actuate. sandbox

[janus-reith]

Before in MUIv4 with makeStyles:
I could pass a root class for a Button, e.g. containing background: "red", within classes.
Now if I also pass a className to that Button, with the class also being created using makeStyles, which e.g. has background: "blue", that one would have priority over what is defined within classes.

Hi @janus-reith, correcty me if I am wrong but this doesn't seem to be actuate. sandbox

Hey @garronej , I can't really make sense of that sandbox, it only shows one h1 and two h2 each receiving a class built with makeStyles? 🤔 It also doesn't run since there is an import for a non-existent styles.css - Did codesandbox by chance mess something up here?

[garronej]

Sorry I didn't save: https://codesandbox.io/s/makestyles-forked-txfry?file=/src/index.js

[janus-reith]

Sorry I didn't save: https://codesandbox.io/s/makestyles-forked-txfry?file=/src/index.js

Ah, I just read your issue in the MUI repo too and figured you were referring to the same thing.
I gave more details on the issue further in this comment: #17 (comment)
Rereading my statement above about classnames and classes, it might've sounded a bit misleading what my point is there, it was about how these style definitions relate, not about how classes and className relate in general.
In my example, the passed className is in the upper scope, and is passed to a Button in a component down the tree - That one has classes.root with a custom color set, and the outer className is able to overwrite it.

I took your Sandbox and added another Button that demonstrates the issue:
https://codesandbox.io/s/makestyles-forked-run7q

As far as I understand things, you won't be able to recreate the same logic with MUI v5 and tss-react currently - The definiton of the cyan inside the button comes later, so that one is used instead of the pink background from className

[garronej]

The example you gave, behave exactly the same with mui v5 and tss-react proof.

And if I move the definition of WrappedButton to the index and make it use the same useStyles the button behave like the others:
your sandborx modified

[janus-reith]

And if I move the definition of WrappedButton to the index and make it use the same useStyles the button behave like the others:
your sandborx modified

Sure, that's expected

The example you gave, behave exactly the same with mui v5 and tss-react proof.

This is confusing, I'm struggling to puzzle the pieces together, since with my earlier testings, this did not work properly. I think this was due to the two separate Caches being necessary, is that not the case anymore? I might need to follow up more on the changes in the latest release then.

[garronej]

In the latest release of tss-react the contextual cache provided by <CacheProvider /> is no longer picked up. By default tss-react uses import { getTssDefaultEmotionCache  } from "tss-react".
If you want tss to use a specific cache there is a provider only for tss <TssCacheProvider />.
So yes in the latest latest version two caches are still required. It won't be the case if the mui team decides to address this but it doesn't seem to go that way.