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.

Sign up for GitHub

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

Jump to bottom

[system] Use cx instead of clsx #32067

Closed
wants to merge 3 commits into from
Closed

[system] Use cx instead of clsx #32067

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9eab152
Use cx instead of clsx
garronej Mar 31, 2022
ecd1b9f
Merge https://github.com/mui-org/material-ui into emotion_compat
garronej May 8, 2022
bd80530
Implement suggestions of @mnajdova
garronej May 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions packages/mui-material/src/Button/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { internal_resolveProps as resolveProps } from '@mui/utils';
import { unstable_composeClasses as composeClasses } from '@mui/base';
import { alpha } from '@mui/system';
import { useCx } from '@mui/styled-engine';
import styled, { rootShouldForwardProp } from '../styles/styled';
import useThemeProps from '../styles/useThemeProps';
import ButtonBase from '../ButtonBase';
Expand Down Expand Up @@ -322,7 +322,7 @@ const Button = React.forwardRef(function Button(inProps, ref) {
variant,
};

const classes = useUtilityClasses(ownerState);
const { root: classes_root, ...classes } = useUtilityClasses(ownerState);

const startIcon = startIconProp && (
<ButtonStartIcon className={classes.startIcon} ownerState={ownerState}>
Expand All @@ -336,14 +336,16 @@ const Button = React.forwardRef(function Button(inProps, ref) {
</ButtonEndIcon>
);

const { cx } = useCx();

return (
<ButtonRoot
ownerState={ownerState}
className={clsx(className, contextProps.className)}
className={cx(contextProps.className, classes_root, className)}
component={component}
disabled={disabled}
focusRipple={!disableFocusRipple}
focusVisibleClassName={clsx(classes.focusVisible, focusVisibleClassName)}
focusVisibleClassName={cx(classes.focusVisible, focusVisibleClassName)}
ref={ref}
type={type}
{...other}
Expand Down
6 changes: 4 additions & 2 deletions packages/mui-material/src/ButtonBase/ButtonBase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { elementTypeAcceptingRef, refType } from '@mui/utils';
import composeClasses from '@mui/base/composeClasses';
import { useCx } from '@mui/styled-engine';
import styled from '../styles/styled';
import useThemeProps from '../styles/useThemeProps';
import useForkRef from '../utils/useForkRef';
Expand Down Expand Up @@ -338,10 +338,12 @@ const ButtonBase = React.forwardRef(function ButtonBase(inProps, ref) {

const classes = useUtilityClasses(ownerState);

const { cx } = useCx();

return (
<ButtonBaseRoot
as={ComponentProp}
className={clsx(classes.root, className)}
className={cx(classes.root, className)}
ownerState={ownerState}
onBlur={handleBlur}
onClick={onClick}
Expand Down
8 changes: 8 additions & 0 deletions packages/mui-styled-engine-sc/src/cx.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type { CxArg } from './tools/classnames';

export type { CxArg };
export declare type Cx = (...classNames: CxArg[]) => string;

export declare function useCx(): {
cx: Cx;
};
8 changes: 8 additions & 0 deletions packages/mui-styled-engine-sc/src/cx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/* eslint-disable import/prefer-default-export */
import { classnames } from './tools/classnames';

const cx = (...args) => classnames(args);

export function useCx() {
return { cx };
garronej marked this conversation as resolved.
Show resolved Hide resolved
}
1 change: 1 addition & 0 deletions packages/mui-styled-engine-sc/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export { default as StyledEngineProvider } from './StyledEngineProvider';

export { default as GlobalStyles } from './GlobalStyles';
export * from './GlobalStyles';
export * from './cx';

// These are the same as the ones in @mui/styled-engine
// CSS.PropertiesFallback are necessary so that we support spreading of the mixins. For example:
Expand Down
1 change: 1 addition & 0 deletions packages/mui-styled-engine-sc/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,4 @@ export default function styled(tag, options) {
export { ThemeContext, keyframes, css } from 'styled-components';
export { default as StyledEngineProvider } from './StyledEngineProvider';
export { default as GlobalStyles } from './GlobalStyles';
export * from './cx';
13 changes: 13 additions & 0 deletions packages/mui-styled-engine-sc/src/tools/classnames.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export declare type CxArg =
garronej marked this conversation as resolved.
Show resolved Hide resolved
| undefined
| null
| string
| boolean
| {
[className: string]: boolean | null | undefined;
}
| readonly CxArg[];
/** Copy pasted from
* https://github.com/emotion-js/emotion/blob/23f43ab9f24d44219b0b007a00f4ac681fe8712e/packages/react/src/class-names.js#L17-L63
* */
export declare const classnames: (args: CxArg[]) => string;
58 changes: 58 additions & 0 deletions packages/mui-styled-engine-sc/src/tools/classnames.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/* eslint-disable no-continue */
/* eslint-disable no-plusplus */
/* eslint-disable import/prefer-default-export */
/* eslint-disable @typescript-eslint/no-unused-expressions */
/* eslint-disable no-restricted-syntax */

/** Copy pasted from
* https://github.com/emotion-js/emotion/blob/23f43ab9f24d44219b0b007a00f4ac681fe8712e/packages/react/src/class-names.js#L17-L63
* */
export const classnames = (args) => {
const len = args.length;
let i = 0;
let cls = '';
for (; i < len; i++) {
const arg = args[i];
if (arg == null) {
continue;
}

let toAdd;
switch (typeof arg) {
case 'boolean':
break;
case 'object': {
if (Array.isArray(arg)) {
toAdd = classnames(arg);
} else {
if (
process.env.NODE_ENV !== 'production' &&
arg.styles !== undefined &&
arg.name !== undefined
) {
console.error(
'You have passed styles created with `css` from `@emotion/react` package to the `cx`.\n' +
garronej marked this conversation as resolved.
Show resolved Hide resolved
'`cx` is meant to compose class names (strings) so you should convert those styles to a class name by passing them to the `css` received from <ClassNames/> component.',
);
}
toAdd = '';
for (const k in arg) {
if (arg[k] && k) {
toAdd && (toAdd += ' ');
toAdd += k;
}
}
}
break;
}
default: {
toAdd = arg;
}
}
if (toAdd) {
cls && (cls += ' ');
cls += toAdd;
}
}
return cls;
};
8 changes: 8 additions & 0 deletions packages/mui-styled-engine/src/cx.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import type { CxArg } from './tools/classnames';

export type { CxArg };
export declare type Cx = (...classNames: CxArg[]) => string;

export declare function useCx(): {
cx: Cx;
};
149 changes: 149 additions & 0 deletions packages/mui-styled-engine/src/cx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
/* eslint-disable import/prefer-default-export */
/* eslint-disable @typescript-eslint/naming-convention */
/* eslint-disable import/first */
/* eslint-disable @typescript-eslint/no-use-before-define */
/* eslint-disable no-labels */
/* eslint-disable no-restricted-syntax */
/* eslint-disable @typescript-eslint/no-shadow */
import { useMemo } from 'react';
import { serializeStyles } from '@emotion/serialize';
import { insertStyles, getRegisteredStyles } from '@emotion/utils';
import { __unsafe_useEmotionCache as useEmotionCache } from '@emotion/react';
import createCache from '@emotion/cache';
import { classnames } from './tools/classnames';

const { createCx } = (() => {
function merge(registered, css, className) {
const registeredStyles = [];

const rawClassName = getRegisteredStyles(registered, registeredStyles, className);

if (registeredStyles.length < 2) {
return className;
}

return rawClassName + css(registeredStyles);
}

function createCx(params) {
const { cache } = params;

const css = (...args) => {
const serialized = serializeStyles(args, cache.registered);
insertStyles(cache, serialized, false);
const className = `${cache.key}-${serialized.name}`;

scope: {
const arg = args[0];

if (!matchCSSObject(arg)) {
break scope;
}

increaseSpecificityToTakePrecedenceOverMediaQueries.saveClassNameCSSObjectMapping(
cache,
className,
arg,
);
}

return className;
};

const cx = (...args) => {
const className = classnames(args);

const feat27FixedClassnames =
increaseSpecificityToTakePrecedenceOverMediaQueries.fixClassName(cache, className, css);

return merge(cache.registered, css, feat27FixedClassnames);
};

return { cx };
}

return { createCx };
})();

/** Will pickup the contextual cache if any */
export function useCx() {
const cache = useEmotionCache();

const { cx } = useMemo(
() => createCx({ cache: cache ?? createCache({ key: 'never' }) }),
[cache],
);

return { cx };
}

// https://github.com/garronej/tss-react/issues/27
const increaseSpecificityToTakePrecedenceOverMediaQueries = (() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@garronej garronej Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@garronej garronej May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screenshot 2022-05-09 at 19 30 16

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thx, make sense!

const cssObjectMapByCache = new WeakMap();

return {
saveClassNameCSSObjectMapping: (cache, className, cssObject) => {
let cssObjectMap = cssObjectMapByCache.get(cache);

if (cssObjectMap === undefined) {
cssObjectMap = new Map();
cssObjectMapByCache.set(cache, cssObjectMap);
}

cssObjectMap.set(className, cssObject);
},
fixClassName: (() => {
function fix(classNameCSSObjects) {
let isThereAnyMediaQueriesInPreviousClasses = false;

return classNameCSSObjects.map(([className, cssObject]) => {
if (cssObject === undefined) {
return className;
}

let out;

if (!isThereAnyMediaQueriesInPreviousClasses) {
out = className;

for (const key in cssObject) {
if (key.startsWith('@media')) {
isThereAnyMediaQueriesInPreviousClasses = true;
break;
}
}
} else {
out = {
'&&': cssObject,
};
}

return out;
});
}

return (cache, className, css) => {
const cssObjectMap = cssObjectMapByCache.get(cache);

return classnames(
fix(
className.split(' ').map((className) => [className, cssObjectMap?.get(className)]),
).map((classNameOrCSSObject) =>
typeof classNameOrCSSObject === 'string'
? classNameOrCSSObject
: css(classNameOrCSSObject),
),
);
};
})(),
};
})();

function matchCSSObject(arg) {
return (
arg instanceof Object &&
!('styles' in arg) &&
!('length' in arg) &&
!('__emotion_styles' in arg)
);
}
1 change: 1 addition & 0 deletions packages/mui-styled-engine/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export { default as StyledEngineProvider } from './StyledEngineProvider';

export { default as GlobalStyles } from './GlobalStyles';
export * from './GlobalStyles';
export * from './cx';

export interface SerializedStyles {
name: string;
Expand Down
1 change: 1 addition & 0 deletions packages/mui-styled-engine/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,4 @@ export default function styled(tag, options) {
export { ThemeContext, keyframes, css } from '@emotion/react';
export { default as StyledEngineProvider } from './StyledEngineProvider';
export { default as GlobalStyles } from './GlobalStyles';
export * from './cx';
13 changes: 13 additions & 0 deletions packages/mui-styled-engine/src/tools/classnames.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export declare type CxArg =
| undefined
| null
| string
| boolean
| {
[className: string]: boolean | null | undefined;
}
| readonly CxArg[];
/** Copy pasted from
* https://github.com/emotion-js/emotion/blob/23f43ab9f24d44219b0b007a00f4ac681fe8712e/packages/react/src/class-names.js#L17-L63
* */
export declare const classnames: (args: CxArg[]) => string;
Loading