[typescript] Style typing improvements

[pelotom]

• Fixes definition of Omit so that it distributes across union types. This allows us to give better return types for withStyles, withTheme and withWidth.
• Use a custom AnyComponent definition instead of React.ComponentType, which allows us to get rid of the ugly type annotation when using union-typed props. Remove corresponding caveat from the docs.
• Better typing of withStyles so that if the type checker can statically determine that you passed the withTheme: true option to withStyles, then theme will not be undefined in the inner component's props.

Resolves #12070, supersedes #12106.
Closes #12106

[gordonmleigh]

@pelotom Commenting here because it was reasonably recently and might be fresh in your mind.

Whatever you've done to withStyles here prevents type inference for styles which use the theme, e.g.:

const styleDecorator = withStyles(theme => ({
  active: {
    backgroundColor: theme.palette.grey[200],
  },
}));

Here the type params of withStyles are inferred as <never, {}> (don't know why) which means that the classes prop of a wrapped component has type Record<never, string> which is obviously no use.

It isn't clear to me why you've chosen to make the theme parameter become optional rather than gone either.

Surely this problem would have been better solved by multiple overloads of the function typing rather than introducing another generic argument? Or even just living with existing return typing since you've barely changed it anyway.

[rosskevin]

Check the latest guide for explanation: https://material-ui.com/guides/typescript/

You will need to add the createStyles function.

[eps1lon]

@rosskevin I don't think that's the issue here.

Typescript has some issue with function parameter types inside generics (don't quote me on that; it's the same issue with generic props and event handlers).

Basically you need to explicitly annotate the function parameter with Theme. But I think it might be worth investigating using overload instead of a generic argument. The reason for the change is somewhat explained in #12673 (comment) Edit: That's a newer change.

[gordonmleigh]

@rosskevin createStyles doesn't help here. As your link explains, it prevents type widening. The sample code I provided works in package versions before this pull request was merged, which is how I managed to narrow it down to this pull request.

Further testing I did confirms that the type inference of the first parameter fails if there are two generic arguments—I expect this to be something to do with the second generic argument being related to the first (via extends WithStylesOptions<ClassKey>).

[eps1lon]

Did some quick testing with overloads on not only does this add quite a bit of duplicate code it also breaks the patterns we document and test against.

@gordonmleigh I would recommend you explicitly type your function parameters. This might be also required in the future when we make our Props generic which again will cause typescript to fail because it can't infer function paremters in generic types.

[pelotom]

I would love to someday have a withStyles which is typed perfectly and does everything we want it to do without needing superfluous type annotations... unfortunately every time we push something onto one end of the shelf something seems to fall off the other end, and we've gone through many iterations of this. PRs always welcome!

[blasterpistol]

Doubtful compromise between ease of code writing and type safety, we haven't experienced any problems with the previous solution. This solution prevents our codebase (and I think others) from upgrading to next version of the library without large rewritings.

[eps1lon]

@DenisNeustroev What pattern did you previously use?

[rosskevin]

@DenisNeustroev For me it was opt-in to omit the *ClassKey, not a forced rewrite. I indeed still have some code doing it the older way that I have yet to touch.

Note that there was a point release with a type breaking change in the withStyles parameterized type options which was quickly solved and another point release. But if you are using the current release I see no reason why you would think a style rewrite is in order.

[vmajsuk]

@eps1lon in 3.0.0 withStyles return type was a function that could be parameterized explicitly with your components props, that's how we used it:

const styles = withStyles({ root: { background: 'white' } })

interface Props {
    text: string
}

export const WhiteText = styles<Props>(({ classes, text }) => (
    <span className={classes.root}>{text}</span> 
))

I mean I don't think that it's hugely better, just a different code style. Just a little bit upset that we need to update the entire existing codebase.

I'm also a bit concerned about new types, I think they are overcomplicated. E. g. I can't understand, why this code gives an error:

import { withStyles as mWithStyles } from '@material-ui/core/styles'
export function withStyles<ClassKey extends string>(
  style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
  options?: { name: string },
) {
  return function styles<Props extends {}>(
    component: React.ComponentType<Props & WithStyles<typeof style>>,
  ) {
    return mWithStyles(style, options)(component)
  }
}

I also feel like types could be much simpler if PropInjector had a type parameter of component's own props

[eps1lon]

We need the component to enable defaultProps. Otherwise this information is lost.

I don't understand what you are trying to do with your withStyles implementation.

const styles = withStyles({ root: { background: 'white' } })

interface Props {
    text: string
}

export const WhiteText = styles<Props>(({ classes, text }) => (
    <span className={classes.root}>{text}</span> 
))

I understand but again we had to change this for defaultProps. It doesn't do anything different than

const styles = { root: { background: 'white' } }

interface Props extends WithStyles<typeof styles> {
    text: string
}

export const WhiteText = withStyles(styles)(({ classes, text }: Props) => (
    <span className={classes.root}>{text}</span> 
))

It's the same pattern we use in our codebase and throughout the docs.

I think that you don't have to expect any major shifts anymore. When you add typings to an existing codebase you have to expect changes especially since the overall typescript support for react was very immature. If you simply can't migrate I recommend you pull out the typings of withStyles from 3.0 and configure your tsconfig to use those e.g.

"paths": {
  "@material-ui/core/styles/withStyles": ["path/to/3.0/styles/withStyles"]
}

but again you're disregarding a lot of edge cases in your implementation.

[vmajsuk]

@eps1lon sure we'll migrate, I just gave you an example of how we had used it before since you asked & shared my opinion regarding new typings

EDIT: Also, there is one meaningful difference between our code style and yours.
We could specify options like this:

const styles = withStyles({}, { name: 'MyComp' })
const MyComp = styles<Props>(() => <div />)

With the approach from material docs you need to either

const MyCompBase = (props: Props) => <div />
export const MyComp = withStyles({}, {{ name: 'MyComp' })(MyCompBase)

or

// next line will most likely be turned into 4 by Prettier
export const MyComp = withStyles({}, {name: 'MyComp' })((props: Props) => <div />)

Both being too verbose IMO

[nmchaves]

I agree with @vmajsuk that this change makes it a bit painful for some teams to upgrade past MUI 3.0. We have many components (at least a couple hundred) using the decorate<Props> approach that the docs used to recommend. But I also understand that the withStyles typing has required a lot of thought/effort, and I do think it's worthwhile to upgrade.

@eps1lon I think your suggestion to use path mapping is a great idea. However, we import directly from @material-ui/core and rely on tree-shaking.

import { withStyles } from "@material-ui/core";

So I don't think path mapping will work in our case. Instead, we ended up making a custom declaration file:

// src/typings/mui.d.ts

import {
  WithTheme,
  Theme,
  ConsistentWith,
  Overwrite,
  Omit
} from "@material-ui/core";
import * as React from "react";
import * as CSS from "csstype";
import * as JSS from "jss";

declare module "@material-ui/core" {
  // AnyComponent is no longer part of MUI's own typings, so add it manually
  type AnyComponent<P = any> =
    | (new (props: P) => React.Component)
    | ((
        props: P & { children?: React.ReactNode }
      ) => React.ReactElement<any> | null);

  // ... copy all of the `withStyles` typings from v3.0 here (omitted for brevity)

  function withStyles<
    ClassKey extends string,
    Options extends WithStylesOptions<ClassKey> = {}
  >(
    style: StyleRulesCallback<ClassKey> | StyleRules<ClassKey>,
    options?: Options
  ): <
    P extends ConsistentWith<
      P,
      StyledComponentProps<ClassKey> & Partial<WithTheme>
    >
  >(
    component: AnyComponent<P & WithStyles<ClassKey, Options["withTheme"]>>
  ) => React.ComponentType<
    Overwrite<Omit<P, "theme">, StyledComponentProps<ClassKey>>
  >;
}

Just figured I'd share in case anyone else is in our position and would like to find a gradual upgrade path.

[vmajsuk]

@nmchaves for now we use the following hack:

import {  withStyles as mWithStyles } from '@material-ui/core/styles'

export function withStyles<ClassKey extends string>(
  style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
  options?: { name: string },
) {
  return function styles<Props extends {}>(
    component: React.ComponentType<Props & WithStyles<typeof style>>,
  ) {
    // @ts-ignore
    const componentWithStyles = mWithStyles(style, options)(component)
    return componentWithStyles as React.ComponentType<
      Props & StyledComponentProps<ClassKey>
    >
  }
}

It kind of should work without ts-ignore, but ts throws strange error, maybe it will work fine with future versions of withStyles typings

[eps1lon]

@vmajsuk Do you have strictNullChecks enabled in your tsconfig?

[vmajsuk]

@eps1lon Yep