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.57k stars 32.19k forks source link

[styled-engine-sc] Types are not taken into account when using `styled` function with a template string #40498

Open delijah opened 9 months ago

delijah commented 9 months ago

Steps to reproduce

Link to live example: https://github.com/DND-IT/material-ui-cra-styled-components-ts/pull/1

Steps:

  1. Checkout the project
  2. Hover the position variable to see, that the type is any

Current behavior

Screenshot 2024-01-09 at 14 32 53

Expected behavior

The provided types must be taken into account

Context

We try to use proper typed styled-components

Your environment

npx @mui/envinfo ``` System: OS: macOS 12.6.1 Binaries: Node: 20.10.0 - /usr/local/bin/node Yarn: 1.22.19 - /usr/local/bin/yarn npm: 10.2.3 - /usr/local/bin/npm Browsers: Chrome: 120.0.6099.199 Edge: Not Found Safari: 15.6.1 npmPackages: @mui/base: 5.0.0-beta.30 @mui/core-downloads-tracker: 5.15.3 @mui/lab: latest => 5.0.0-alpha.159 @mui/material: latest => 5.15.3 @mui/private-theming: 5.15.3 @mui/styled-engine: 5.15.3 @mui/styled-engine-sc: 6.0.0-alpha.11 => 6.0.0-alpha.11 @mui/system: 5.15.3 @mui/types: 7.2.12 @mui/utils: 5.15.3 @types/react: latest => 18.2.47 react: latest => 18.2.0 react-dom: latest => 18.2.0 styled-components: latest => 6.1.8 typescript: latest => 4.9.5 ```

Support Key

80425

Search keywords: styled-engine-sc styled-components typescript

mnajdova commented 9 months ago

@delijah are you open to trying to create a fix for it? I don't have the bandwidth to look into this soon, but I can likely help with the review. I can't bump the priority of this issue, considering that @mui/styled-engine-sc is about 10% of the downloads (https://npm-stat.com/charts.html?package=%40mui%2Fstyled-engine-sc&package=%40mui%2Fstyled-engine).

delijah commented 8 months ago

Unfortunately we do not have the time either. We gave the emotion version a try, but it appears that you cannot really use variables inside the css template function the way we could with styled-components.

So we are kind of stuck here, we have to make up our minds. Actually it was one of our big reasons to move towards MUI, because we thought we could use all the styled-components functionality. So it's a bit frustrating to find out the opposite in the middle of the migration of our whole application (which is quite big).

delijah commented 8 months ago

I just gave it a try, but.. this is the spot where styled-components and material-ui come together. To provide a proper fix, i think one needs to have detailed knowledge about the internals of styled-components and material-ui. This is simply not realistic for me to solve.

delijah commented 8 months ago

Just one thing i've found during my investigation. I know it is far from any proper fix, but it might a ring a bell for you @mnajdova . If i replace this

export type Interpolation<P> =
  | InterpolationValue
  | CSSObjectWithVariants<P>
  | InterpolationFunction<P>
  | FlattenInterpolation<P>;

with that

export type Interpolation<P> =
  | InterpolationValue
  | CSSObjectWithVariants<P>
  | InterpolationFunction<P>
  | FlattenInterpolation<P>
  | { [key: string]: any };

inside packages/mui-styled-engine-sc/src/index.d.ts, the error goes away and the types are applied correctly.

How did i come up with this idea? After getting rid of the missing variants property error, by commenting line 111 variants: Array<{ props: Props; style: CSSObject }>; (temporarily), the next error that was coming up was Index signature for type 'string' is missing in type 'Interpolation<......

mnajdova commented 8 months ago

Unfortunately we do not have the time either. We gave the emotion version a try, but it appears that you cannot really use variables inside the css template function the way we could with styled-components.

Can you elaborate on this? There is this API in emotion: https://emotion.sh/docs/css-prop#string-styles

| { [key: string]: any };

This is more or less similar as adding any on the type.


Btw, everything work as expected with the default setup, see https://codesandbox.io/p/sandbox/ts-styled-v5s2rz?file=%2Fsrc%2FApp.tsx%3A21%2C5. I would recommend looking into the differences of the types between https://github.com/mui/material-ui/blob/master/packages/mui-styled-engine-sc/src/index.d.ts and https://github.com/mui/material-ui/blob/master/packages/mui-styled-engine/src/index.d.ts

delijah commented 8 months ago

Can you elaborate on this? There is this API in emotion: https://emotion.sh/docs/css-prop#string-styles

Sure. This is what we can do with styled-components:

const colorCss = css<{ color: string }>`
    color: ${({ color }) => color};
`;

const Wrapper = styled<{ color?: boolean }>('div')`
    display: flex;
    ${({ color }) => color && colorCss}
`;

const SomeComponent = ({ color }) => <Wrapper color={color} />;

But as far as i understand this pattern is not possible with emotion. We cannot really read from props within css in emotion. We would have to do something like that:

const Wrapper = styled('div')`
    display: flex;
`;

const SomeComponent = ({ color }) => {
    const colorCss = css`
        color: ${color};
    `;

    return <Wrapper css={color && colorCss} />
};

This is also why that problem does not exist with the default styled-engine. The css function cannot read from props and therefore you do not have to set types for props on the css function.

And we have a lot of situations like these in our app and we also like to do things that way :)

delijah commented 8 months ago

This is more or less similar as adding any on the type.

Haha yes, but the types for our use case are applied correctly 😅 But yeah, I assume this will cause troubles somewhere else..

delijah commented 8 months ago

Ok, there we go. I've created a PR. It fixes the issues mentioned in this issue. It's hard for me to grasp if this could lead to any unwanted side effects.

Side note: The theme variable inside css functions is still typed wrong, until https://github.com/mui/material-ui/issues/40140 is fixed as well.

delijah commented 8 months ago

Can i somehow use all the mui packages in my project locally by using npm link? Is there a guide available how to do this? I am a bist lost here. It worked well for styled-engine-sc, but for the material package i get a lot of errors about files that are not being found...

mnajdova commented 8 months ago

Can i somehow use all the mui packages in my project locally by using npm link? Is there a guide available how to do this? I am a bist lost here. It worked well for styled-engine-sc, but for the material package i get a lot of errors about files that are not being found...

You should be able to point in your package.json to the PR sandboxed packages version. Did you resolve this?

delijah commented 8 months ago

You should be able to point in your package.json to the PR sandboxed packages version. Did you resolve this?

What i've meant is, i'd like to make changes on the mui packages and immediately use those changes in our project. Without the need of waiting for something to build or deploy. By using npm link i could achieve it for styled-engine-sc, but not for the other libraries.

How i am doing it now is, after every change in a mui package, i build it, and copy it manually in my projects node_modules folder. It works, but is not very convenient.