[typescript] Improve type definition for withStyles

[pelotom]

This improves the TypeScript typing for withStyles using mapped types, rendering it more type safe and requiring fewer explicit type arguments. For example, you can write:

interface NonStyleProps {
  message: string
}
const Test = withStyles({ foo: { backgroundColor: 'red' } })<NonStyleProps>(props =>
  <div className={props.classes.foo}>{props.message}</div>
)

and it is fully type safe. The type of props is inferred to be both NonStyleProps and { classes: { foo: string } }.

[pelotom]

Resolves #8267.

[pelotom]

@oliviertassinari I can adapt @ssalbdivad's testcase in #8310 to this.

As an aside, I noticed that yarn typescript is currently failing, but the build passes. It looks like checking the typescript definitions is not incorporated into your CI config?

[oliviertassinari]

yarn typescript is currently failing

@pelotom 🙈 oops, I haven't added typescript for the CI correctly. We need it to pass!

[codecov-io]

Codecov Report

Merging #8320 into v1-beta will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           v1-beta    #8320   +/-   ##
========================================
  Coverage    99.42%   99.42%           
========================================
  Files          149      149           
  Lines         2256     2256           
========================================
  Hits          2243     2243           
  Misses          13       13

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 355b312...d24ec03. Read the comment docs.

[pelotom]

Ok, I updated the tests to reflect the new typings and also tested that this resolves #8267. yarn typescript is passing for me locally.

[oliviertassinari]

Does it mean people will have to duplicate the styles keys, one in the definition and one in the object?

[pelotom]

No, in fact you can now omit the type annotation completely here and get more type safety than you used to with the type annotation. You could also just annotate it as StyleRules as before and get the same (reduced) type safety that you had before (i.e., forgetting the particular style names and treating them as strings.)

[pelotom]

There should rarely be any reason for users to have to reference StyleRules explicitly in their code now.

[pelotom]

This is currently the least painful way in TypeScript that you can use a type-mutating decorator on a class.

[pelotom]

Ok, I'm done tweaking this PR :)

[oliviertassinari]

@pelotom The PR looks really great! Thanks a lot. Step by step you are making the typescript coverage high quality :).

[pelotom]

You’re welcome, glad to help out!

[sebald]

Just came back from vacation, this is an awesome improvement 👍 Thx you sooo much! :)

[sebald]

@oliviertassinari Can you add a BR notice on the next release for the TS typings? With this change it's no longer necessary to pass in a { [key:string]: string }, but rather the keys as union.

[oliviertassinari]

@sebald I'm not sure what I should be writing. Do you want to prepare the message for v1.0.0-beta.12? https://github.com/callemall/material-ui/blob/v1-beta/CHANGELOG.md

[sebald]

I only have one caveat: I am not sure how people will react, if they can't use decorators anymore. This was requested a lot (even though the spec is constantly changing people seem to use it a lot with TS /shrug).

[sebald]

@pelotom Could you explain what your reason was to move the props part to the returned HOC? :)

Talking about this:

- const StyledComponent = withStyles<
-   StyledComponentProps,
-   StyledComponentClassNames
- >(styles)(Component);
+ const StyledComponent = withStyles(styles)<StyledComponentProps>(
+   ({ classes, text }) => (
+     <div className={classes.root}>
+       {text}
+     </div>
+   )
+ );

Maybe we should support both options? Even though setting props beforehand may not be a good idea. But I see a lot of people writing their code like this (below code was removed by this PR):

const Component: React.SFC<
  StyledComponentProps & { classes: StyledComponentClassNames }
> = ({ classes, text }) =>
  <div className={classes.root}>
    {text}
  </div>;
 
const StyledComponent = withStyles<
  StyledComponentProps,
  StyledComponentClassNames
>(styles)(Component);

[pelotom]

@sebald

I only have one caveat: I am not sure how people will react, if they can't use decorators anymore. This was requested a lot (even though the spec is constantly changing people seem to use it a lot with TS /shrug).

Due to TypeScript #4881 I don't know of a way to make withStyles satisfy all of the following:

1. have the correct type as a function
2. avoid withStyles in Typescript, Props classes non nullable? #8267
3. usable as a decorator

With this PR I sacrificed (3) in order to achieve the first 2, which I think are more important.

[pelotom]

@pelotom Could you explain what your reason was to move the props part to the returned HOC? :)

The main reason is that the props variable P usually needs to be explicitly annotated, whereas the Names variable can almost always be inferred. If you put them in the same parameter group then you need to specify both, so you lose out on type inference. A secondary reason is that if you do

const hoc = withStyles(styles)

In principle you should now be able to apply hoc to any component whose props are consistent with the names in styles, so it's a bit silly to have already limited the props a priori.

Maybe we should support both options? Even though setting props beforehand may not be a good idea. But I see a lot of people writing their code like this (below code was removed by this PR):

True, you could probably overload it to be more backwards compatible. It feels like an antipattern to annotate withStyles with the type of props for the reason I gave above, so I was intentionally breaking backwards compatibility.

[pelotom]

After playing with it some more, I think I've found an alternative typing that would allow using withStyles as a class decorator in TypeScript, at the cost of a very small amount of type safety: under strict type checking you have to use the ! operator when accessing the classes prop in a decorated class, like so:

@withStyles(styles)
class DecoratedComponent extends StyledComponent<Props, 'root'> {
  render() {
    return (
      <div className={this.props.classes!.root}> // <-- note the `!`
        {this.props.text}
      </div>
    );
  }
}

This seems like a pretty good compromise, I'll open a PR and you can see what you think...

[scinos]

Can you provide an example with an stateful component?

I got it working with:

class MyComponent extends Component<MyProps & WithStyles<'root' | 'other'>, MyState> {
  render() {
    const { root, other } = this.props.classes;
    //...
  }
}

export default withStyles(styles)<MyProps>(
  (props) => (
    <MyComponent {...props}/>
  )
);

(I couldn't make it work with the class decorator)

Is that how it is supposed to work?

[pelotom]

You’ll want to use StyledComponentProps instead of WithStyles for a decorated component class.

[pelotom]

See this comment for a good example.

[scinos]

But if I use StyledComponentProps<'root'> then I have two new problems:

1. Can't use Partial as a type for defaultProps
2. Still need the stateless wrapper

This is the code:

@withStyles(styles)
class AddTweet extends Component<Props & StyledComponentProps<"tweetCount" | "tweetCountOverflow">, State> {
  static defaultProps: Partial<Props> = { //...}
  //...
}

export default withStyles(styles)<Props>(AddTweet);

And this is the error from the defaultProps

Argument of type 'typeof AddTweet' is not assignable to parameter of type 'ComponentClass<StyledComponentProps<"tweetCount" | "tweetCountOverflow">>'.
  Types of property 'defaultProps' are incompatible.
    Type 'Partial<Props>' is not assignable to type 'Partial<StyledComponentProps<"tweetCount" | "tweetCountOverflow">> | undefined'.
      Type 'Partial<Props>' has no properties in common with type 'Partial<StyledComponentProps<"tweetCount" | "tweetCountOverflow">>'.'

And this is the error from the wrapper

Argument of type 'typeof AddTweet' is not assignable to parameter of type 'StatelessComponent<Props & WithStyles<"tweetCount" | "tweetCountOverflow">>'.
  Type 'typeof AddTweet' provides no match for the signature '(props: Props & WithStyles<"tweetCount" | "tweetCountOverflow"> & { children?: ReactNode; }, context?: any): ReactElement<any> | null'.'

If I change defaultProps to be Partial<Props & StyledComponentProps<'tweetCount' | 'tweetCountOverflow'>>, the first error is solved. And if I add the stateless wrapper again, the second error is gone.

But... then what is the advantage of StyledComponentProps over WithStyles? Is there any way to avoid the stateless wrapper?

[pelotom]

@scinos

1. If you're not providing defaults for classes you can just use

   static defaultProps: Partial<Props & StyledComponentProps> = { /*...*/ }

2. I'm a little confused on the second point. Why are you trying to

   export default withStyles(styles)<Props>(AddTweet);

   when AddTweet was already annotated with @withStyles(styles)? Don't you just want:

   @withStyles(styles)
   export default class AddTweet extends React.Component<
     Props & StyledComponentProps<"tweetCount" | "tweetCountOverflow">,
     State
   > {
     //...
   }

In general if your component is stateless, you should just use

export default withStyles(styles)<Props>(props => /*...*/)

because it doesn't require all the noise of mentioning StyledComponentProps<...>. But if you require a class component, it's hard to avoid.

[scinos]

@pelotom You are right, my second case doesn't make sense 🤦‍♂️. Now it works perfectly, many thanks.

If someone comes here with a similar issue, this is how I solved it:

import { StyledComponentProps } from 'material-ui';

type Props = {
  prop1: string,
  prop2?: string
} & StyledComponentProps;

@withStyles(styles)
class MyComponent extends Component<Props, State> {
  static defaultProps: Partial<Props> = {
    prop2: 'hello'
  };

  render() {
     const class1 = this.props.classes!.class1; //Note the '!'
     //...
  }
}