search

mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.59k stars 32.2k forks source link

[styled-engine-sc] TS error when using a `css`-variable within `styled` #40140

Open delijah opened 10 months ago

delijah commented 10 months ago

Duplicates

Latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/p/devbox/mui-theme-scoping-within-css-and-css-within-styled-knpr8m?file=%2Fsrc%2FApp.tsx

Steps:

  1. Open the provided link

Current behavior 😯

  1. TS throws error when using a css-variable within styled: Property 'variants' is missing in type 'Interpolation<object>[]' but required in type 'CSSObjectWithVariants<any>'.
  2. We have to use THEME_ID to access the MUI theme

Expected behavior 🤔

  1. It must be possible to use a css-variable within styled, without getting a TS error.
  2. Theme scoping should work fine, also within css. There should be no need to use the THEME_ID variable

Context 🔦

We try to use MUI with theme scoping with styled-components v6.

Your environment 🌎

npx @mui/envinfo ``` Used Browser: Chrome System: OS: macOS 12.6.1 Binaries: Node: 16.20.1 - /usr/local/bin/node Yarn: 1.22.19 - /usr/local/bin/yarn npm: 8.19.4 - /usr/local/bin/npm Browsers: Chrome: 119.0.6045.199 Edge: Not Found Safari: 15.6.1 npmPackages: @mui/base: 5.0.0-beta.25 @mui/core-downloads-tracker: 5.14.19 @mui/material: 5.14.19 => 5.14.19 @mui/private-theming: 5.14.19 @mui/styled-engine: 5.14.19 @mui/styled-engine-sc: 6.0.0-alpha.7 => 6.0.0-alpha.7 @mui/system: 5.14.19 @mui/types: 7.2.10 @mui/utils: 5.14.19 @types/react: 17.0.70 => 17.0.70 react: 17.0.2 => 17.0.2 react-dom: 17.0.2 => 17.0.2 styled-components: 6.1.0 => 6.1.0 typescript: 4.8.4 => 4.8.4 ```

Support Key

80425

KumarNitin19 commented 10 months ago

@delijah I have fixed this issue by just removing css from colors variable, you can refer the screenshot below

KumarNitin19 commented 10 months ago
Screenshot 2023-12-08 at 1 01 47 PM
delijah commented 10 months ago

@delijah I have fixed this issue by just removing css from colors variable, you can refer the screenshot below

Thank you @KumarNitin19 for your suggestion. If i do so, i get the following TS error for the theme argument variable: Binding element 'theme' implicitly has an 'any' type.

Which makes sense, because how should TS know what theme is? We just have a plain template string now. Nothing that tells TS that something (like css) will inject a theme variable.

And using the type any, like you do in your example, is not really a solution for us. We need a proper typed theme variable within our css code.

KumarNitin19 commented 10 months ago

Hey @delijah can you please refer below screenshot for this bug, feel free to check out this codesandbox: https://codesandbox.io/p/devbox/mui-theme-scoping-within-css-and-css-within-styled-forked-pykhvq?

file=%2Fsrc%2FApp.tsx%3A3%2C1

Screenshot 2023-12-08 at 3 07 34 PM
delijah commented 10 months ago

Hey @delijah can you please refer below screenshot for this bug, feel free to check out this codesandbox:

Thanks again for you suggestion @KumarNitin19. We are actually not looking for a quickfix. I assume there are several ways of somehow getting around the bug. We work on a big codebase and depend on being able to use the css function to define styles and then re-use those styles in other parts of the app within styled-templates. If i understand the MUI docs correctly, this is something that must work out of the box.

delijah commented 10 months ago

@zannager @siriwatknp Added our support key to the issue description.

mnajdova commented 10 months ago

It is related to using @mui/styled-engine-sc, no? Have you checked if it happens with the default setup too?

delijah commented 10 months ago

It is related to using @mui/styled-engine-sc, no? Have you checked if it happens with the default setup too?

Yes, it looks like with the default installation it works fine. But not, as you're suggesting, with @mui/styled-engine-sc.

mnajdova commented 10 months ago

I am curious in general and I always tend to ask this question :) Why have you decided to go with @mui/styled-engine-sc? I am asking because there are known issues for e.g. with SSR (see https://mui.com/material-ui/guides/styled-components/). I am taking a look at the issue now.

mnajdova commented 10 months ago

I will look into the typing issue, I agree the css util output should be usable in the styled util.

On the second issue around having to use THEME ID, the reasoning for this is that the the css util that we export from @mui/material/styles is the util exported directly from the styled engine (emotion or styled-components). For all other utils, we have a wrapper where we make sure that you don't have to use the THEME_ID (except this one). Historically we have exported it so that people have one place to import styling utils from, however I can see how this is confusing for people. @siriwatknp what are your thoughts on this? We should either support this in the css util, or make it clear in the docs that it is not supported. I've seen other issues around wrong expectations on this prop, I am starting to think that it was a bad call to re-export it.

delijah commented 10 months ago

I am curious in general and I always tend to ask this question :) Why have you decided to go with @mui/styled-engine-sc? I am asking because there are known issues for e.g. with SSR (see https://mui.com/material-ui/guides/styled-components/). I am taking a look at the issue now.

Sure :) Basically because we already use styled-components all over our stack and we do not want to add an extra dependency which we do not use anywhere.

We do not use SSR and we will not in the future, therefore we are happy to not have to rely on that :)

siriwatknp commented 10 months ago

I will look into the typing issue, I agree the css util output should be usable in the styled util.

On the second issue around having to use THEME ID, the reasoning for this is that the the css util that we export from @mui/material/styles is the util exported directly from the styled engine (emotion or styled-components). For all other utils, we have a wrapper where we make sure that you don't have to use the THEME_ID (except this one). Historically we have exported it so that people have one place to import styling utils from, however I can see how this is confusing for people. @siriwatknp what are your thoughts on this? We should either support this in the css util, or make it clear in the docs that it is not supported. I've seen other issues around wrong expectations on this prop, I am starting to think that it was a bad call to re-export it.

I missed this one. Developer should not need to access the THEME_ID, the css util should resolve the theme[THEME_ID].

mnajdova commented 10 months ago

Thanks @siriwatknp I will leave that one to you, and I will try to resolve the TypeScript issue by next week.

delijah commented 8 months ago

Hello @siriwatknp and @mnajdova . I've created two PR's which solve part 1 and 2 of this issue:

https://github.com/mui/material-ui/pull/40640 https://github.com/mui/material-ui/pull/40656

Looking forward 😃