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] createStyles and improved WithStyles helpers #11609

Merged
merged 11 commits into from
May 29, 2018

Conversation

pelotom
Copy link
Member

@pelotom pelotom commented May 28, 2018

  • Resolves [typescript] withStyles() complains unless using as #10995 by adding the helper function createStyles which is basically what was suggested in [typescript] add sample with return types #11512 (comment). It doesn't actually "do anything", it's just the identity function, but it's useful for guiding TypeScript inference, particularly for styles which depend on the theme.
  • Augments the WithStyles type operator to allow taking typeof styles as input, which allows declaring prop types with injected style types like so:
    interface Props extends WithStyles<typeof styles> {
      // ...
    }

See the updated TypeScript guide for more about how to use these.

@pelotom pelotom changed the title [typescript] createStyles and InjectedStyles helpers [typescript] createStyles and improved WithStyles helpers May 28, 2018
Copy link
Member

@sebald sebald left a comment

Choose a reason for hiding this comment

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

Really good work!

I am not a fan of having to use the identity function in TS to have good type inference, but we tried long enough to not use this approach I guess 🙂

@@ -5,45 +5,129 @@ Have a look at the [Create React App with TypeScript](https://github.com/mui-org

## Usage of `withStyles`

The usage of `withStyles` in TypeScript can be a little tricky, so it's worth showing some examples. You can first call `withStyles()` to create a decorator function, like so:
The usage of `withStyles` in TypeScript can be a little tricky for a number of reasons, but
Copy link
Member

Choose a reason for hiding this comment

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

Is there something missing after the "but"? 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops 😬

bar: boolean;
}

const styles = (theme: Theme) => createStyles({
Copy link
Member

Choose a reason for hiding this comment

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

Maybe first styles then Props so you know what typeof styles refers to? Also "declare before use" lint error! 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Types can refer to things in any order, and at least in my code I tend to put all type declarations at the top of the file. But I don't feel too strongly about it.

describe('createStyles', () => {
it('is the identity function', () => {
const styles = {};
assert.strictEqual(createStyles(styles), styles);
Copy link
Member

Choose a reason for hiding this comment

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

This is a little nit-picking, but does strictEqual compare properties or checks that the passed in arguments are the same "pointer". Meaning, the result of createStyles and styles is the same reference.

If not, we could add another tests that just check for ===.

Copy link
Member

@oliviertassinari oliviertassinari May 28, 2018

Choose a reason for hiding this comment

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

strictEqual uses === 👍 .

export interface WithStyles<ClassKey extends string = string> extends Partial<WithTheme> {
classes: ClassNameMap<ClassKey>;
}
export type WithStyles<T extends string | StyleRules | StyleRulesCallback> = Partial<WithTheme> & {
Copy link
Member

Choose a reason for hiding this comment

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

This is absolutely awesome and a good use case for conditional types 🤩

Copy link
Contributor

Choose a reason for hiding this comment

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

Removing the default = string is a breaking change though?

Copy link
Member Author

Choose a reason for hiding this comment

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

@geirsagberg right you are. I've opened #11808 to fix that.

@@ -0,0 +1,5 @@
// @flow

export default function createStyles(s: Object) {
Copy link
Member

Choose a reason for hiding this comment

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

It's funny, life is a circle, we had this function in v1.0.0-alpha.x.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know that... did it serve the same purpose?

Copy link
Member

Choose a reason for hiding this comment

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

Nop, we used it to perform some operations on the styles.

describe('createStyles', () => {
it('is the identity function', () => {
const styles = {};
assert.strictEqual(createStyles(styles), styles);
Copy link
Member

@oliviertassinari oliviertassinari May 28, 2018

Choose a reason for hiding this comment

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

strictEqual uses === 👍 .

guttered: theme.mixins.gutters({
'&:hover': {
textDecoration: 'none',
},
}),
listItem: {
'&:hover $listItemIcon': {
visibility: 'inherit',
// visibility: 'inherit',
Copy link
Member

Choose a reason for hiding this comment

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

On purpose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, will restore.

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.

4 participants