[system] Use `cx` instead of `clsx` #32067

garronej · 2022-03-31T01:56:05Z

Following our discussion there is a PR to show you what I have in mind.

You can run the test with:

yarn test:regressions:dev

Then access: http://localhost:3000/regression-Cx/Cx#no-dev

With this branch you'll get:

With mui/material-ui#master:

Summary, for the record, of why I am doing this:

To enable @emotion/react to be used for writing custom-style. See PR#31825.
To make @emotion/css work as expected when user to write custom styles.
Enable TSS to work out of the box with MUI, without user having to explicitly provide a cache and follow specific instructions for SSR.

I am looking forward to reading what you think of it.

Best regard,

mui-bot · 2022-03-31T01:58:56Z

Details of bundle changes

@material-ui/core: parsed: +0.15% , gzip: +0.24%
@material-ui/lab: parsed: +0.20% , gzip: +0.26%
@mui/material-next: parsed: +0.61% , gzip: +0.82%

Generated by 🚫 dangerJS against bd80530

garronej · 2022-04-01T16:27:09Z

I realize that this PR might be a bit hard to grasp. Let me give you some additional technical details.

@emotion/css export cx. It's like clsx but it's able to ensures styles are overwritten in the correct order (among styles generated by emotion with a given cache).
The cx function is exported only from @emotion/css but not from @emotion/react. Yet mui-styled-engine depends on @emotioin/react, we don't want to add an extra dependency so we recreate cx with what's available in @emotion/react.
Il order for cx to work we need the contextual emotion cache or whatever cache is used by default. I got an API in @emotion/react that enables us to pick it up: __unsafe_useEmotionCache.
In the bowser __unsafe_useEmotionCache() never returns null. On the server, if the user wants SSR to be working, he has to explicitly provide an emotion cache. There is no way around it. Thus we can be sure that we always have the relevant cache.
This is just the implementation of the well known classenames function. We could very well use clsx instead. I copy pasted the implementation from @emotion/react because it doesn't export it and I wanted to avoid adding an extra dependency to mui-styled-engine and mui-styled-engine-sc.
mui-styled-engine-sc needs to have the same API than mui-styled-engine, yet styled-component isn't able, like emotion is, to ensure styles are overwritten in the correct order. When mui-styled-engine-sc is used useCx only returns an implementation of the classnames API with no extra smart.
Of course the change mades to Button (use of cx in place of clsx) would have to be applied to every component. I didn't do it yet because this PR should be enough to start the discussion.
There is a block of code that modify the default behaviour of @emotion/css's cx. It is related to this issue users faced upgrading from v4. To make it simple, if we use the default cx behaviour, in this image the background is red because of the higher specificity induced by the media query. I think the expected behaviour is for the background to be green.

Best regards,

mnajdova

The changes more or less make sense (I would kill the fix for the media queries for now, but I already left a comment for it below, so we can continue the thread there).

Regarding the test-case, note that also plan to add a new API (coming from the @mui/base package), where we would have componentsProps.slot and we would need to merge the className prop from there in the correct order too (for the @mui/base components we don't plan to suppor the classes prop, only the componentsProps API, but for the @mui/material components I wouldn't kill the classes API yet, to avoid numeros breaking changes again). Anyway, the fix would apply if the css prop is used there too, so this fact shouldn't change much.

One scary thought is that we would need to update all components, which always comes with the risk of breaking something along the way. I am curious to hear what @siriwatknp's thoughts on the changes too.

packages/mui-styled-engine-sc/src/cx.js

packages/mui-styled-engine-sc/src/tools/classnames.d.ts

packages/mui-styled-engine-sc/src/tools/classnames.js

mnajdova · 2022-04-11T09:56:41Z

packages/mui-styled-engine/src/cx.js

+}
+
+// https://github.com/garronej/tss-react/issues/27
+const increaseSpecificityToTakePrecedenceOverMediaQueries = (() => {


This looks suspicious. I understand that it fixes an issue that was reported, but would it be better if this is reported inside emotion and we get a validation from the authors there that this is a bug? Increasing the specificity behind the scenes sounds scary.

There is no 'bug' strictly speaking in the default cx, it works as the specs says it work; but not as the users expect it to work.
Whether or not the increase of specificity to override media queries should be the default behavior of cx is up to debate.
For TSS I had no choice but to implement the feature. Users were able to override media queries in v4 so I had to implement it or it would have made it very hard for some projects to upgrade to MUIv5.

I personally think that having this increase of specificity is a hard requirement for us.
If I have a button that, by default, is red for large screens and blue otherwise and I decide to make it white by passing a custom class. I expect my button to be white regardless of the screen size.

If I kill this feature MUI and TSS will end up with an avalanche of issues like "BUG: My custom style only applies on small screen size".

Interesting, I've tried replicating this with pure CSS, and looks like it works as you are suggesting (if we assume the order of the classes in the CSS file I have would correspond with the order of the classes added in the cx util): https://codepen.io/mnajdova/pen/VwyBEyX

@Andarist what do you think about this? I remember we once discussed this and you had a push back on it with some reason, but I don't remember what it was. I couldn't find the issue where it was discussed.

Interesting, I've tried replicating this with pure CSS, and looks like it works as you are suggesting (if we assume the order of the classes in the CSS file I have would correspond with the order of the classes added in the cx util): https://codepen.io/mnajdova/pen/VwyBEyX

Yeah, this is how it already works in Emotion because we just "flatten" everything into a single rule~. Media query doesn't increase the specificity anyhow so it "just works". However, at times this might be confusing because sometimes people only want to override the "base" declaration without affecting the media query one and there is no easy way to do this.

This might depend on how exactly you merge styles here, if you nest them etc. I would appreciate a codesandbox or something of the "broken" behavior in MUI - then I could perhaps comment further because right now I don't fully understand it as the basic case should work exactly like that.

Hi @Andarist, thank you for your involvement,
I'll fix you a sandbox ASAP

I mean, I'd really love to be proven wrong but I dont see any other way to make it work but to use cx.

I've successfully rendered this text as pink with just those two patches applied to your repro (this one):

Patch 1

diff --git a/node_modules/@mui/base/composeClasses/composeClasses.js b/node_modules/@mui/base/composeClasses/composeClasses.js index 8d25ea4..2382b9c 100644 --- a/node_modules/@mui/base/composeClasses/composeClasses.js +++ b/node_modules/@mui/base/composeClasses/composeClasses.js @@ -3,17 +3,24 @@ export default function composeClasses(slots, getUtilityClass, classes) { Object.keys(slots).forEach( // `Objet.keys(slots)` can't be wider than `T` because we infer `T` from `slots`. // @ts-expect-error https://github.com/microsoft/TypeScript/pull/12253#issuecomment-263132208 slot => { - output[slot] = slots[slot].reduce((acc, key) => { + const arr = slots[slot].reduce((acc, key) => { + if (key) { + acc.push(getUtilityClass(key)); + } + + return acc; + }, []) + + slots[slot].forEach((key) => { if (key) { if (classes && classes[key]) { - acc.push(classes[key]); + arr.push(classes[key]); } - acc.push(getUtilityClass(key)); } + }) - return acc; - }, []).join(' '); + output[slot] = arr.join(' '); }); return output; }

Patch 2

diff --git a/node_modules/@mui/material/ButtonBase/ButtonBase.js b/node_modules/@mui/material/ButtonBase/ButtonBase.js index f2333f8..407818e 100644 --- a/node_modules/@mui/material/ButtonBase/ButtonBase.js +++ b/node_modules/@mui/material/ButtonBase/ButtonBase.js @@ -321,7 +321,7 @@ const ButtonBase = /*#__PURE__*/React.forwardRef(function ButtonBase(inProps, re const classes = useUtilityClasses(ownerState); return /*#__PURE__*/_jsxs(ButtonBaseRoot, _extends({ as: ComponentProp, - className: clsx(classes.root, className), + className: clsx(className, classes.root), ownerState: ownerState, onBlur: handleBlur, onClick: onClick,

Take a look at the generated class name, now the color: pink; is at the very bottom:

I don't advise actually using those patches, this is just a quick hack - the point is that this can be fixed without using cx. I lack the knowledge about the internal composition of styles and "slots" to know how to actually this should be fixed.

And actually... the first patch isn't even needed here but I feel more comfortable with the logic included in it. OTOH, I don't entirely understand why the composeClasses is inserting that root class name there if it ends up being provided directly~ to the className prop in the ButtonBase anyway.

Thank you for taking the time to prove your point.
I was stuck on the idea that:

clsx(classes.root, className) === `${classes.root} ${className}`

which I assumed to be equivalent to

`${classname} ${classes.root}`

I thought only cx was able to enforce priority of one class over another.

Turns out I was wrong, I am not familiar enough with the inner working of emotion.

It’s good news.

What u are saying is correct but in this case the resulting className is passed to Emotion and it can still handle decoding that

Ok thx, make sense!

siriwatknp · 2022-04-12T08:38:22Z

FYI, this is on my radar. I might take a few days to get more context, will share my thought after that.

garronej · 2022-05-08T21:07:54Z

Hi @mnajdova @siriwatknp,
I just applied the suggestion of @mnajdova.

A lot have been said but the important pointers to understand this PR are in this message.

I still think it's a problem for us that emotion, unlike vanilla CSS, assigns a higher specificity to rules within media queries but regardless, this effort is still undoubtedly worthy to be pursued.
Besides, maybe, while being a problem in theory, it won't be a problem in practice.

Best regards,

UPDATE:
Latest update from @Andarist suggests that there might be a simpler way to achieve the same result.
I'll investigate.

mnajdova · 2022-05-17T12:29:30Z

UPDATE:
Latest update from @Andarist suggests that there might be a simpler way to achieve the same result.
I'll investigate.

@garronej let me know if you have any updates on this.

garronej · 2022-05-23T19:08:05Z

let me know if you have any updates on this.

@mnajdova Sure, I didn't give up in this, I just need to make time...

garronej · 2022-06-18T14:13:47Z

Hi @mnajdova,
There is no need to use cx indeed.
We just need to make sure Emotion receive the classeName in the correct order and that's it!

I've opened a new pr

mnajdova · 2022-06-22T13:01:46Z

This is great news! I am happy we can have a simple fix :)

Use cx instead of clsx

9eab152

danilo-leal changed the title ~~Use cx instead of clsx~~ [system] Use cx instead of clsx Mar 31, 2022

danilo-leal added the package: system Specific to @mui/system label Mar 31, 2022

This was referenced Apr 5, 2022

Fix customization with createTheme gregnb/mui-datatables#1882

Merged

[codemod] Add jss to tss-react codemod #31802

Merged

mnajdova requested changes Apr 11, 2022

View reviewed changes

siriwatknp self-assigned this Apr 12, 2022

This was referenced Apr 14, 2022

Wrong color applied to link when overriding MUI theme garronej/tss-react#77

Closed

@media styles are applied in the wrong order emotion-js/emotion#1860

Closed

garronej mentioned this pull request Apr 28, 2022

Can this be used in a single spa application? garronej/tss-react#69

Closed

garronej added 2 commits May 8, 2022 20:46

Merge https://github.com/mui-org/material-ui into emotion_compat

ecd1b9f

Implement suggestions of @mnajdova

bd80530

garronej mentioned this pull request Jun 18, 2022

[system] Make @emotion/* fully supported in the System #33205

Merged

garronej closed this Jun 18, 2022

garronej mentioned this pull request Aug 9, 2022

classes styles don't override default when using @emotion/react #28045

Closed

2 tasks

garronej mentioned this pull request Sep 27, 2022

Unable to get desired style precedence without TssCacheProvider garronej/tss-react#115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[system] Use `cx` instead of `clsx` #32067

[system] Use `cx` instead of `clsx` #32067

garronej commented Mar 31, 2022 •

edited

Loading

mui-bot commented Mar 31, 2022 •

edited

Loading

garronej commented Apr 1, 2022 •

edited

Loading

mnajdova left a comment

mnajdova Apr 11, 2022

garronej Apr 11, 2022 •

edited

Loading

mnajdova Apr 12, 2022

Andarist Apr 12, 2022

garronej Apr 12, 2022

garronej May 9, 2022 •

edited

Loading

Andarist May 9, 2022

garronej May 9, 2022

Andarist May 9, 2022

garronej May 9, 2022

siriwatknp commented Apr 12, 2022

garronej commented May 8, 2022 •

edited

Loading

mnajdova commented May 17, 2022

garronej commented May 23, 2022

garronej commented Jun 18, 2022

mnajdova commented Jun 22, 2022

[system] Use cx instead of clsx #32067

[system] Use cx instead of clsx #32067

Conversation

garronej commented Mar 31, 2022 • edited Loading

mui-bot commented Mar 31, 2022 • edited Loading

garronej commented Apr 1, 2022 • edited Loading

mnajdova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garronej Apr 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garronej May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siriwatknp commented Apr 12, 2022

garronej commented May 8, 2022 • edited Loading

mnajdova commented May 17, 2022

garronej commented May 23, 2022

garronej commented Jun 18, 2022

mnajdova commented Jun 22, 2022

[system] Use `cx` instead of `clsx` #32067

[system] Use `cx` instead of `clsx` #32067

garronej commented Mar 31, 2022 •

edited

Loading

mui-bot commented Mar 31, 2022 •

edited

Loading

garronej commented Apr 1, 2022 •

edited

Loading

garronej Apr 11, 2022 •

edited

Loading

garronej May 9, 2022 •

edited

Loading

garronej commented May 8, 2022 •

edited

Loading