makeStyles: Styles are not being overridden if the original style is declared with a function

danguilherme commented 3 years ago

Properties declared with functions in makeStyles (e.g. backgroundColor: props => props.color) are not overridden by latest styles that override the same property, but without a function. For example, for this style hook:

const useStyles = makeStyles({
  root: {
    backgroundColor: props => props.color
  },
  redBackground: {
    backgroundColor: 'red'
  }
});

A component using both classes (e.g. className={`${classes.root} ${classes.redBackground}`}) will always have a background color as "props.color" (see the example code sandbox in the "Steps to Reproduce" section).

[x] The issue is present in the latest release.
[x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Given the following useStyles declaration:

import { makeStyles, Theme } from "@material-ui/core/styles";

const theme = {
  // ... simplified theme object ...
  green: 'green'
}

const useStyles = makeStyles<Theme, typeof theme, "root" | AvatarVariant>({
  root: {
    backgroundColor: theme => theme.green,
    fontWeight: 700,
    color: "red",
    width: "64px",
    height: "64px"
  },
  // variants
  default: {},
  outline: {
    backgroundColor: "transparent",
    border: `1px solid red`
  }
});

Rendering the following component

export default function AvatarWithCustomVariants() {
  const { root, outline } = useStyles(theme);
  return (
    <React.Fragment>
      <MuiAvatar className={root}>DF</MuiAvatar>
      <MuiAvatar className={`${root} ${outline}`}>OL</MuiAvatar>
    </React.Fragment>
  );
}

Renders two Avatar components with a green background:

Expected Behavior 🤔

The first Avatar has a green background, and the second has a transparent background.

Steps to Reproduce 🕹

Running example: https://codesandbox.io/s/nice-nobel-cwg7t (using Material UI v4 because makeStyles seems to be broken in the latest beta of v5).

Changing the following line, it works as expected.

const useStyles = makeStyles<Theme, typeof theme>({
  root: {
-   backgroundColor: theme => theme.green,
+   backgroundColor: "green",
    fontWeight: 700,
    color: "red",
    width: "64px",
    height: "64px"
  },
  // variants
  default: {},
  outline: {
    backgroundColor: "transparent",
    border: `1px solid red`
  }
});

Or, if you're feeling like doing something more hacky, this also works:

const useStyles = makeStyles<Theme, typeof theme>({
  root: {
   backgroundColor: theme => theme.green,
    fontWeight: 700,
    color: "red",
    width: "64px",
    height: "64px"
  },
  // variants
  default: {},
  outline: {
-   backgroundColor: "transparent",
+   backgroundColor: () => "transparent",
    border: `1px solid red`
  }
});

Context 🔦

I am building an Avatar component for our design system, wrapping the Material UI's Avatar. It has 3 different sizes and two variants: default, with a dark background, and outline, with no background.

I am using different class names in the makeStyles declaration to avoid the excessive use of ternaries, and neatly group variants' styles. So in the component I can do something like this:

export const Avatar: React.FC<AvatarProps> = ({
    name,
    size = 'large',
    variant = 'default',
}) => {
    const classes = useStyles(useTheme());

    const classNames = [classes.root, classes[size], classes[variant]];

    return (
        <MuiAvatar variant="circle" className={classNames.join(' ')}>
            {formatName(name)}
        </MuiAvatar>
    );
};

Here is how it looks like so far, simplified.

I am aware I can just move the backgroundColor declaration to the default variant -- that's how I am going to fix my issue. But I still believe the mentioned behaviour is not working as expected - or it is just not documented.

Your Environment 🌎

`npx @material-ui/envinfo`

Browser used: Chrome ``` System: OS: macOS 11.4 Binaries: Node: 12.14.1 - ~/.nvm/versions/node/v12.14.1/bin/node Yarn: 1.21.1 - /usr/local/bin/yarn npm: 6.13.4 - ~/.nvm/versions/node/v12.14.1/bin/npm Browsers: Chrome: 91.0.4472.114 Edge: Not Found Firefox: Not Found Safari: 14.1.1 npmPackages: @emotion/styled: 10.0.27 @material-ui/core: ^4.11.1 => 4.11.3 @material-ui/data-grid: ^4.0.0-alpha.6 => 4.0.0-alpha.6 @material-ui/lab: ^4.0.0-alpha.47 => 4.0.0-alpha.47 @material-ui/styles: ^4.9.10 => 4.9.10 @material-ui/system: 4.11.3 @material-ui/types: 5.1.0 @material-ui/utils: 4.11.2 @types/react: ^16.8.2 => 16.9.13 react: ^16.9.0 => 16.12.0 react-dom: ^16.11.0 => 16.12.0 styled-components: ^5.1.1 => 5.1.1 typescript: ^4.2.4 => 4.2.4 ```

mnajdova commented 3 years ago

I've updated the codesandbox to use v5 - https://codesandbox.io/s/material-demo-forked-phnu0?file=/demo.tsx I can reproduce the issue.

Not sure what would be the timeline for us to debug the issue. My suggestion in the mean time would be to use the makeStyles theme callback function for getting values from the theme, that way there wouldn't be any surprises, and there wouldn't be any perf implication for using props. Here is a dirty example of it - https://codesandbox.io/s/material-demo-forked-4s640?file=/demo.tsx (you would need to use module augmentaiton for updating the Theme & ThemeOptions types, as described in https://next.material-ui.com/customization/theming/#custom-variables

danguilherme commented 3 years ago

Hi @mnajdova, thanks for the update!

We currently don't "augment" the Material UI theme object, we use two different objects to deal with our Styled System and Material UI components. So we'll probably have to 'merge' them to make our lives easier.

[...] and there wouldn't be any perf implication for using props.

Could you point me what are those performance implications? We have noted some of them, but it would be nice to have some more information about it - including how to avoid them. Of course a link to an issue would be sufficient.

mnajdova commented 3 years ago

@danguilherme sure these could be a good starting points - https://github.com/mui-org/material-ui/issues/21657, https://github.com/mui-org/material-ui/issues/21143 Note that, these are fixed by v5 and the new @material-ui/styled-engine (fixed in terms of there will be better performance if the alternatives are used)

The general problems with makeStyles is when dynamic props are used, apart from that, there are no known performance implications. Also, note that in v5, the default styling engine used will be emotion, and these utilities will be available from @material-ui/styles and would still use JSS as styling engine.

Also, this may be relevant - https://github.com/mui-org/material-ui/issues/26571

mui / material-ui