Skip to content
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] Fix withStyles typing for class components; remove usage as TS decorator #8561

Merged

Conversation

pelotom
Copy link
Member

@pelotom pelotom commented Oct 5, 2017

The compromised typing of withStyles to support using it as a TypeScript class decorator has caused a lot of confusion. This PR makes the typing fully correct, at the expense of being able to use withStyles as a decorator. If and when TypeScript #4881 is fixed, withStyles should automatically be usable as a decorator again, with no loss of type safety.

Resolves #8447.

Breaking change

  • Drop TypeScript decorator support

@@ -53,6 +53,7 @@ import Table, {
} from '../../src/Table';
import { withStyles, StyleRulesCallback } from '../../src/styles';
import { withResponsiveFullScreen, DialogProps } from '../../src/Dialog';
import { WithStyles } from '../../src/styles/withStyles';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add this to the exports of material-ui/styles, so people don't have multiple imports? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithStyles is already exported in styles no? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sir ... I am going to blame this on Firday morning 😴

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think withStyles is currently exported in material-ui/styles since it keeps coming back as undefined for me.

Copy link
Member Author

@pelotom pelotom Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amyasapp these comments were about WithStyles, which is a type... but it certainly seems like withStyles is exported from material-ui/styles.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm using v0.19.4 and not v1 beta. Would that change anything? My coworkers and I grep'd for withStyles and couldn't find it anywhere in our node_modules.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amyasapp yeah, I don't think there's a withStyles in v0.19.4.

@sebald sebald merged commit e6d8d7a into mui:v1-beta Oct 6, 2017
@pelotom pelotom deleted the fix-withStyles-applied-to-class-component branch October 6, 2017 08:18
@oliviertassinari oliviertassinari changed the title Fix withStyles typing for class components; remove usage as TS decorator [typescript] Fix withStyles typing for class components; remove usage as TS decorator Oct 6, 2017
@pelotom pelotom mentioned this pull request Oct 13, 2017
the-noob pushed a commit to the-noob/material-ui that referenced this pull request Oct 17, 2017
…tor (mui#8561)

* Fix withStyles typing for class components; remove usage as TS decorator
* Remove unnecessary non-null assertion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants