Material-ui: [TypeScript] withStyles still cannot infer generic type from its parameter

[franklixuefei]

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

Expected Behavior

No error should be reported.

Current Behavior

Context

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | beta.44 |
| React | 16.2.0 |

[franklixuefei]

@estaub @pelotom I thought #11003 fixed this...

[pelotom]

@franklixuefei could you please provide a text code sample instead of screenshots?

[franklixuefei]

@pelotom Sure. Here you go.

SomeComponent.tsx

import withStyles, { WithStyles } from 'material-ui/styles/withStyles';
import * as React from 'react';

const decorate = withStyles<classList>((theme) => ({
  main: {
    ...
  }
}));

type classList =
  | 'main';

export interface IProps {
  someProp?: string;
}

class SomeComponent extends React.PureComponent<IProps & WithStyles<classList>> {

  render() {
    ...
  }
}

export default decorate(SomeComponent); // note that I don't specify a generic type here

usage:

import SomeComponent from './SomeComponent.tsx';
...
<SomeComponent someProp="hello world" /> // this reports error, as can be seen in the screenshot below.
...

[pelotom]

Hm, yet another edge case. If all of your non-style props are optional, the first withStyles overload is used, whereas the second should be used.

[estaub]

Note that this one fails with strictFunctionTypes both set and unset.

[pelotom]

@estaub it may be time to consider rolling back part or all of that PR. Unfortunately the jadedness of my original review comment is proving to have been warranted.

[estaub]

@pelotom
I agree.

[estaub]

@pelotom I should mention that there's another possible option: drop support for top-level unions in props, and go back to an Omit-based implementation. All of the problems we're having revolve around overload matching; the Omit-based implementation has no overloading. I'm guessing, though, that union support is a hard requirement. For myself, I'd give up unions in a heartbeat for generic inferencing that worked reliably, but that matters little.

For anyone reading wondering exactly what I mean by top-level union props, consider:

interface A { d: 'A', a: string }
interface B { d: 'B', b: string }
type Props = A | B

The difficulty here is that:
type PropKeys = keyof Props
has a value of 'd', not 'a' | 'b' | 'd' as one might expect. This breaks any HOC declaration relying on keyof, including.

[olee]

Same problem here - updated material-ui and suddenly lots of broken styled components where the props could not be inferred any more.
This also happens for components where there are non optional props.

[estaub]

@olee To make sure that we capture all of the failing cases, to improve tests so that another attempt is well-informed about what people are using, can you provide example(s) where they aren't clearly of the same ilk?

[estaub]

@pelotom Do I need to do something to initiate a rollback? Or do you want to look at the options without union support?

[olee]

This sample generates an error in the last line.

import * as React from 'react';

import { WithStyles, Theme, StyleRules, StyleRulesCallback, StyledComponentProps } from 'material-ui/styles';
import { WithStylesOptions } from 'material-ui/styles/withStyles';

import { $Keys, $call } from 'utility-types';
import withStyles from 'material-ui/styles/withStyles';

interface Props {
    testProp: string;
    testPropOptional?: string;
}

type StyleKeys = 'root';
const cssStyles: StyleRules<StyleKeys> = {
    root: {
        color: 'red',
    } as React.CSSProperties,
};

type FullProps = Props & WithStyles<StyleKeys>;

export class TestComponentUnstyled extends React.PureComponent<FullProps> {
    public render() {
        return <div>Hello world!</div>;
    }
}

const TestComponent = withStyles(cssStyles)(TestComponentUnstyled);
export default TestComponent;

var thisCrashes = <TestComponent testProp="test" testPropOptional="test" />;

If I exchange the last few lines with this code however (which is pretty much how withStyles was defined previously I think) it works:

type ConsistentWith<O> = Partial<O> & Record<string, any>;
declare function WithStylesFixed<ClassKey extends string>(
    style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
    options?: WithStylesOptions,
): {
    <P extends ConsistentWith<StyledComponentProps<ClassKey>>>
    (component: React.ComponentType<P & WithStyles<ClassKey>>):
        React.ComponentType<P & StyledComponentProps<ClassKey>>;
};
const withStylesFixed: typeof WithStylesFixed = withStyles;

const TestComponent = withStylesFixed(cssStyles)(TestComponentUnstyled);

[olee]

Ok I just noticed, that just changing the order of the overloaded signatures seems to fix the issue:

declare function WithStylesFixed<ClassKey extends string>(
    style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
    options?: WithStylesOptions,
): {
    // First with generic
    <P extends ConsistentWith<StyledComponentProps<ClassKey>>>
    (component: React.ComponentType<P & WithStyles<ClassKey>>): React.ComponentType<P & StyledComponentProps<ClassKey>>;
    // Second without generic
    (component: React.ComponentType<WithStyles<ClassKey>>): React.ComponentType<StyledComponentProps<ClassKey>>;
};

[estaub]

@olee Thanks for the example. Flipping the order makes other cases fail. I can't find the details now, but I'm sure I tried it.

[olee]

Well - as far as I know, the change that happened with the last version which added that non-generic overload surely broke more code that it could ever help.
So I think it should definitely be reverted for now and if there's a component without props, we just have to continue specifying <{}> .

For now I'm using this file which I import instead of 'material-ui/styles' to work around this issue - I hope this helps others as well:

import withStylesOld from 'material-ui/styles/withStyles';

export * from 'material-ui/styles';

import { WithStyles, StyleRules, StyleRulesCallback, StyledComponentProps } from 'material-ui/styles';
import { WithStylesOptions } from 'material-ui/styles/withStyles';

type ConsistentWith<O> = Partial<O> & Record<string, any>;
declare function WithStylesFixed<ClassKey extends string>(
    style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
    options?: WithStylesOptions,
): {
    <P extends ConsistentWith<StyledComponentProps<ClassKey>>>
    (component: React.ComponentType<P & WithStyles<ClassKey>>): React.ComponentType<P & StyledComponentProps<ClassKey>>;
    (component: React.ComponentType<WithStyles<ClassKey>>): React.ComponentType<StyledComponentProps<ClassKey>>;
};

export const withStyles =  withStylesOld as typeof WithStylesFixed;

[pelotom]

I have a solution which solves all known edge cases, but requires TypeScript 2.8. See #11280, and feel free to comment there. I'd like to get a sense from the users whether 2.8 is an acceptable baseline.

[olee]

@pelotom
I definitely think that requiring an up-to-date typescript version is an acceptable requirement.
I mean - material-ui itself is a kinda bleeding-edge library and when working with react in general I'd always say you should use latest typescript.

TLDR: definitely YES

[pelotom]

@olee I'm of the same mind, especially since TS 2.9 is scheduled to land this week.

[estaub]

👍 2.8. Beta users of material-ui are in large part self-selecting for early adopters. And once 1.0 releases, for any migrants from 0.* the intrinsic mui changes will dwarf any Typescript version issues.