mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

Typescript type error in component returned by withStyles() #8447

Closed cfilipov closed 6 years ago

cfilipov commented 6 years ago

When using withStyles() hoc in typescript, I am getting the following error when trying to use the returned component:

Type '{}' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<App> & Readonly<{ children?: ReactNode; }> & Reado...'.
  Type '{}' is not assignable to type 'Readonly<WithStyles<"main">>'.
    Property 'classes' is missing in type '{}'.

It appears this change to the type definition might be related to this issue.

Expected Behavior

Given the App component code below, I should be able to use the component <App /> without the type error as I did in 1.0.0-beta.10.

Current Behavior

Given the App component code below, trying to use <App /> results in the aforementioned error.

The Code

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

const styles = {
    main: {
        marginTop: 48,
        padding: 10,
    },
    foo: {
        margin: 0,
    },
};

interface Props {
    message: string;
};

type ClassNames = { classes: { [className in keyof typeof styles]: string } };

class App extends React.Component<Props & ClassNames> {
    render() {
        const { classes, message } = this.props;
        return (
            <div className={classes.main}>
                <div className={classes.foo} >
                    Hello World! {message}
                </div>
            </div>
        );
    }
}

export default withStyles(styles)(App);

Context

The code worked fine in 1.0.0-beta.10, when I upgraded to 1.0.0-beta.12 I got the type error. In the code snippet provided I used the `keyof typeof styles` trick so that I would not need to define a list of class names twice (I strongly dislike the repetitiveness). I have also tried other variations: ```typescript type ClassNames = WithStyles; ``` and doing it the more common way (as seen in [styles.spec.tsx](https://github.com/sebald/material-ui/blob/db1a42102097344c2614a9163cd9a0125bfd655a/test/typescript/styles.spec.tsx#L58-L61)): ```typescript type ComponentClassNames = 'main' | 'foo'; type ClassNames = WithStyles; ``` I still get the same error. It seems the previous type definition would return a component whose props type would be `StyledComponentProps` which has an optional `classes` property. The new definition... ```typescript >>( component: C ): C; ``` ...returns the same type `C` as the component, this means that passing `ClassNames` which is not marked optional propagates to the returned component. I see mentioned [here](https://github.com/callemall/material-ui/pull/8399) the use of `Partial<>` which I think it an unsightly hack. ## Your Environment
Tech Version
Material-UI 1.0.0-beta.12
React 15.6.1
browser Chrome 61.0.3163.100
pelotom commented 6 years ago

@cfilipov when decorating a component class you should use StyledComponentProps instead of WithStyles, and if in strict type checking mode you need to use the non-null assertion operator ! to extract the fields of classes. This is a compromise to allow the use of withStyles as a class decorator. Class decorators in TypeScript suffer from the limitation that their return type must match the argument type. Only one option given this limitations is possible:

  1. don't support using withStyles as a class decorator
  2. require that you pass a dummy classes prop when constructing an element of the decorated component type (the previous compromise, which was arguably more annoying)
  3. require that classes be considered nullable inside the component, forcing you to use the ! operator when accessing its fields (the current compromise)

I would highly recommend that if your component has no state you use a stateless functional component, which will require fewer type annotations and be more type safe:

export default withStyles(styles)<Props>(({ classes, message }) => (
  <div className={classes.main}>
    <div className={classes.foo}>
      Hello World! {message}
    </div>
  </div>
));
pelotom commented 6 years ago

I see mentioned here the use of Partial<> which I think it an unsightly hack.

If you read my followup to that comment you'll see that Partial<> is not required.

cfilipov commented 6 years ago

If you read my followup to that comment you'll see that Partial<> is not required.

I saw your comment regarding Partial<>. Using StyledComponentProps essentially amounts to the same thing (it's using Partial<> in that definition). My gripe with that is now class names must be accessed with ! (as you mentioned). I think passing a dummy classes prop or requiring the use of ! are both poor compromises.

To clarify, I guess the root issue I am having here is the choice to introduce a regression to the hoc in an effort to work around a limitation of an experimental feature of typescript. I admit to some bias since I don't care for decorators, whereas others clearly do, but the tradeoff here doesn't make sense to me.

pelotom commented 6 years ago

@cfilipov my first refactor of the withStyles typings was to choose option (1), i.e. make the typings completely correct for both class and function components, at the expense of using withStyles as a class decorator. I then got feedback that using the decorator syntax was a popular request, so I switched to option (3). I'm happy to revisit that decision; I too would prefer type safety to supporting decorators in their current semifunctional state.

cfilipov commented 6 years ago

Yeah, I understand the desire to support a popular request like this. I want to use decorators too, but I keep running into many issues with them every time I do (outside of mui) so I personally decided to not use them until they are ready.

It sounds like you share my concern and I don't have much more to add so hopefully other people can provide feedback in this direction to help influence it.

marcusjwhelan commented 6 years ago

I switched to beta.13 from beta.10 to see if anything had changed and Yes this is a real issue. To throw my 2 cents in here, for me, decorators are experimental. They obviously could be changed in the future. And until then I would fully support the 100% accurate way. I would much rather have coherent type safety to hacking my types to make things work.

pelotom commented 6 years ago

8550 looks like further evidence that people are confused by this, and we should consider not supporting @withStyles() as a decorator (in TypeScript).

pelotom commented 6 years ago

This is what it would look like to "decorate" a class if we made the typings correct:

type NonStyleProps = {
  text: string
};

const styles = {
  root: {
    backgroundColor: 'red'
  }
};

const DecoratedComponent = withStyles(styles)(
  class extends React.Component<NonStyleProps & WithStyles<'root'>> {
    render() {
      return (
        <div className={this.props.classes.root}>
          {this.props.text}
        </div>
      );
    }
  }
);
marcusjwhelan commented 6 years ago

@pelotom I have yet to move forward from beta.10 because of this issue. I made comment about the styling of a class with Redux's connect method before. I think its relatively easy and robust. in #8059 the third comment is myself with the types.

wcandillon commented 6 years ago

@pelotom Thank so much for educating us on this issue. I am a big user of TS decorators but in that instance, I would be happy for withStyles to drop its decorating support in order to have better type safety.

oliviertassinari commented 6 years ago

we should consider not supporting @withStyles() as a decorator (in TypeScript).

@pelotom I'm personally in favor of this change. @sebald What do you want to do here?

pelotom commented 6 years ago

It's a simple change; I went ahead and opened a PR in case you want to go with it 🙂

marcusjwhelan commented 6 years ago

@pelotom would the typings still work if I used them like this?

interface IStyles {
    // styles interface
}
interface IHeaderInfoStyles {
    classes: any;
}
interface IHeaderInfoProps {
    // properties resulting from dispatches
}
interface IHeaderInfoInjectedProps {
   // props injected from parent if any
}
interface IHeaderInfoDispatchProps {
    // dispatches
}
interface IHeaderInfoState {
    // your state for the class
}

type HeaderInfoProps = IHeaderInfoProps & IHeaderInfoInjectedProps & IHeaderInfoDispatchProps & IHeaderInfoStyles;

class HeaderInfo extends Component<HeaderInfoProps, IHeaderInfoState> {

export const HeaderInfo_container = withStyles<IHeaderInfoInjectedProps, IStyles>(styles)(
    connect<IHeaderInfoProps, IHeaderInfoDispatchProps, (IHeaderInfoInjectedProps & IHeaderInfoStyles)>(mapStateToProps, mapDispatchToProps)(HeaderInfo),
);
pelotom commented 6 years ago

@marcusjwhelan withStyles no longer takes 2 type parameters, and you shouldn't have to provide a type parameter for styles (it can be inferred from styles). You should instead be able to write

withStyles(styles)<NonStyleProps>(...);

If you give the types for mapStateToProps and mapDispatchToProps I might be able to show you what this would look like for your example.

sebald commented 6 years ago

tl;dr; Let's do the changes and see what the backlash is, I guess 👷🏻


@oliviertassinari @pelotom ¯_(ツ)_/¯ I do not use decorators, so personally, I really don't care about this change, because I am not affected. But it seems like a lot of people do care about this "feature". That's why we added it in the first place. That's IMHO what has priority here.

I am very happy with the changes of withStyles, but when you take a look at other more functional libraries, like ramda or recompose, the typings aren't that strict, neither are they super type-safe. Often times you have to pass in a generic, which represents the return value of a function. Not pretty but it will work for a 99.9% of the users.

Why can't we bring back the old typings that worked for the people using decorators? Because it did have two generics, we may be able to have both typings side by side.

Also, I am a bit confused what peoples issues with decorators are (regarding "not working"). Libs like Angular and https://nestjs.com/ make heavy use of them after all. In our case, people could just add WithStyles to the props and are fine.

pelotom commented 6 years ago

@sebald

Why can't we bring back the old typings that worked for the people using decorators? Because it did have two generics, we may be able to have both typings side by side.

I'm not sure what you mean. There was never any typing that allowed people to use decorators painlessly. The first implementation suffered from #8267, which was that you couldn't construct an element of a decorated component without passing a dummy classes prop, e.g. <StyledComp classes={{} as any} />. The second (current) implementation suffers from the converse problem, that classes is seen as potentially undefined within the component class. If you want to use TypeScript decorators in their current form, those are your only 2 options; pick your poison.

The third way is using the correct type, so that classes is defined within the class but not required to be passed as a prop. To have both those conditions, you need to give up decorators.

sebald commented 6 years ago

@pelotom Yes, sorry. You're right. Really is not my day... 🤐 So lets merge!

pelotom commented 6 years ago

@sebald

Also, I am a bit confused what peoples issues with decorators are (regarding "not working"). Libs like Angular and https://nestjs.com/ make heavy use of them after all.

I'm not sure how they're used in Angular, but there are certainly use cases where decorators can be used painlessly; basically if the decorator doesn't need to change the type of the class it's decorating, they work fine. That's not the situation we have here; withStyles needs to change the type of the component it decorates.

sebald commented 6 years ago

@pelotom Yes, exactly. It's just the mutation that is bad. Actually, the way TS currently implements decorators might even be good for the Angular users, since the decorators AFAIK set up contracts with the framework, like "register this class as a component" or "add meta data so I can use this in the DI" ... god even writing about this makes me feel 🤢

marcusjwhelan commented 6 years ago

@pelotom the reason I have the types the way I do is it provided Type Safety for my components. Currently the types have no type safety when it comes to components. The injectedProps in my example are the types that are required by the component to work. The types for connect from react-redux need to be that of Props which come from mapStateToProps and DispatchProps which come from the mapDispatchToProps.

At the very end there needs to be the injectedProps so that my parent component knows that I need to inject them or my project will not compile. (this is what I want). Is this change going to migrate back to this kind of typeSafety?

pelotom commented 6 years ago

@marcusjwhelan again, if you can provide a full (but minimal) example that was compiling in beta 10 I can show you how to migrate. AFAIK the new version should be every bit as expressive and more type safe than before.

wcandillon commented 6 years ago

@pelotom Silly question, is there a way I could get notified once a new release is done?

oliviertassinari commented 6 years ago

@wcandillon Follow us on Twitter.

marcusjwhelan commented 6 years ago

@pelotom I did post an example above... Why would you need to see any more than that? You can assume the properties are a,b,c,d,e. The only thing is the injected props need to be emitted as a requirement.

Edit

I figured it out.

iamjem commented 6 years ago

I'm chiming into this discussion a little late, but I've been seeing a similar error as mentioned here, and I've yet to determine how I can correct it. I am using material-ui@1.0.0-beta.16 and typescript@2.5.3.

const LoginForm = withStyles(styles)(
    class extends React.Component<WithStyles<'root' | 'button'>, ILoginFormState> {
    // ...
    }
);

render(
  <MuiThemeProvider theme={theme}>
    <LoginForm />
  </MuiThemeProvider>,
  document.getElementById('login-form')
);

The error message I'm getting in this particular case is:

Type '{}' is not assignable to type 'IntrinsicAttributes & WithStyles<"root" | "button"> & StyledComponentP...'.
  Type '{}' is not assignable to type 'WithStyles<"root" | "button" | "progress" | "textField">'.
    Property 'classes' is missing in type '{}'.
 not assignable to type 'WithStyles<"root" | "button" | "progress" | "textField">'.
    Property 'classes' is missing in type '{}'.
doppio commented 6 years ago

when decorating a component class you should use StyledComponentProps instead of WithStyles

@pelotom - is there an example of this somewhere? I'm having a ton of trouble determining how to style a stateful component class in TypeScript.

pelotom commented 6 years ago

@iamjem it's a little hard to tell since you haven't provided styles, but it looks like the first issue is that you have more class keys mentioned in styles than you're mentioning in WithStyles<...>. I think if you change it to

const LoginForm = withStyles(styles)(
    class extends React.Component<WithStyles<keyof typeof styles>, ILoginFormState> {
    // ...
    }
);

it should take care of the first issue. Then it looks like there is a second issue, which is that the resulting type of LoginForm is

React.ComponentType<WithStyles<"root" | "button"> & StyledComponentProps<"root" | "button">>

which is obviously not correct; it looks like the absence of non-style props is confusing the type system. You can help it out by being explicit about what the non-style props are, by passing {} as a type argument:

const LoginForm = withStyles(styles)<{}>( // <-- note the type argument
    class extends React.Component<WithStyles<keyof typeof styles>, ILoginFormState> {
    // ...
    }
);

Sorry it's so complicated, I wish I knew of an easier way to make this stuff work!

iamjem commented 6 years ago

That did the trick! Thank you for the quick help @pelotom. I've been using React for a long time, but just recently jumped into Material-ui, and thought I'd try out Typescript while I'm at it. Needless to say, I'm finding there's some edge cases where its difficult to tell how to make Typescript happy.

isman-usoh commented 6 years ago

I solved using recompose

example

import { StyleRules, Theme, withStyles } from "material-ui/styles";
import * as React from "react";
import { compose } from "recompose";

interface IProps {
    classes?: any; // <-- add question mark
    text: string;
}

const styles = (theme: Theme): StyleRules => ({
    root: {

    },
});

@compose(
    withStyles(styles),
)
export class MyComponent extends React.Component<IProps> {
    render() {
        const { classes, text } = this.props;
        return (
            <div className={classes.root}>
                {text}
            </div>
        );
    }
}
svachmic commented 6 years ago

I was fighting with this issue (and #8704), with no clear result over the past few days. Then I took the advice from @pelotom:

you should use StyledComponentProps instead of WithStyles

and searched on GitHub for similar ways to solve this. And I found ONE working example 😂. It's a good one, though, and it did solve my problem - separate container and component with TypeScript being a happy cookie: mentioned example here (Note: In my case, the component mapping is in a separate container file, but the idea is the same.).

If anybody thinks this is a bad solution, I'm open to any changes and ideas. RIght now, I'm just glad my code stopped complaining.

lookfirst commented 6 years ago
type Styles = 'foo';
const styles: StyleRulesCallback<Styles> = (theme: Theme) => ({
    foo: {
        position: 'relative',
    }
});

interface Props {
  something: string;
}

class Sidebar extends React.Component<Props & WithStyles<Styles>> {
    render() {
        const { classes, something } = this.props;

        return (
                    <div className={classes.foo}>{something}</div>
        );
    }
}

export default withStyles(styles)<Props>(Sidebar);
otroboe commented 6 years ago

I don't want to create a new issue, but I've tried anything I saw in documentation, example, and passed issues, even with recompose, but I can't make my component working when I add some properties to it. And the resources I found are mostly with older versions of TS, MUI or even React.

Here's my component:

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

const styles: StyleRulesCallback<'positionFixed'> = () => ({
  positionFixed: {
    top: 'auto',
    bottom: 0,
  },
});

const decorate = withStyles(styles);

interface Props {
   someProp: string;
};

export const BottomNav = decorate<Props>(
  class extends React.Component<Props & WithStyles<'positionFixed'>, {}> {
    render() {
      return (
        <AppBar />
      );
    }
  }
);

export default BottomNav;

And the error is:

TS2322: Type '{}' is not assignable to type 'IntrinsicAttributes & Props & StyledComponentProps<"positionFixed"> & { children?: ReactNode; }'.
  Type '{}' is not assignable to type 'Props'.
    Property 'someProp' is missing in type '{}'.

I'm quite a beginner with TS, but I find the documentation page, and the example quite confusing and/or not complete.

If you guys have any idea, thank you ;-)

pelotom commented 6 years ago

@otroboe are you leaving out the code that actually got flagged with the error? Does it look like this?

<BottomNav />

If so, the problem is you need to provide the someProp prop (which is required according to your definition of Props):

<BottomNav someProp="foo" />
otroboe commented 6 years ago

Shame on me... Oh, shame on me. I was so focus on the TS integration, I forgot to look around and take some steps back.

Thanks a lot @pelotom :smile:

lookfirst commented 6 years ago

@otroboe Also remove the string duplication...

type Styles = 'positionFixed';

Wish that one was easier though...

otroboe commented 6 years ago

Yes I did that too, thanks :+1:

nishmeht7 commented 5 years ago

I just faced this same problem, but it turns out that it occurs only if I have initialized the styles object in the same file as my class or function. Alternately, If I am importing the styles from another file, I don't get this error.

Any clue why this happens?

pelotom commented 5 years ago

@nishmeht7 can you post a self-contained snippet?

nishmeht7 commented 5 years ago

@pelotom I just worked on making one, and it works fine in my sandbox environment. I'm currently working on a large app and am still on mui version 1.2.2, while my sandbox env version is the latest one. So I'm guessing once I upgrade versions I won't be able to reproduce my error.

HenrikBechmann commented 5 years ago

I followed the example of a basic form from https://material-ui.com/demos/selects/ but got complaints that theme doesn't have root (using latest typescript and material-ui), and no class was being applied to the form element. I tried following the discussion above, but it seems inconclusive. And indeed the inherited classes list was missing the generated class name for form. If I added the generated classname manually (found in withStyles with console.log(theme) in dev tools inspect elements, all fine, so apparently the class was being generated correctly. Wasn't getting passed through withStyles to the form element though. Quite confusing.

So I've reverted back to styles for now until this gets sorted:

<form style = {{display:'flex',flexWrap:'wrap'}} autoComplete="off">
lookfirst commented 5 years ago

@HenrikBechmann Have you tried following the styles documentation for typescript? In the past, it has been quite helpful for me. https://material-ui.com/guides/typescript/

HenrikBechmann commented 5 years ago

Thanks @lookfirst! I looked (though not first :-) ) at that doc, and used

export default withStyles({
  root: {
    display: 'flex',
    flexWrap: 'wrap',
  },
})(BaseForm)

(passing the object rather than the function)

... which both avoided the typescript errors, and enabled the injection of the generated class.

Hopefully the other tips there will help with more complex constructs.

HenrikBechmann commented 5 years ago

I also confirmed that the more complex structure for the styles function works (injects a generated className into form):

import React from 'react'

import { withStyles, createStyles, Theme } from '@material-ui/core/styles'

/*
    patterned after first demo https://material-ui.com/demos/selects/ for 3.03
    use Typsecript fixes from here: https://material-ui.com/guides/typescript/
*/

const styles = (theme:Theme) => createStyles({
  root: {
    display: 'flex',
    flexWrap: 'wrap',
  },
})

class BaseForm extends React.Component<any,any> {

    render() {
        const { classes } = this.props

        return (
            <form className = {classes.root} autoComplete="off">
                {this.props.children}
            </form>
        )
    }
}

export default withStyles(styles)(BaseForm)
chrislambe commented 5 years ago

Edit: @eps1lon has pointed out that this is unnecessary with the use of WithStyles!

I've had some success using ReturnType<T> to generate the ClassKey for me:

import * as React from 'react';

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

import { Theme } from '@material-ui/core/styles/createMuiTheme';

const overrideStyles = (theme: Theme) => ({
  root: {
    display: 'flex',
    flexDirection: 'column',
  },
});

type Props = StyledComponentProps<keyof ReturnType<typeof overrideStyles>>;

class MyComponent extends React.PureComponent<Props> {
  render() {
    return <div className={this.props.classes.root}></div>;
  }
}

export default withStyles(overrideStyles as StyleRulesCallback, {withTheme: true})(MyComponent);

Using keyof ReturnType<typeof styleOverrides> as the StyledComponentProps ClassKey will get you the keys from the object returned by overrideStyles and save you from having to keep a list of those keys manually. The only glitch I've noticed is a type error if I don't cast overrideStyles as StyleRulesCallback in the withStyles call. I'm not 100% sure why. I think it's withStyles not understanding what overrideStyles is for some reason.

To clarify that rather elaborate type, typeof styleOverrides resolves to the function that returns the style object. ReturnType<T> will get you the style object itself. keyof will get you the keys from the style object.

eps1lon commented 5 years ago

@chrislambe You should checkout the typescript guide. You shouldn't need to use ReturnType etc. createStyles and WithStyles should be sufficient as helpers.

chrislambe commented 5 years ago

@eps1lon Oh hey, very cool! Thanks!

HenrikBechmann commented 5 years ago

fwiw I'm liking the createStyles/withStyles pair more and more as I use them. Promotes tidy code, deals with all my styling/typescript issues, and if I want local conditional css I just create local style attributes, which of course take precedence over classes.

Nice!!

valoricDe commented 5 years ago

Following the typescript guide with @material-ui/core@3.2.2 I get Test does not have required attribute classes on:

import React from 'react'
import { Theme, WithStyles, withStyles, createStyles } from '@material-ui/core/styles'

const styles = (theme: Theme) => createStyles({
  root: {
    color: theme.palette.action.active
  },
})

interface Props extends WithStyles<typeof styles> {
  asd: boolean
}

class TestComponent extends React.Component<Props> {

  render() {
    const { classes } = this.props

    return (
      <div className={classes.root}>
      </div>
    )
  }
}

const Test = withStyles(styles)(TestComponent)

const a = () => <Test asd={true}/>
eps1lon commented 5 years ago

@aaronlifton2 Please follow Augmenting your props using WithStyles

TrejGun commented 5 years ago

@valoricDe Have you solved yout issue?