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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot use MUI's styleProps pattern to pass variants to styled() in TypeScript projects #27530

Open
2 tasks done
dantman opened this issue Jul 31, 2021 · 4 comments
Open
2 tasks done
Labels
docs Improvements or additions to the documentation package: system Specific to @mui/system typescript

Comments

@dantman
Copy link
Contributor

dantman commented Jul 31, 2021

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 馃槸

When using styled() to create styled components we need some way to pass things like variant props to the styles.

MUI's own internal code handles this by adding a styleProps prop and using that in styles. i.e. styleProps={{variant}} and then styleProps.variant === 'foo' in the style code.

However this does not work in projects written in TypeScript. CreateMUIStyled gets the props that are permitted from the component/tag you are forwarding props to. As a result the created component does not accept any prop other than the component's own props. So all styled-only props (styleProps, variant, etc...) are type errors.

Expected Behavior 馃

There should be some way to pass style props like variant to components made with styled() in a type safe way.

One possibility could be officially codifying the styleProps pattern and adding a defaultStyleProps to styled's options. Just like CreateMUIStyled infers props from the component/tag the type could be written in a way that styleProp's type would be inferred by the type of the defaultStyleProps you pass.

Steps to Reproduce 馃暪

Edit cranky-bogdan-khxyt

Your Environment 馃寧

`npx @material-ui/envinfo`

  System:
    OS: Linux 5.10 Ubuntu 20.10 (Groovy Gorilla)
  Binaries:
    Node: 16.5.0 - ~/.nvm/versions/node/v16.5.0/bin/node
    Yarn: 1.22.5 - /mnt/c/Program Files (x86)/Yarn/bin/yarn
    npm: 7.19.1 - ~/.nvm/versions/node/v16.5.0/bin/npm
  Browsers:
    Chrome: Not Found
    Firefox: Not Found
  npmPackages:
    @emotion/react: ^11.4.0 => 11.4.0 
    @emotion/styled: ^11.3.0 => 11.3.0 
    @material-ui/core: ^5.0.0-beta.2 => 5.0.0-beta.2 
    @material-ui/icons: ^5.0.0-beta.1 => 5.0.0-beta.1 
    @material-ui/lab: ^5.0.0-alpha.41 => 5.0.0-alpha.41 
    @material-ui/private-theming:  5.0.0-beta.2 
    @material-ui/styled-engine:  5.0.0-beta.1 
    @material-ui/styles: ^5.0.0-beta.2 => 5.0.0-beta.2 
    @material-ui/system:  5.0.0-beta.2 
    @material-ui/types:  6.0.1 
    @material-ui/unstyled:  5.0.0-alpha.41 
    @material-ui/utils: ^5.0.0-beta.1 => 5.0.0-beta.1 
    @types/react: ^17.0.15 => 17.0.15 
    react: ^17.0.2 => 17.0.2 
    react-dom: ^17.0.2 => 17.0.2 
    styled-components:  5.2.1 
    typescript: ~4.2.0 => 4.2.4 

@dantman dantman added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 31, 2021
@eps1lon
Copy link
Member

eps1lon commented Aug 2, 2021

Thanks for the feedback.

const DemoRoot = styled("div", {
  name: "Demo",
  slot: "Root"
})(({ theme, styleProps }) => ({
  backgroundColor: "lightgrey",
  ...(styleProps.variant === "red" && {
    backgroundColor: "red"
  }),
  ...(styleProps.variant === "blue" && {
    backgroundColor: "blue"
  })
}));

is already not passing type-checking since a div does not implement styleProps. You need to tell styled that you implement that prop instead.

const DemoRoot = styled("div", {
  name: "Demo",
  slot: "Root"
-})(({ theme, styleProps }) => ({
+})<{ styleProps: {variant: 'blue' | 'red' }}>(({ theme, styleProps }) => ({
  backgroundColor: "lightgrey",
  ...(styleProps.variant === "red" && {
    backgroundColor: "red"
  }),
  ...(styleProps.variant === "blue" && {
    backgroundColor: "blue"
  })
}));

Might be interesting to explore if we can automatically augment the types with all the literals returned from shouldForwardProp (and its default implementation).

Though I fear that TypeScript will default to returning string which means you can easily defeat type-checking to accept any Record<string, unknown>. But I'm just guessing here so we would need to check first. PRs welcome.

@eps1lon eps1lon added new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted package: styled-engine Specific to @mui/styled-engine typescript and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 2, 2021
@gurkerl83
Copy link

The topic described in the present issue seems related to #27234.

The basic question here seems to be how the object styleProps could be generated in its form in a dynamic way.

Perhaps for the typing of the overridesResolver also the values returned by the function shouldForwardProp is meaningful, in order to compute a flexible type.

Thx!

@mnajdova
Copy link
Member

mnajdova commented Aug 3, 2021

The basic question here seems to be how the object styleProps could be generated in its form in a dynamic way.

Perhaps for the typing of the overridesResolver also the values returned by the function shouldForwardProp is meaningful, in order to compute a flexible type.

Thx!

What exact problem is not solved with #27530 (comment)? You are providing the props per component creation, you can make them loose if you need to. Can you provide more concrete example of what is not working or is difficult to be implemented.

@siriwatknp siriwatknp added docs Improvements or additions to the documentation package: system Specific to @mui/system and removed new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted package: styled-engine Specific to @mui/styled-engine labels Oct 26, 2022
@siriwatknp
Copy link
Member

I think this should be included in the docs instead of making it as a feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: system Specific to @mui/system typescript
Projects
None yet
Development

No branches or pull requests

5 participants