Material-ui: [Material-UI/Style v4] Possible wrong typing for StyleRules

[VincentLanglet]

• [x] This is not a v0.x issue.
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

I expect no error from this

type ClassKey = 'option';

const useStyles = makeStyles(
  (): StyleRules<{}, ClassKey> => ({
    option: {
      backgroundColor: (): string => 'red',
    },
  }),
);

Current Behavior 😯

I have an error () => string is not assignable to string

This is of course correct

type ClassKey = 'option';

const useStyles = makeStyles(
  (): StyleRules<{}, ClassKey> => ({
    option: {
      backgroundColor: 'red',
    },
  }),
);

This is also correct

type ClassKey = 'option';

const useStyles = makeStyles(
  (): StyleRules<{}, ClassKey> => ({
    option: () => {
      backgroundColor: 'red',
    },
  }),
);

But differs from the doc (https://material-ui.com/css-in-js/basics/#adapting-hook-api).

Context 🔦

The typing for StyleRules is

export interface CSSProperties extends CSS.Properties<number | string> {
    // Allow pseudo selectors and media queries
    [k: string]: CSS.Properties<number | string>[keyof CSS.Properties] | CSSProperties;
  }

export type StyleRules<Props extends object, ClassKey extends string = string> = Record<
    ClassKey,
    CSSProperties | ((props: Props) => CSSProperties)
  >

Maybe we can fix it with something like

type CSSProperty = CSS.Properties<number | string>[keyof CSS.Properties];

  export interface CSSProperties extends CSS.Properties<number | string> {
    // Allow pseudo selectors and media queries
    [k: string]: CSSProperty | CSSProperties;
  }

  export interface CSSPropertiesByProps<Props extends object> {
    // Allow pseudo selectors and media queries
    [k: string]: ((props: Props) => CSSProperty | CSSProperties);
  }

  export type StyleRules<Props extends object, ClassKey extends string = string> = Record<
    ClassKey,
    CSSProperties | CSSPropertiesByProps<Props> | ((props: Props) => CSSProperties)
  >;

[eps1lon]

Please don't use StyleRules. It won't protect you against type widening which I suspect is happening here. Is this still failing with createStyles?

[VincentLanglet]

createStyles is only protecting against type widening because it types the return type.
By typing the return type by myself, I'm also protected and I use it because of the standards of our project. When you use the typedef rule from tslint you have to type function return type.

Because the issue is not about the use of StyleRules, since createStyles use it.

export default function createStyles<C extends string, P extends object>(
    styles: StyleRules<P, C>,
  ): StyleRules<P, C>;

Still the bug with

const useStyles = makeStyles(
  () => ({
    option: {
      backgroundColor: (): string => 'red',
    },
  }),
);

Same error with

const useStyles = makeStyles(() =>
  createStyles({
    option: {
      backgroundColor: (): string => 'red',
    },
  }),
);

[eps1lon]

Because the issue is not about the use of StyleRules, since createStyles use it.

Maybe not but I have to get rid of all the mental overhead possible.

I was under the impression that we could only write

const styles = {
  root: props => stylesForProps(props)
}

but it seems like we can also do

const styles = {
  root: {
    color: props => props.color
  }
}

?

Could you put together a codesandbox with all the possible patterns for styles depending on props?

[VincentLanglet]

Maybe not but I have to get rid of all the mental overhead possible.

Sure, no problem.

@eps1lon Like this ? https://codesandbox.io/s/p2wljkm9zm

I tested all possible case

const styles = () =>
  createStyles({
    text: {
      backgroundColor: "red", // WORKING
      color: () => "blue", // WORKING
      "&:hover": {
        marginLeft: 20, // WORKING,
        marginRight: () => 20 // WORKING,
      }
    },
    someText: () => ({
      backgroundColor: "red", // WORKING
      color: () => "blue", // NOT WORKING
      "&:hover": {
        marginLeft: 20, // WORKING,
        marginRight: () => 20 // WORKING,
      }
    }),
    otherText: {
      backgroundColor: "red", // WORKING
      color: () => "blue", // WORKING
      "&:hover": () => ({
        marginLeft: 20, // NOT WORKING,
        marginRight: () => 20 // NOT WORKING,
      })
    },
    finalText: () => ({
      backgroundColor: "red", // WORKING
      color: () => "blue", // NOT WORKING
      "&:hover": () => ({
        marginLeft: 20, // WORKING,
        marginRight: () => 20 // NOT WORKING,
      })
    })
  });

[eps1lon]

I can't find a consistent pattern there. I don't even know if everyone of these is supported. We need to confirm first with jss that this is all intended.

[VincentLanglet]

Sure, need to understand what is really supported before to type it.

In the meantime, the following doc (https://material-ui.com/css-in-js/basics/#adapting-based-on-props) should be updated to give example which respect the typescript typing

This is not critical since we can use it this way

const useStyles = makeStyles(
  (theme) => ({
    option: (props) => ({
      backgroundColor: props.isRed ? 'red' : '',
    }),
  }),
);

BTW, I was expecting a use like this

const useStyles = makeStyles(
  (theme, props) => ({
    option: {
      backgroundColor: props.isRed ? 'red' : '',
    },
  }),
);

Or

const useStyles = makeStyles(
  (theme) => (props) => ({
    option: {
      backgroundColor: props.isRed ? 'red' : '',
    },
  }),
);

To avoid declaring multiple functions using props

const useStyles = makeStyles(
  (theme) => ({
    option: (props) => ({ ... }),
    text: (props) => ({ ... }),
    title: (props) => ({ ... }),
    root: (props) => ({ ... })
  }),
);

[d-fischer]

+1 on this.

I tried this:

export type CSSProperties<Props extends object = object> = {
  [PropertyKey in string]:
      // pseudo selectors and media queries
      | CSSProperties<Props>
      | (PropertyKey extends keyof CSS.Properties<number | string> ? (
          // regular properties
          | CSS.Properties<number | string>[PropertyKey]
          // props to styles
          | ((props: Props) => CSS.Properties<number | string>[PropertyKey])
      ) : never);
};

export type StyleRules<Props extends object, ClassKey extends string = string> = Record<
  ClassKey,
  CSSProperties<Props>
>;

but unfortunately, it failed spectacularly with tons of errors similar to:

Error:(42, 13) TS2322: Type 'string' is not assignable to type 'CSSProperties<{}>'.

The compiler now errors on any regular property even though I think it should be covered by the "regular properties" line.

Hope you can improve on this.

Edit: I removed the class key problems, they were unrelated to this issue.

[eps1lon]

The actual fix for this should be #14797.