Material-ui: Question regarding latest (beta.43) WithStyles update.

[emreeren]

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

Expected Behavior

Sample code to compile.

Current Behavior

does not compile with that error

Type '{ caption: string; }' has no properties in common with type 'IntrinsicAttributes & StyledComponentProps<"content"> & { children?: ReactNode; }'.

Steps to Reproduce (for bugs)

1. Try to compile the sample code

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

interface IStyle {
  content: any;
}

interface IComponentProps {
  caption: string;
}

type ComponentProps = IComponentProps & WithStyles<'content'>;

const decorate = withStyles((theme): IStyle => ({
  content: {
    margin: 4
  }
}));

const Component = (props: ComponentProps) => {
  return <div className={props.classes.content}>Hello {props.caption}</div>
}

const StyledComponent = decorate(Component);

class App extends React.Component {
  public render() {
    return (
      <div className="App">
        <StyledComponent caption="Developer" />
      </div>
    );
  }
}

export default App;

Context

The sample code was compiling fine before beta.43 update. Now I need to change

const StyledComponent = decorate(Component);

line as

const StyledComponent = decorate<IComponentProps>(Component);

to make it compile without errors.

I'm not sure if it is a bug or an intentional change so I wanted to ask before attempting to change my all styled components. I think this probably relates with https://github.com/mui-org/material-ui/pull/11003

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | v1.0.0-beta.43 |
| React | 16.3.2 |
| browser | Chrome |
| typescript | 2.8.3 |

[estaub]

@emreeren No, it was very much not intentional!

@pelotom Reversing the order of the alternative function types returned by withStyles seems to fix this particular case. Do you recall if you had a reason (test case) for the original order?

export default function withStyles<ClassKey extends string>(
  style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
  options?: WithStylesOptions,
): {
    <P extends ConsistentWith<WithStyles<ClassKey>>>(
        component: React.ComponentType<P & WithStyles<ClassKey>>,
    ): React.ComponentType<P & StyledComponentProps<ClassKey>>;
  (component: React.ComponentType<WithStyles<ClassKey>>): React.ComponentType<
    StyledComponentProps<ClassKey>
  >;
};

[pelotom]

Nope, reversing the order seems like the right move as long as it satisfies the rest of our test cases. We should add this example as a test case too.

[estaub]

@emreeren The bug does not occur if

`"strictFunctionTypes": true`

is set in tsconfig.json.

@pelotom Swapping the order of the function-overload alternatives now fails where one might suspect: the no-props case. I've no idea of how to fix the reversed case.

It seems over-presumptive to me, but I need to ask: _is it reasonable to require strictFunctionTypes be set_? Reading the doc, it's very clear how it makes the released implementation work. It's been out since 2.6, so I'd expect that most published declarations are good with it.

[pelotom]

I think it's reasonable to expect that people use strictFunctionTypes if they want an optimal experience.

[emreeren]

Sounds good. Unrelated but at my first attempt I've got an issue because of this. https://github.com/ReactiveX/rxjs/issues/3031

[estaub]

@ljvanschie Does changing to
"strictFunctionTypes": true
work for you?

[ljvanschie]

@estaub I've tried the setting, but it caused some other problems with stateless functional components that use withStyles. We ended up just adding the generic props parameters to the decorates.

[pelotom]

@ljvanschie if it caused problems, you should probably pay attention to them... it’s helping you find bugs!

[geirsagberg]

I ran into the same problem as OP, enabling strictFunctionTypes made the error go away. This should probably be documented though.

[geirsagberg]

Actually, enabling strictFunctionTypes caused a whole bunch of problems for other places in our code. Possibly a symptom we should look into, but is it really necessary for Material UI to require everyone to use strictFunctionTypes?

[pelotom]

I looked into this some more and I don't think this has anything to do with strictFunctionTypes... you can fix it by just making your classes prop optional:

type ComponentProps = IComponentProps & Partial<WithStyles<'content'>>;

Since ComponentProps represents the "outward-facing" view of your component, i.e. what a user of the component deals with, classes is optional for them.

[pelotom]

Actually, it should really be

type ComponentProps = IComponentProps & StyledComponentProps<'content'>;

but the typings are subtly wrong to make that not work... I will fix that.

[geirsagberg]

The point is that the inner classes prop should be hidden for the consumer, shouldn't it? For example, many MUI components have a separate outward facing classes prop used to override the inner CSS styles, and only the specified keys should be allowed, e.g. root for Card

[pelotom]

@geirsagberg I'm not sure what you mean... classes is visible both within the component and to a user of the decorated component; in the former case it's guaranteed to be there and have all class names set, in the latter it's optional, and all properties within it are optional (because it represents overrides).

[geirsagberg]

Nevermind, I think I misunderstood.

So the classes that are exposed from e.g. Card, are actually the same ClassKeys as are used internally by Card.

So if you set <Card classes={{root: 'myClass'}} />, you override the root class inside the Card component (from https://github.com/mui-org/material-ui/blob/v1-beta/packages/material-ui/src/Card/Card.js):

export const styles = {
  root: {
    overflow: 'hidden', // this will be overridden?
  },
};

[pelotom]

Yep, that's the idea!

[pelotom]

Hm, I misspoke though, you can't fix it by just saying

type ComponentProps = IComponentProps & Partial<WithStyles<'content'>>;

and strictFunctionTypes does make this go away. However you can also fix it by rewriting as

interface IComponentProps {
  caption: string;
}

const decorate = withStyles<'content'>(theme => ({
  content: {
    margin: 4
  }
}));

const StyledComponent = decorate<IComponentProps>(props =>
  <div className={props.classes.content}>Hello {props.caption}</div>
);

<StyledComponent caption="Developer" />

[geirsagberg]

Wasn't the point of https://github.com/mui-org/material-ui/pull/11003 to avoid the necessity of an extra generic parameter though? Or what were the other benefits?

[pelotom]

Wasn't the point of https://github.com/mui-org/material-ui/pull/11003 to avoid the necessity of an extra generic parameter though? Or what were the other benefits?

Yes, that was the point of it... however it appears that benefit was predicated on using strictFunctionTypes, for reasons I don't understand. It's definitely a gotcha that should be documented.

[estaub]

Ok, let me explain a bit.

The reason it fails without strictFunctionTypes in the theme case is because functions are bivariantly equivalent in that case, and this affects whether the first alternative in the withStyles overload pair is deemed valid.

It reads

(component: React.ComponentType<WithStyles<ClassKey>>): React.ComponentType<
    StyledComponentProps<ClassKey>
  >;

and question is whether the parameter declaration in usage

```ts
(props: IComponentProps & WithStyles<'content'>) => {
return

...

}

matches the withStyles declaration's parameter
```ts
component: React.ComponentType<WithStyles<ClassKey>>

With bivariance (without strictFunctionTypes) Typescript asks the wrong question: it asks:

Does IComponentProps & WithStyles<'content'> extend WithStyles?

which of course is true, but not the right question. With contravariance (with strictFunctionTypes), the opposite question is asked:

Does WithStyles extend IComponentProps & WithStyles<'content'> ?

which is false, and what we want.

This may seem backward, until you think about it in the context of being a function argument, which is exactly what strictFunctionTypes is about. See https://www.stephanboyer.com/post/132/what-are-covariance-and-contravariance for a good writeup. Abbreviated, the question is:

If Dog extends Animal and I have a function f(dog:Dog), should it accept any old Animal?

With strictFunctionTypes, the answer is "No, we only do Dogs (or any subtype)"; without it, the answer is "Sure, it might work".

[pelotom]

@estaub nice analysis, that makes sense. So I think the question is, is there a way we can disqualify the first alternative even when "strictFunctionTypes": false, in such a way that it doesn't mess up any other existing use cases? It seems difficult, and we also don't have a way currently to test under more than one set of compiler flags.

[pelotom]

This may also be something that could be handled via conditional types instead of overloads, but I’m pretty sure requiring TS 2.8 as a baseline would be more disruptive than requiring strictFunctionTypes / extra type annotations.

[geirsagberg]

@estaub There is a problem with a static stylesheet as well:

This sample works only if I change L25 to const MyComponent = withStyles(styles)<OwnProps>(InnerComponent), or if I roll back to beta 42. Or if I enable strictFunctionTypes.

[estaub]

@pelotom I've thought on it a lot without getting any traction, but I'm no Typescript maven. I very much identify with developers tired of definition upgrades breaking their code all the time. (Witness my rantiness about earlier csstype-related issues.) But we're hardly alone; redux just did a big redefinition churn.

Flipping the two overload alternatives might work if there was some way to restrict P to cases where
P - WithStyles<ClassKey> (not real code)
is non-empty.

If you recall, I found an apparently solid implementation that got tripped up by scenarios where the inner props are a discriminated union. FWIW, I have deep misgivings about the continued viability of a union as a top-level prop definition; I'm pretty sure at least some other HOC implementations (e.g. react-router's) run afoul of them also. So I'd trade them away quickly; if someone wants a discriminated union it has to be wrapped in an object. Obviously, everyone's mileage will differ; I suspect the union-based test case and docs arose from some real usage.

Even when it's viable to require 2.8, I don't believe 2.8's conditional types will help with this, again, as long as support of a top-level union prop definition is required. This is based on experimentation.

[pelotom]

@estaub

But we're hardly alone; redux just did a big redefinition churn.

You’ll be happy to learn I was responsible for that as well 😜 https://github.com/reactjs/redux/pull/2563

[estaub]

@geirsagberg Yes, thanks, I see it now; my thinking that this is dependent on the theme=> was muddled. I've edited it out of the earlier comment to avoid confusion for other readers. That probably doubles the number of people affected.

[estaub]

The workaround

const StyledComponent = decorate<IComponentProps>(Component);

gives me qualms, because it requires specification of the wrong generic; when inference is working properly, the inferred parameter is ComponentProps, not IComponentProps. I fear that this workaround _may_ break with an improved declaration. Of course, if the fix is to remove the generic parameter altogether, folks won't be too upset.

---

Here are a couple of constructs, either of which, if it existed, would allow a declaration that supports strictFunctionTypes=false.

• a type that excludes {}, but is extended by all other interfaces and interface unions. I think we could reverse the overload order with that.
• an Omit/Exclude implementation that works when the remainder is a union.

Update
Sorry, I think the first option "a type that excludes {}" ends up still needing a working Omit/Exclude.

[pelotom]

it requires specification of the wrong generic

It's not wrong, it's minimal by design. You shouldn't be required to specify redundant information, i.e. the type of classes, which is already determined by earlier generics, if you don't want to. This comes in handy when you're just trying to make a quick and dirty component and don't need to reference the name of the props type elsewhere:

const StyledComponent = decorate<{ content: string }>(({ content, classes })=> (
  // `content` and `classes` are inferred correctly
  <div className={classes.root}>{content}</div>
));

[estaub]

FWIW, the problems with support of unions in other implementations arises out of the definition of keyof, which is the intersection of the keys of a union, not their union. It's very analogous to strictFunctionTypes.

interface A { d: 'A', a: string }
interface B { d: 'B', b: string }

type Key = keyof (A | B)
const key: Key = 'a'  //  TS2322: Type '"a"' is not assignable to type '"d"'.

Discussion: https://github.com/Microsoft/TypeScript/issues/12948 et al.

[emreeren]

Thank you very much for the detailed explanation of the case. However I think old type declaration should just work fine without need of an alternate declaration.

<P>(component: React.ComponentType<P & WithStyles<ClassKey>>): React.ComponentType<P & StyledComponentProps<ClassKey>>;

On the test case P resolves to IComponentProps and that's great. Similarly shouldn't it resolve to just {} on NoProps case? If I'm understanding it correctly what we're dealing with is probably is a TS bug.

[estaub]

@emreeren
The old docs at the left top of https://github.com/mui-org/material-ui/pull/11003/files demonstrate a couple of the cases where generic inferencing failed with the old definition.

If I'm understanding it correctly what we're dealing with is probably is a TS bug.

I'd call it a deficiency that was fixed by strictFunctionTypes. But the fix obviously creates flags lots of typing "errors" (or perhaps "laxness") that folks may not have time or inclination to clean up, hence the default of strictFunctionTypes=false.

Don't take either of the above as an opinion about what we should do going forward. I'm still churning, trying to find something that works well in all cases, including without strictFunctionTypes. My only prospect at the moment is 2.8-dependent, and not correct yet. I'm not sure what we'd do with it even if&when it works, given the version dependency; maybe we can figure out a way to offer the two alternatives in a clean way. What TS version are you using?

[emreeren]

I'm using 2.8.3 and please let me know if you have something to try.

[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.

[kevinhughes27]

I'm still running into this issue on the latest release and typescript 2.8.3. I'm super new to typescript so I'm probably missing something - is there a way the code needs to be written to work with the merged fix?

I'm using this method to define props from the docs:

interface Props extends WithStyles<typeof styles> {
   ....
}

[pelotom]

@kevinhughes27 can you provide a complete self-contained example?

[kevinhughes27]

Sure:

import * as React from 'react';
import * as ReactDOM from 'react-dom';

import AppBar from '@material-ui/core/AppBar';
import Toolbar from '@material-ui/core/Toolbar';
import Typography from '@material-ui/core/Typography';
import IconButton from '@material-ui/core/IconButton';
import MenuIcon from '@material-ui/icons/Menu';

import { withStyles, WithStyles } from '@material-ui/core/styles';

const styles = {
  flex: {
    flex: 1,
  },
  menuButton: {
    marginLeft: -12,
    marginRight: 20,
  },
};

interface Props extends WithStyles<typeof styles> {
  openNav: (event: React.SyntheticEvent<{}>) => void,
}

class TopBar extends React.Component<Props> {
  public render () {
    const { classes } = this.props;

    return (
      <AppBar position="static">
        <Toolbar>
          <IconButton className={classes.menuButton} color="inherit" aria-label="Menu">
            <MenuIcon />
          </IconButton>
          <Typography variant="title" color="inherit" className={classes.flex}>
            Title
          </Typography>
        </Toolbar>
      </AppBar>
    );
  }
}

const StyledTopBar = withStyles(styles)<{}>(TopBar);

class App extends React.Component {
  public render () {
    return (
      <div>
        <StyledTopBar openNav={() => { return }} />
      </div>
    );
  }
}

ReactDOM.render(
  <App />,
  document.getElementById('root') as HTMLElement
);

fails with:

Type '{ openNav: () => void; }' has no properties in common with type 'IntrinsicAttributes & StyledComponentProps<"flex" | "menuButton"> & { children?: ReactNode; }'.

[pelotom]

@kevinhughes27 try removing the <{}>:

-const StyledTopBar = withStyles(styles)<{}>(TopBar);
+const StyledTopBar = withStyles(styles)(TopBar);

this annotation used to be required to make type inference work out, but is no longer necessary, and in fact creates spurious errors.

[kevinhughes27]

That did it!

The example app still has the old syntax. I can submit a PR later. I think a couple of other things in the example are outdated as well.