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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong TS type infered for object created using createUseStyles #1479

Open
MaximeCheramy opened this issue Mar 24, 2021 · 13 comments
Open

Wrong TS type infered for object created using createUseStyles #1479

MaximeCheramy opened this issue Mar 24, 2021 · 13 comments

Comments

@MaximeCheramy
Copy link

MaximeCheramy commented Mar 24, 2021

Expected behavior:

Given the following snippet:

const useStyles = createUseStyles((theme) => ({
  SectionTitle: {
    color: theme.color, // Commenting out this line fixes the bug

    // Bullet
    "&:before": {
      content: (props: JssProps) => `"${props.number}"`,
      border: [1, "solid", "black"]
    }
  }
}));

The type should be:

const useStyles: (data?: JssProps & {
    theme?: Jss.Theme;
}) => Record<"SectionTitle", string>

But instead, it's:

const useStyles: (data?: {
    theme?: Jss.Theme;
}) => Record<"SectionTitle", string>

Describe the bug:

This only occurs in a very specific case:

  • We use the theme in the JSS
  • The Theme is not correctly defined (any)

My theme is actually generated by a webpack plugin, that's why the type not precise.

Reproduction:

https://codesandbox.io/s/bug-type-jss-j5gkw

Versions (please complete the following information):

  • jss: 10.6.0
  • Browser [e.g. chrome, safari]: Chrome / FireFox
  • OS [e.g. Windows, macOS]: Linux
@ITenthusiasm
Copy link
Member

First off, thank you so much for adding a codesandbox. Plus the example code. Makes life much easier.

Ugh. Yeah that really bites. The issue is that TypeScript is trying to infer what Props and Theme are since they weren't provided as generic type arguments. When it comes to the recursively defined JssStyles object, TS seems to run into a bit of trouble.

I'm looking into this. I can't guarantee that a simple fix will be possible; but hopefully I (or someone else) can find one.

In the meantime, there's workaround:

const useStyles = createUseStyles((theme) => ({
  SectionTitle: (props: JssProps) => ({
    color: theme.color,

    // Bullet
    "&:before": {
      content: `"${props.number}"`,
      border: [1, "solid", "black"]
    }
  })
}));

If you can clarify the Props in a function at the root level of the object -- before TypeScript dives into the recursive JssStyles loophole -- TS will be able to properly type Props and Theme. Depending on the kinds of style objects you're making, this may create a slight inconvenience if you want things looking a certain way. But this at least provides all the functionality you need.

Pro tip for the future:
You can add syntax highlighting to your code blocks by specifying the language you're using. For example: ```typescript

@ITenthusiasm
Copy link
Member

UPDATE

Here's another workaround that I discovered:

const useStyles2 = createUseStyles((theme) => ({
  SectionTitle: {
    color: theme.color as string,

    // Bullet
    "&:before": {
      content: (props: JssProps) => `"${props.number}"`,
      border: [1, "solid", "black"]
    }
  }
}));

This one is much less inconvenient, visually. 😄 (At least in my opinion. 😅) For some reason, TypeScript seems to get really confused when the type you give to a CSS Property (color, fontSize, etc.) is any. I'm sure this still relates to the recursive JssStyles type; I'll continue investigating to see if there's a way to keep users from having to do workarounds.

@ITenthusiasm
Copy link
Member

Another update. (This update likely won't interest anyone searching for workarounds/fixes.)

I played around with this code, and apparently it produces the same error:

const useStyles = createUseStyles((theme) => ({
  SectionTitle: {
    color: '' as any,

    // Bullet
    '&:before': (props: MyProps) => ({
      content: `"${props.property}"`,
      border: [1, 'solid', 'black']
    })
  }
}))

Apparently, if the Props are not specified at the root level of the object in a data function and a CSS Property is typed as any, TS will be unable to determine Props. More accurately, TS will assume Props to be unknown. Since unknown & { theme?: T } is the same as { theme?: T }, that's what data for useStyles gets simplified to.

Of course, no one who's explicitly providing the necessary generic type arguments will run into this issue.

This is the extent of my investigation for now. I'll try to look into it more later. But seeing as TS is getting upset specifically when the CSS Properties are typed unpredictably, I don't know how deeply this should be explored. @MaximeCheramy you can let me know if you have any thoughts about any of this. I hope the workarounds I provided work for you in the meantime.

@pip8786
Copy link

pip8786 commented Mar 29, 2021

Not sure if this the right place to comment about the 10.6 parameter changes, but we've downgraded to 10.5 to avoid all the errors for now until this is figured out. I wonder if we can look at what Material UI did to wrap react-jss because IIRC you'd get props types and css classname checking on there too.

@ITenthusiasm
Copy link
Member

@pip8786 Are you getting this same exact error? Or are you just talking about the re-ordering of parameters?

@pip8786
Copy link

pip8786 commented Mar 29, 2021

I'm getting a lot of errors of this type:
src/components/teams/CreateTeams.tsx(233,30): error TS2339: Property 'background' does not exist on type '{ theme: Theme; }'.
background in this case is part of the props which are not typed since that wasn't working well in react-jss (at least last I tried).
I haven't counted but it looks like 100s in our project. If this is a separate discussion I can go back to version 10.6.0 and get more details.

@ITenthusiasm
Copy link
Member

Do you have a code snippet for more context?

@pip8786
Copy link

pip8786 commented Mar 29, 2021

Here's one of the smaller examples I could find:

const useStyles = createUseStyles({
    pre: {
        fontFamily: '"Roboto", "Helvetica", "Arial", "sans-serif"',
        whiteSpace: "pre-wrap",
        margin: 0,
        color: ({color}) => color ? color : "white",
        fontWeight: ({weight}) => weight ? weight : 400,
        wordWrap: ({breakWords}) => breakWords ? "break-words" : "inherit"
    }
});
src/components/UserProvidedText.tsx(19,18): error TS2339: Property 'color' does not exist on type '{ theme: Theme; }'.
src/components/UserProvidedText.tsx(20,23): error TS2339: Property 'weight' does not exist on type '{ theme: Theme; }'.
src/components/UserProvidedText.tsx(21,21): error TS2339: Property 'breakWords' does not exist on type '{ theme: Theme; }'.
src/components/UserProvidedText.tsx(27,32): error TS2345: Argument of type '{ color: string | undefined; weight: number | undefined; breakWords: boolean | undefined; }' is not assignable to parameter of type '{ theme: Theme; } & { theme?: Theme | undefined; }'.

@ITenthusiasm
Copy link
Member

Okay, thanks. And just for clarity...

Let's say your props were named MyProps. Would the error go away if, for that code block, you did something like this:

const useStyles = createUseStyles<"pre", MyProps>({
    pre: {
        fontFamily: '"Roboto", "Helvetica", "Arial", "sans-serif"',
        whiteSpace: "pre-wrap",
        margin: 0,
        color: ({color}) => color ? color : "white",
        fontWeight: ({weight}) => weight ? weight : 400,
        wordWrap: ({breakWords}) => breakWords ? "break-words" : "inherit"
    }
});

@pip8786
Copy link

pip8786 commented Mar 29, 2021

Yes that fixes it but like I said, this was one simple example of many places where this errors out. I'm not sure I want to specify every single classname coming out of the createUseStyles() just to get typed props. Based on the discussions i read around this issue, it sounds like this may be avoidable in the future if Typescript fixes some issues, so I think for our project we rather wait until that happens and stick to 10.5.0 for now.

Was I wrong about Material UI having figured this out? I know I didn't specify class names coming out of that but had props completion as well classnames, I think.

@ITenthusiasm
Copy link
Member

Yes that fixes it but like I said, this was one simple example of many places where this errors out.

Yes, I understand. I was mainly asking to get a handle on what the actual problem was on your end. Your problem seems to be separate from the one in this issue.

Was I wrong about Material UI having figured this out? I know I didn't specify class names coming out of that but had props completion as well classnames, I think.

I'm uncertain. I've only started to get some familiarity with this library, and I haven't dived too deep into Material UI's code. It's possible they either solved the problem React-JSS faces or found a workaround (and required users to use that workaround). Given the complexity of this issue though, I have my doubts.

Are you sure they use React-JSS? Or do they just use JSS?

@pip8786
Copy link

pip8786 commented Mar 29, 2021

It appears they use JSS:
https://github.com/mui-org/material-ui/blob/next/packages/material-ui-styles/package.json#L57

I just tested it on another project I have that uses Material UI and it doesn't seem to autocomplete the class names, though it does have props checking. Looks like it works a lot like the previous version of react-jss based on what I'm testing and see here: https://github.com/mui-org/material-ui/blob/next/packages/material-ui-styles/src/makeStyles/makeStyles.d.ts

@ITenthusiasm
Copy link
Member

Yeah. I guess I'm not surprised. This is a pretty bothersome bind that TypeScript puts us in. I really really hoping it gets resolved in the near future.

lotorvik added a commit to navikt/skjemabygging-formio that referenced this issue Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants