Better typescript and withstyles decorate

[Bnaya]

With this typings change you can do:

const decorate = withStyles(({ palette, spacing }) => ({
  root: {
    padding: spacing.unit,
    background: palette.background,
    color: palette.primary,
  },
  foo: {
    padding: spacing.unit,
    background: palette.background,
    color: palette.primary,
  }
}));

const DecoratedClass = decorate(
  class extends React.Component<Props & {classes: typeof decorate.PossibleClassesType}> {
    render() {
      const { text, type, color, classes } = this.props
      return (
        <Typography type={type} color={color} classes={classes}>
          {text}
         <span className={classes.foo}></span>
        </Typography>
      )
    }
  }
);

[pelotom]

Duplicate of #8829?

[oliviertassinari]

What do you guys want to do about it? @sebald @pelotom

[pelotom]

I was in favor of #8829 but I've since discovered that it still doesn't solve all of the problems. Specifically, you frequently need to specify the union of class keys when calling withStyles. For example,

const decorate = withStyles(theme => ({
  parent: {
    display: 'flex',
    margin: theme.spacing.unit * 3,
  },
  child: {
    justifyContent: 'center',
  },
}))

Due to a design limitation of TypeScript, the compiler can't infer the correct type here and it fails to type check. To fix it you need to explicitly provide the union of class keys when invoking withStyles:

const decorate = withStyles<'parent' | 'child'>(theme => ({
  parent: {
    display: 'flex',
    margin: theme.spacing.unit * 3,
  },
  child: {
    justifyContent: 'center',
  },
}))

which of course defeats the entire point of #8829 and this PR, which is to be able to automatically derive these class keys from the style object .

However in conjunction with #8819 it would make more sense, because you'd be able to assist inference in the other direction, from the inside out:

const decorate = withStyles(theme => ({
  parent: style({
    display: 'flex',
    margin: theme.spacing.unit * 3,
  }),
  child: style({
    justifyContent: 'center',
  }),
}))

For this reason I think my preference would be to accept both #8819 and #8829. The current situation of having to maintain both a style object as well as a union of its keys is pretty annoying.

[sebald]

Maybe we should pin @pelotom answer to that so we don't have this PR a third time? 😓

[pelotom]

@sebald what do you think about merging some form of both #8819 and #8829? I think the fact that PRs for this keep popping up indicates that it's a pain point 🙂

[oliviertassinari]

@pelotom This plan sounds good to me.

[sebald]

I am not of fan of both 🤔 Especially #8819. I think adding a layer of indirection should almost never be the answer and in this case adding StyleRule/StyleRuleCallback as an assertion will do the same for you as having another function, namely style.

#8829 goes a bit in the same direction. It seems like people just want less to write, which I can relate too, but it's kinda like with redux, when someone says that soooooo muuuuch boilerplate to write. Yes it is, but redux is not optimize for the simple case, but rather will help you write (explicit and thus) maintainable software. The same goes for TypeScript and flow.

tl;dr; That said: @pelotom is completely right. We should do something. Of the three I like @Bnaya solution the most, even though it not fully solves the issues described in #8819 + #8829.

---

@pelotom @oliviertassinari Sorry to the latency, I am stuck in a pile of work / organizing Christmas parties, the later on is a first-world problem I guess (but on a side note, if any of you ever is visiting Freiburg, ping me and lets get some coffee! ...or maybe give a talk at the local meetup.

[tcrognon]

If it's helpful to anyone while this is being sorted out, here's a hack that work with the current code:

const styles = ({ spacing }: Theme) => ({
    root: {
        padding: spacing.unit,
    }
});
const stylesReturnTypeHack = true ? null as never : styles(null as any);
type WithStylesHack = WithStyles<keyof typeof stylesReturnTypeHack>;

const decorate = withStyles(styles);

const DecoratedClass = decorate(
    class extends React.Component<WithStylesHack> {
        render() {
            return (
                <div className={this.props.classes.root}></div>
            );
        }
    }
);

[pelotom]

I think we should close this. #9272 outlines a simpler way in the common case to avoid the need to maintain an explicit union of class keys. Any objections?

[oliviertassinari]

@pelotom I was just going to ask as not active! I need to read this issue (#9272).

[Bnaya]

Well, feel free to do so :) ill check #9272
One could always create a wrapper around withStyles with these declarations
My immediate thinking about #9272 is that its cool, but you can't change the theme dynamically because you provide it on the js module level and not inside the react component tree (zoom up? color scheme for accessibly and more )

[pelotom]

@Bnaya it has limitations for sure; it's only a good for application developers with a single global theme.

[pelotom]

Ok, closing this for now...