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

siriwatknp · 2025-03-04T11:53:14Z

The issue is that Omit<CSSOthersObject, 'variants'> doesn't work as expected as CSSOthersObject is a mapped type, which Omit has known issues with:

"in keyof" type mapping incorrectly interacts with indexers microsoft/TypeScript#42436
Using keyof with key remapping to exclude index signatures doesn't return known keys microsoft/TypeScript#41966

In a nutshell: By design, Omit won't remove variants from CSSOthersObject, so the type defined in CSSObjectWithVariants is not used.

Solution and discussion below

mui-bot · 2025-03-04T11:55:44Z

Netlify deploy preview

https://deploy-preview-45478--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 48dda59

siriwatknp · 2025-03-05T02:47:12Z

docs/data/material/getting-started/templates/marketing-page/components/Features.js

@@ -42,7 +42,7 @@ const items = [
 const Chip = styled(MuiChip)(({ theme }) => ({
  variants: [
    {
-      props: ({ selected }) => selected,
+      props: ({ selected }) => !!selected,


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?

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

Does this mean this is a breaking change?

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?

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.

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

Let's do that.

siriwatknp · 2025-03-05T02:50:56Z

packages/mui-styled-engine/src/index.d.ts

+  | ComponentSelector
+  | Keyframes
+  | SerializedStyles
+  | CSSPropertiesWithMultiValues


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

What do you mean with "myth"?

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:

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.

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.

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.

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

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

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? 🙏

Sure, I am taking a look.

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

DiegoAndai

Thanks for taking a look at this @siriwatknp! 😊

I have a couple of questions

packages/mui-styled-engine/src/index.d.ts

…ltiValues

DiegoAndai · 2025-03-07T18:58:43Z

Hey @siriwatknp! I took a look at this, and I managed to remove CSSPropertiesWithMultiValues from Interpolation by removing components from the Theme interface, see daa9579.

The CssVarsTheme and Joy's Theme types do not have the components property, so I wonder if it being included in Material's Theme was a mistake.

Do you think this is the correct approach?
This fixed the createTheme(theme issue, but it brought up a bunch of other ones 😅 I will continue taking a look on Monday

siriwatknp · 2025-03-10T02:08:08Z

Hey @siriwatknp! I took a look at this, and I managed to remove CSSPropertiesWithMultiValues from Interpolation by removing components from the Theme interface, see daa9579.

I suggest not removing it as it will introduce another issue for people who read theme.components.* like this:

const theme = createTheme({
  component: { … }
})

console.log(theme.components); // << TS error.

Also, removing the components is technically incorrect because the theme object contains components node (in JS).

…esWithMultiValues" This reverts commit daa9579.

DiegoAndai · 2025-03-14T19:10:10Z

@siriwatknp I closed #45346 in favor of this PR, lets move forward with this solution

@mnajdova may I ask you to review this?

mnajdova

I will report soon with my findings.

mnajdova · 2025-03-24T15:29:45Z

docs/data/material/getting-started/templates/marketing-page/components/Features.js

@@ -42,7 +42,7 @@ const items = [
 const Chip = styled(MuiChip)(({ theme }) => ({
  variants: [
    {
-      props: ({ selected }) => selected,
+      props: ({ selected }) => !!selected,


Does this mean this is a breaking change?

mnajdova · 2025-03-24T15:30:58Z

packages/mui-material/src/styles/createTheme.spec.ts

@@ -218,6 +218,7 @@ const theme = createTheme();
            variants: [
              {
                props: ({ ownerState }) => ownerState.color === 'primary',
+                style: {},


Wasn't style required before?

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

Yes. that's correct.

mnajdova · 2025-03-24T15:47:40Z

packages/mui-styled-engine/src/index.d.ts

+  | ComponentSelector
+  | Keyframes
+  | SerializedStyles
+  | CSSPropertiesWithMultiValues


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.

DiegoAndai

LGTM, let's release the fix.

@mnajdova FYI:

There are some type changes that might be perceived as "breaking":
- Variants' props is now correctly typed as () => boolean
- Variants' styles is now correctly required

These were always the intended types, but they weren't being used, which is the bug we're fixing now. Because they're now correctly used, this effectively changed the resolved types.

Because these are the correct types, Jun and I think this is a fix worth releasing in v7. But because it can be perceived as breaking, the compromise is to release it in a minor bump and explain it in the CHANGELOG. Adapting to these changes is straightforward.

In the worst case scenario, if too many users complain, plan B is to loosen these types in a patch version after the minor one.

poc

9618878

siriwatknp added the typescript label Mar 4, 2025

siriwatknp added 4 commits March 4, 2025 21:52

fix types

04ea73d

docs ts format

cade9e8

fix types

d7ec526

fix types

1537b32

siriwatknp commented Mar 5, 2025

View reviewed changes

DiegoAndai reviewed Mar 5, 2025

View reviewed changes

packages/mui-styled-engine/src/index.d.ts Show resolved Hide resolved

Test alternative fix which doesn't require adding CSSPropertiesWithMu…

daa9579

…ltiValues

DiegoAndai added 2 commits March 14, 2025 15:50

Revert "Test alternative fix which doesn't require adding CSSProperti…

cbe257a

…esWithMultiValues" This reverts commit daa9579.

Merge branch 'master' into fix/styleoverrides-variants-type

e91681e

DiegoAndai mentioned this pull request Mar 14, 2025

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

Closed

DiegoAndai changed the title ~~[POC] Fix Interpolation types~~ styled-engine] Fix style overrides variants type Mar 14, 2025

DiegoAndai changed the title ~~styled-engine] Fix style overrides variants type~~ [styled-engine] Fix style overrides variants type Mar 14, 2025

DiegoAndai requested a review from mnajdova March 14, 2025 19:09

DiegoAndai self-assigned this Mar 14, 2025

DiegoAndai marked this pull request as ready for review March 14, 2025 19:09

mnajdova reviewed Mar 24, 2025

View reviewed changes

siriwatknp requested review from DiegoAndai and mnajdova April 9, 2025 09:02

Merge branch 'master' into fix/styleoverrides-variants-type

48dda59

DiegoAndai approved these changes Apr 10, 2025

View reviewed changes

siriwatknp merged commit ef07974 into mui:master Apr 11, 2025
22 checks passed

siriwatknp mentioned this pull request May 5, 2025

[styled-engine] Infer ownerState from props in styled #46083

Merged

1 task

ZeeshanTamboli mentioned this pull request May 12, 2025

[styled-engine] Type Interpolation is not assignable to type CSSObject #46101

Closed

Uh oh!

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

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

Uh oh!

Conversation

siriwatknp commented Mar 4, 2025 • edited by DiegoAndai Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mui-bot commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Netlify deploy preview

Bundle size report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siriwatknp Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiegoAndai Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiegoAndai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DiegoAndai commented Mar 7, 2025

Uh oh!

siriwatknp commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DiegoAndai commented Mar 14, 2025

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DiegoAndai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

siriwatknp commented Mar 4, 2025 •

edited by DiegoAndai

Loading

mui-bot commented Mar 4, 2025 •

edited

Loading

siriwatknp Mar 6, 2025 •

edited

Loading

DiegoAndai Apr 2, 2025 •

edited

Loading

siriwatknp commented Mar 10, 2025 •

edited

Loading