Skip to content

[styled-engine] Fix style overrides variants type #45478

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

Merged
merged 9 commits into from
Apr 11, 2025
Merged

[styled-engine] Fix style overrides variants type #45478

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9618878
poc
siriwatknp Mar 4, 2025
04ea73d
fix types
siriwatknp Mar 4, 2025
cade9e8
docs ts format
siriwatknp Mar 4, 2025
d7ec526
fix types
siriwatknp Mar 4, 2025
1537b32
fix types
siriwatknp Mar 5, 2025
daa9579
Test alternative fix which doesn't require adding CSSPropertiesWithMu…
DiegoAndai Mar 7, 2025
cbe257a
Revert "Test alternative fix which doesn't require adding CSSProperti…
DiegoAndai Mar 14, 2025
e91681e
Merge branch 'master' into fix/styleoverrides-variants-type
DiegoAndai Mar 14, 2025
48dda59
Merge branch 'master' into fix/styleoverrides-variants-type
siriwatknp Apr 9, 2025
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
2 changes: 1 addition & 1 deletion docs/data/material/getting-started/templates/marketing-page/components/Features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const items = [
const Chip = styled(MuiChip)(({ theme }) => ({
variants: [
{
props: ({ selected }) => selected,
props: ({ selected }) => !!selected,
Copy link
Member Author

Choose a reason for hiding this comment

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

Have to fix this because the type of selected is boolean | undefined.

Should we loosen the type like of props? to be () => boolean | null | undefined?

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind the !! to cast into boolean

Copy link
Member

Choose a reason for hiding this comment

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

Does this mean this is a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

I would argue it's a fix as it was always supposed to be () => boolean (it's only boolean in the CSSObjectWithVariants type), but it wasn't actually because the type wasn't being correctly picked up.

It's one of those fixes that could be perceived as a breaking change.

Maybe we could compromise with a minor release and an explanation on the changelog?

Copy link
Member

Choose a reason for hiding this comment

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

This would be the last thing that we need to decide on. Similar to the styles now being required, this is a fix that might be perceived as breaking.

My proposal is that we release it with in a minor version and a note.

Copy link
Member Author

Choose a reason for hiding this comment

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

My proposal is that we release it with in a minor version and a note.

Let's do that.

style: {
background:
'linear-gradient(to bottom right, hsl(210, 98%, 48%), hsl(210, 98%, 35%))',
Expand Down
2 changes: 1 addition & 1 deletion docs/data/material/getting-started/templates/marketing-page/components/Features.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ interface ChipProps {
const Chip = styled(MuiChip)<ChipProps>(({ theme }) => ({
variants: [
{
props: ({ selected }) => selected,
props: ({ selected }) => !!selected,
style: {
background:
'linear-gradient(to bottom right, hsl(210, 98%, 48%), hsl(210, 98%, 35%))',
Expand Down
8 changes: 5 additions & 3 deletions docs/src/components/typography/GradientText.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { styled } from '@mui/material/styles';
import { PaletteColor, styled } from '@mui/material/styles';

type Color = 'primary' | 'error' | 'success' | 'warning';

const GradientText = styled('span')<{
color?: 'primary' | 'error' | 'success' | 'warning';
color?: Color;
}>(({ theme }) => ({
variants: [
...Object.entries((theme.vars || theme).palette as Record<string, any>)
...(Object.entries((theme.vars || theme).palette) as Array<[Color, PaletteColor]>)
.filter(([color, value]) => color !== 'primary' && value && value[400])
.map(([color, value]) => ({
props: { color },
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/premium-themes/onepirate/modules/components/Paper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const PaperRoot = styled(MuiPaper, {
})(({ theme }) => ({
variants: [
{
props: ({ padding }) => padding,
props: ({ padding }) => !!padding,
style: {
padding: theme.spacing(1),
},
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/premium-themes/onepirate/modules/components/Paper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const PaperRoot = styled(MuiPaper, {
})<ExtraPaperProps>(({ theme }) => ({
variants: [
{
props: ({ padding }) => padding,
props: ({ padding }) => !!padding,
style: {
padding: theme.spacing(1),
},
Expand Down
22 changes: 22 additions & 0 deletions packages/mui-material/src/styles/createTheme.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ const theme = createTheme();
variants: [
{
props: ({ ownerState }) => ownerState.color === 'primary',
style: {},
Copy link
Member

Choose a reason for hiding this comment

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

Wasn't style required before?

Copy link
Member

Choose a reason for hiding this comment

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

It was, but the CSSObjectWithVariants wasn't being picked correctly, so it wasn't enforced. Is that correct @siriwatknp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. that's correct.

},
],
}),
Expand Down Expand Up @@ -263,3 +264,24 @@ const theme = createTheme();
},
});
}

// Invalid variant
{
createTheme({
components: {
MuiButton: {
styleOverrides: {
// @ts-expect-error invalid variant
root: {
variants: [
{
props: { variant: 'not-a-variant' },
style: { border: 0 },
},
],
},
},
},
},
});
}
39 changes: 24 additions & 15 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 @@ -59,19 +59,7 @@ export interface CSSOthersObjectForCSSObject {
}

// Omit variants as a key, because we have a special handling for it
export interface CSSObject
extends CSSPropertiesWithMultiValues,
CSSPseudos,
Omit<CSSOthersObject, 'variants'> {}

interface CSSObjectWithVariants<Props> extends Omit<CSSObject, 'variants'> {
variants: Array<{
props: Props | ((props: Props) => boolean);
style:
| CSSObject
| ((args: Props extends { theme: any } ? { theme: Props['theme'] } : any) => CSSObject);
}>;
}
export interface CSSObject extends CSSPropertiesWithMultiValues, CSSPseudos, CSSOthersObject {}

export interface ComponentSelector {
__emotion_styles: any;
Expand Down Expand Up @@ -106,8 +94,29 @@ export interface FunctionInterpolation<Props> {
export interface ArrayInterpolation<Props> extends ReadonlyArray<Interpolation<Props>> {}

export type Interpolation<Props> =
| InterpolationPrimitive
| CSSObjectWithVariants<Props>
| null
| undefined
| boolean
| number
| string
| ComponentSelector
| Keyframes
| SerializedStyles
| CSSPropertiesWithMultiValues
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a myth to me. If not adding CSSPropertiesWithMultiValues explicitly as a union, the spec fail for recreating nested theme.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean with "myth"?

Copy link
Member Author

@siriwatknp siriwatknp Mar 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

At first, I thought that CSSObject is enough because it extends CSSPropertiesWithMultiValues. However, if I excluded CSSPropertiesWithMultiValues from the union type, I got this error from createTheme.spec:

image

Here is the full error:

Argument of type 'Theme' is not assignable to parameter of type 'Omit<ThemeOptions, "components"> & Pick<CssVarsThemeOptions, "colorSchemes" | "defaultColorScheme" | "components"> & { ...; }'.
  Type 'Theme' is not assignable to type 'Pick<CssVarsThemeOptions, "colorSchemes" | "defaultColorScheme" | "components">'.
    Types of property 'components' are incompatible.
      Type 'Components<BaseTheme> | undefined' is not assignable to type 'Components<Omit<Theme, "palette" | "components"> & CssVarsTheme> | undefined'.
        Type 'Components<BaseTheme>' is not assignable to type 'Components<Omit<Theme, "palette" | "components"> & CssVarsTheme>'.
          Types of property 'MuiAlert' are incompatible.
            Type '{ defaultProps?: Partial<AlertProps> | undefined; styleOverrides?: Partial<OverridesStyleRules<keyof AlertClasses, "MuiAlert", BaseTheme>> | undefined; variants?: { ...; }[] | undefined; } | undefined' is not assignable to type '{ defaultProps?: Partial<AlertProps> | undefined; styleOverrides?: Partial<OverridesStyleRules<keyof AlertClasses, "MuiAlert", Omit<Theme, "palette" | "components"> & CssVarsTheme>> | undefined; variants?: { ...; }[] | undefined; } | undefined'.
              Type '{ defaultProps?: Partial<AlertProps> | undefined; styleOverrides?: Partial<OverridesStyleRules<keyof AlertClasses, "MuiAlert", BaseTheme>> | undefined; variants?: { ...; }[] | undefined; }' is not assignable to type '{ defaultProps?: Partial<AlertProps> | undefined; styleOverrides?: Partial<OverridesStyleRules<keyof AlertClasses, "MuiAlert", Omit<Theme, "palette" | "components"> & CssVarsTheme>> | undefined; variants?: { ...; }[] | undefined; }'.
                Types of property 'styleOverrides' are incompatible.
                  Type 'Partial<OverridesStyleRules<keyof AlertClasses, "MuiAlert", BaseTheme>> | undefined' is not assignable to type 'Partial<OverridesStyleRules<keyof AlertClasses, "MuiAlert", Omit<Theme, "palette" | "components"> & CssVarsTheme>> | undefined'.
                    Type 'Partial<OverridesStyleRules<keyof AlertClasses, "MuiAlert", BaseTheme>>' is not assignable to type 'Partial<OverridesStyleRules<keyof AlertClasses, "MuiAlert", Omit<Theme, "palette" | "components"> & CssVarsTheme>>'.
                      Types of property 'root' are incompatible.
                        Type 'Interpolation<AlertProps & Record<string, unknown> & { ownerState: AlertProps & Record<string, unknown>; } & { theme: BaseTheme; }>' is not assignable to type 'Interpolation<AlertProps & Record<string, unknown> & { ownerState: AlertProps & Record<string, unknown>; } & { theme: Omit<Theme, "palette" | "components"> & CssVarsTheme; }>'.
                          Type 'CSSObject & { variants?: { props: Partial<AlertProps & Record<string, unknown> & { ownerState: AlertProps & Record<string, unknown>; } & { ...; }> | ((props: Partial<...> & { ...; }) => boolean); style: CSSObject | ((args: { ...; }) => CSSObject); }[] | undefined; }' is not assignable to type 'Interpolation<AlertProps & Record<string, unknown> & { ownerState: AlertProps & Record<string, unknown>; } & { theme: Omit<Theme, "palette" | "components"> & CssVarsTheme; }>'.
                            Type 'CSSObject & { variants?: { props: Partial<AlertProps & Record<string, unknown> & { ownerState: AlertProps & Record<string, unknown>; } & { ...; }> | ((props: Partial<...> & { ...; }) => boolean); style: CSSObject | ((args: { ...; }) => CSSObject); }[] | undefined; }' is not assignable to type 'Keyframes'.
                              Type 'CSSObject & { variants?: { props: Partial<AlertProps & Record<string, unknown> & { ownerState: AlertProps & Record<string, unknown>; } & { ...; }> | ((props: Partial<...> & { ...; }) => boolean); style: CSSObject | ((args: { ...; }) => CSSObject); }[] | undefined; }' is missing the following properties from type '{ name: string; styles: string; anim: number; toString: () => string; }': name, styles, animts(2345)

I don't fully understand why.

Copy link
Member

Choose a reason for hiding this comment

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

The error is not related too much to the type that's being added, this is suspicious, let me check it again locally and I will report back.

Copy link
Member

Choose a reason for hiding this comment

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

Based on this type: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/styles/createTheme.ts#L38 the error is expected in my opinion. The issue is that in createTheme, we return this type: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/styles/createThemeNoVars.d.ts#L82 which has different definition for components than the input of createTheme. We should probably loosen the input type for createTheme, to accept a Theme, if we are allowing this use-case. We should also make sure that all proposed options from https://mui.com/material-ui/customization/theming/#nesting-the-theme are type safe.

Copy link
Member

@DiegoAndai DiegoAndai Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We discussed loosening createTheme's input type in the past, let's try it out @siriwatknp.

Copy link
Member Author

Choose a reason for hiding this comment

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

@DiegoAndai I tried loosen the type and include the Theme but it produces bigger errors in the existing spec files.
At this point, I think having the CSSPropertiesWithMultiValues without loosen the type might be the safest in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this change is causing major issues on mui-x styled usages with callback props:
mui/mui-x#17802
@siriwatknp, could you check if there is some oversight in the type? 🙏

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I am taking a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote the info in slack > https://mui-org.slack.com/archives/C011VC970AW/p1747795216756729

| (CSSObject & {
variants?: Array<{
props:
| Partial<Props>
| ((
props: Partial<Props> & {
ownerState: Partial<Props>;
},
) => boolean);
style:
| CSSObject
| ((args: Props extends { theme: any } ? { theme: Props['theme'] } : any) => CSSObject);
}>;
})
| ArrayInterpolation<Props>
| FunctionInterpolation<Props>;

Expand Down