Discussion: are callbacks to withStyles always the best way to obtain the theme?

[pelotom]

This question occurred to me while thinking about #9192, and I'll repost part of my comment there:

If one is not developing reusable MUI components, but instead making an app, is there any advantage to using withStyles with a callback, vs. exporting your theme from a module and then importing it everywhere that you want to use it? For example,

// src/theme.ts

import createMuiTheme from 'material-ui/styles/createMuiTheme';
import { red, purple } from 'material-ui/colors';

export default createMuiTheme({
  palette: {
    primary: red,
    secondary: purple,
  },
});

// src/index.tsx

import * as React from 'react';
import App from './App';
import theme from './theme';

ReactDOM.render(
  <MuiThemeProvider theme={theme}>
    <App />
  </MuiThemeProvider>,
  document.getElementById('root')
);

// src/component.ts

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

export default withStyles({
  root: {
    margin: theme.spacing.unit * 3,
  },
})(...);

This is particularly relevant to TypeScript for 2 reasons:

• it seems impossible to solve [TypeScript] Currently not possible to use business variables in a theme with "withStyles" #9192 in a well-typed way with the callback approach
• callbacks interfere with type inference because of this TypeScript issue. If you look through the material-ui issue tracker you'll find that this has been the source of all kinds of confusion. The workaround, which is not obvious at all, requires annotating withStyles with a union of all your class keys, e.g. withStyles<'root'| | 'docked' | 'paper' | 'modal'>(/* ... */). Various complex solutions have been proposed for this, notably Add witness type for WithStyles<ClassKey> to return value of withStyles() #8829 and Better typescript and withstyles decorate #9105.

Simply exporting a global theme and then importing it where you need it rather than using callbacks everywhere magically makes all these problems melt away.

It seems like the advantage of using a callback is that it makes your components more modular; you can ship them as a library and others can use them within any MuiThemeProvider. But in many cases people aren't making reusable libraries, they're making an app, in which case it seems like it's fine to just reference a global theme.

[stunaz]

Building an app here. So no need for withStyles with callback.

[TheHolyWaffle]

I don't agree, mostly due to the following missing feature #7633 (passing props as a parameter to the callback).

Having a callback with the props of the component as arguments is something that will definitely enhance the readability of styling of components. As of right now, this is not yet the case, so the workaround is to make use of inline styling. Which is not the best solution, as we miss out on all the great jss plugin features such as auto-prefixer for example.

For clarification, I'm also building an app, but that doesn't mean I don't want to create my own re-usable components.

Edit: If it would be possible to pass both the props and theme to the callbacks of the individual css properties (such as react-jss) then remark becomes invalid. For example:

const styles = {
  button: {
    background: props => props.color
  }
}

[pelotom]

@TheHolyWaffle I agree that it would be useful to use callbacks for making use of props in styles... this discussion is specifically about the need for callbacks to obtain the theme.

[sebald]

@TheHolyWaffle I guess this is a bit off topic, but anyway 🙂 The API your're describing is what styled-components, glamorous and emotion are doing. AFAIK jss decided to it differently.

Basically, you create all CSS classes beforehand and apply them later on based on props via helpers like classnames. Here is an example from the <Button>:

https://github.com/mui-org/material-ui/blob/865fb416f62940bff4de8b8f4b4bc5f3884c70c7/src/Button/Button.js#L252-L268

---

@pelotom Had the same discussion at my company multiple times. If you're only consuming MUI standard components, there is no downside of your mentioned approach. At least none I can think off. If is only relevant for "3rd party" MUI components, like https://github.com/dmtrKovalenko/material-ui-pickers.

[rosskevin]

I'm still getting my bearings with typescript, but I wanted to add that we have a few libraries that use withStyles (just like the mui uses it), then a composite app that uses those libraries and mui. Any contemplated solution should allow for a similar pattern of use.

Are you suggesting that the theme: Theme properties in the callback would no longer be accessible in this manner?

[pelotom]

Are you suggesting that the theme: Theme properties in the callback would no longer be accessible in this manner?

Not at all! I'm not actually suggesting anything should change about the library... if you're developing a reusable library of components, which it looks like this is an example of, then the callback method definitely seems necessary.

[rosskevin]

FYI - so searchers may easier find this issue.

I ran into this with a stateless functional component (gist):

Error:(9, 3) TS2345:Argument of type '({ palette, shadows, transitions }: { direction: "ltr" | "rtl"; palette: Palette; typography: Typ...' is not assignable to parameter of type 'Record<"root" | "scrolled" | "avatar" | "content" | "contentSecondary", Partial<CSSProperties>> |...'.
  Type '({ palette, shadows, transitions }: { direction: "ltr" | "rtl"; palette: Palette; typography: Typ...' is not assignable to type 'StyleRulesCallback<"root" | "scrolled" | "avatar" | "content" | "contentSecondary">'.
    Type '{ root: { display: string; overflow: string; zIndex: number; backgroundColor: string; transition:...' is not assignable to type 'Record<"root" | "scrolled" | "avatar" | "content" | "contentSecondary", Partial<CSSProperties>>'.
      Types of property 'root' are incompatible.
        Type '{ display: string; overflow: string; zIndex: number; backgroundColor: string; transition: string; }' is not assignable to type 'Partial<CSSProperties>'.
          Types of property 'overflow' are incompatible.
            Type 'string' is not assignable to type '"initial" | "inherit" | "unset" | "auto" | "scroll" | "hidden" | "visible" | undefined'.

which implies the inference failure. The fix being declaring an explicit ClassKey and parameterizing my call to withStyles

type ClassKey = 'root' | 'scrolled' | 'avatar' | 'content' | 'contentSecondary'
const decorate = withStyles<ClassKey>(

Thanks for the guidance @pelotom

[mbrookes]

@rosskevin / @pelotom Would this be worth adding to the docs (Typescript guide)?

[pelotom]

@mbrookes yes, I'll try to add something when I get a chance.

[oliviertassinari]

#7633 starts to be referenced by a lot of people. I will see if I can make it happen. As it was linked here. I'm eager to know if we can improve the DX and the performance with such feature.

is there any advantage to using withStyles with a callback, vs. exporting your theme from a module and then importing it everywhere that you want to use it?

@pelotom At work, we have been relying on the theme callback for most of our styles. The rationale is simple. We are lazy. We find it simpler to add one argument than to add one import.
Ok, it's not the best argument 🤔 . If you don't need the live theme update, or the theme nesting feature (I don't, and most people don't), then yes, you can do what you prefer :).

[pelotom]

At work, we have been relying on the theme callback for most of our styles. The rationale is simple. We are lazy. We find it simpler to add one argument than to add one import.

I'm lazy too! In TypeScript, it auto-imports when you start to type theme 🙂

[Bnaya]

In that case, that we don't pass the theme via the context,
i would take that one step forward.
Another issue with TS and withStyles, is that, withStyles returns a component that expects classes prop ts-wise, even tho withStyles supply it. (ts current limitation)

What about:

export default function withStyles<ClassKey extends string>(
  style: StyleRules<ClassKey> | StyleRulesCallback<ClassKey>,
  options?: WithStylesOptions
): <P>(
  classes: WithStyles<ClassKey>
) => React.ComponentType<P>;


withStyles({root:...})((classes) => {
  return function MyComponent() {
    return <div className={classes.root} >MyComponent</div>
  }
})

// or
withStyles({root:...}, (classes) => {
  return function MyComponent() {
    return <div className={classes.root} >MyComponent</div>
  }
})

[Bnaya]

Or, why not just:

const classes = withStyles({root:...});

[pelotom]

Great questions... I've never really understood the need for the indirection of passing classes through the props. What do you think @oliviertassinari?

Another option that has been discussed is render props.

[rosskevin]

A render callback component (I prefer using children as a function, not an unnecessarily verbose extra render prop) is something we should convert WithStyles to. We can then offer an additional HOC (the current withStyles signature) easily that simply calls the render callback component.

I recently switched my patterns to render callbacks, and while it has been great and works for most, it doesn't always work best for every scenario. Nonetheless I believe the render callback component should have the implementation, with the hoc being a convenience wrapper. react-i18next (<I18n/> and translate) is a good example this.

[pelotom]

Yeah, I'm a big fan of using children as a render prop. Are you thinking something like

class MyComponent extends React.Component<Props> {
  render() {
    return (
      <WithStyles styles={styles}>
        {classes =>
          <...>
        }
      </WithStyles>
    )
  }
}

?

[rosskevin]

Yes, where current args styles and options are props on WithStyles. I haven't though much about the WithStyles name, open to other ideas.

[pelotom]

Maybe just Styled if we're bikeshedding names 😄

[pelotom]

This gives a natural way to accomplish a feature many have been asking for: styles that depend on props. Just move the definition of styles inside the component.

class MyComponent extends React.Component<Props> {
  render() {
    return (
      <Styled styles={this.styles} options={options}>
        {classes =>
          <...>
        }
      </Styled>
    )
  }

  styles = {
    // ... can depend on `this.props`
  }
}

[rosskevin]

I agree with using Styled plus keeping withStyles name for the HOC.

[Bnaya]

I have another iteration
It gives the class names by inferr, supports the them via context, expose type that you can use separately. (See the style: typeof Styled.style)

import { WithStyles } from "material-ui";
import { Theme } from "material-ui/styles";
import withStyles from "material-ui/styles/withStyles";
import * as React from "react";

interface IStyledProps<ClassKey extends string> {
  children(style: WithStyles<ClassKey>): JSX.Element;
}

type stylesParamType<ClassKey extends string> = (
  theme: Theme,
) => Record<ClassKey, React.CSSProperties> | Record<ClassKey, React.CSSProperties>;

export function createStyled<ClassKey extends string>(stylesParam: stylesParamType<ClassKey>) {
  const wrapperComponent = withStyles(stylesParam);

  const createdStyled: React.StatelessComponent<IStyledProps<ClassKey>> = function createdStyledComponent(props) {
    return React.createElement(
      wrapperComponent(stylePropsFromWithStyles => {
        return props.children(stylePropsFromWithStyles);
      }),
    );
  };

  Object.defineProperty(createdStyled, "style", {
    get() {
      throw new Error("style prop is here only for TypeScript inference. use typeof Styled.style for the type");
    },
  });

  // style is just for pulling the types
  return createdStyled as typeof createdStyled & { style: WithStyles<ClassKey> };
}

How to use:

const styles = (theme: Theme) => ({
  root: {
    width: "100%",
    background: theme.palette.background.paper,
    flex: "1",
    overflowY: "scroll",
  } as React.CSSProperties,
  preventInput: {
    pointerEvents: "none",
  },
});

const Styled = createStyled(styles);

function InnerComponent(props: { title: string; style: typeof Styled.style }) {
  return <div className={props.style.classes.root}>{props.title}</div>;
}

function OutterComponent(props: { title: string }) {
  return (
    <Styled>
      {s => {
        return <InnerComponent {...props} style={s}/>;
      }}
    </Styled>
  );
}

[oliviertassinari]

@Bnaya If you build the component outside of the render method, in a static way, it should be good :). This means that we potentially have 3 different styling APIs. The implementation of the 2 other APIs is simple and short sugar on top of withStyles(): styled and Styled. This is exciting. Thanks for sharing. It's something people need to be aware of. But I'm wondering. What's the best way to deliver?

[oliviertassinari]

I'm gonna document the two styling API alternatives.

[Bnaya]

I must point out that Conditional types are on the way, and will be with us in typescript 2.8
That will allow you to create HOC that omits some of the props of the component it wraps
microsoft/TypeScript#21316

[caub]

Should theme data remain simple constants, agnostic to CSS property keys? (only defining their possible values)

or is it still good practice to put full classes in it like that:

export const theme = createMuiTheme({
  palette: {
    primary: blue,
  },
  card: { // some custom reusable classes
    header: {
      background: '...',
      // ...
      '&::before': {
        // ....
      }
    },
    progress: {
      height: 64, 
      // ...
    }
  }
});

then in my components:

const styles = ({ card: { header, progress }, palette }) => ({
  header,
  progress,
  otherProp: {
    color: palette.primary[600],
    // ...
  }
});

export default withStyles(styles)(Component)