cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.08k stars 400 forks source link

Wrong TS type infered for object created using createUseStyles #1479

Open MaximeCheramy opened 3 years ago

MaximeCheramy commented 3 years ago

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:

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):

ITenthusiasm commented 3 years ago

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 commented 3 years ago

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. :smile: (At least in my opinion. :sweat_smile:) 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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

pip8786 commented 3 years ago

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 commented 3 years ago

Do you have a code snippet for more context?

pip8786 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.