-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Typescript type error in component returned by withStyles() #8447
Comments
@cfilipov when decorating a component class you should use
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>
)); |
If you read my followup to that comment you'll see that |
I saw your comment regarding 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. |
@cfilipov my first refactor of the |
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. |
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. |
#8550 looks like further evidence that people are confused by this, and we should consider not supporting |
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>
);
}
}
); |
@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 |
It's a simple change; I went ahead and opened a PR in case you want to go with it 🙂 |
@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),
); |
@marcusjwhelan withStyles(styles)<NonStyleProps>(...); If you give the types for |
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 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 |
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 The third way is using the correct type, so that |
@pelotom Yes, sorry. You're right. Really is not my day... 🤐 So lets merge! |
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; |
@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 🤢 |
@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 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? |
@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. |
@pelotom Silly question, is there a way I could get notified once a new release is done? |
@wcandillon Follow us on Twitter. |
@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. EditI figured it out. |
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>
);
}
} |
I was fighting with this issue (and #8704), with no clear result over the past few days. Then I took the advice from @pelotom:
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. |
|
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. 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 ;-) |
@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 <BottomNav someProp="foo" /> |
Shame on me... Oh, shame on me. Thanks a lot @pelotom 😄 |
@otroboe Also remove the string duplication...
Wish that one was easier though... |
Yes I did that too, thanks 👍 |
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? |
@nishmeht7 can you post a self-contained snippet? |
@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. |
I followed the example of a basic form from https://material-ui.com/demos/selects/ but got complaints that So I've reverted back to styles for now until this gets sorted:
|
@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/ |
Thanks @lookfirst! I looked (though not first :-) ) at that doc, and used
(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. |
I also confirmed that the more complex structure for the
|
Edit: @eps1lon has pointed out that this is unnecessary with the use of I've had some success using 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 To clarify that rather elaborate type, |
@chrislambe You should checkout the typescript guide. You shouldn't need to use |
@eps1lon Oh hey, very cool! Thanks! |
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!! |
Following the typescript guide with @material-ui/[email protected] I get
|
@aaronlifton2 |
@valoricDe Have you solved yout issue? |
@TrejGun I just checked. With a functional component and @material-ui/[email protected] I do not have this problem |
I don't really understand. I've been reading these issues for a few hours now and I'm not really getting it. Any help would be so so nice at this point. Regarding this suggestion above
How do I found out how to use But the docs tell us to do something that simply won't work? what am I missing? I woiuld like to use https://material-ui.com/guides/typescript/#augmenting-your-props-using-withstyles... Is this possible? @valoricDe, How did you do the functional component that didn't have that issue
I'm trying something like this:
When I use this component I have this error:
|
@yehudamakarov Try you code without When encountering these issues I'll
It promotes cleaner code. Especially with regards to order of operations. You currently mix |
Thanks so much Sebastian. I think those generic arguments were messing with my OwnProps interface that extends WithStyles<>. Since I’m passing everything to the component anyway the type checking I get from my Props is sufficient. Not sure why id need the connect<> generics. Thanks! |
And how to add themes (useTheme()) inside like:
|
When using
withStyles()
hoc in typescript, I am getting the following error when trying to use the returned component: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
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:and doing it the more common way (as seen in styles.spec.tsx):
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 optionalclasses
property. The new definition......returns the same type
C
as the component, this means that passingClassNames
which is not marked optional propagates to the returned component. I see mentioned here the use ofPartial<>
which I think it an unsightly hack.Your Environment
The text was updated successfully, but these errors were encountered: