[styles] Dynamic makeStyles unexpected behavior

jmdsg commented 5 years ago

[x] This is not a v0.x issue.
[x] I have searched the issues of this repository and believe that this is not a duplicate.

I have found some issues using makeStyles with dynamic styles (arrow functions) and i'm going to list them here since i think those may be related, and i feel that is unnecessary to create one issue per each of these points.

This issues are shown in this codesandbox MCVE https://codesandbox.io/s/lrwvw5lx6q.

The class name of a dynamic style contains 2 classes, the first is the represents the static class (hence is empty) and the second the dynamic class (with the actual styles). This behavior may be the intended to allow the reference of a static class from a dynamic class but i don't think is working, at least not completely.
Cannot mix or reference (with $) a static class (not using arrow function) from a dynamic class (using arrow).
Using some complex classname (not sure exacly which ones cause this behavior) when the component is rerendered (by a state change for instance) the previous style is not removed and the style sheet gets polluted and starts to have duplicate classes.
Cannot use a dynamic keyframe (using arrow function).
Cannot use/reference a static keyframe (just as an object) from a dynamic class. This may be related to (2).
And the last is more a question, i checked the code for makeStyles and i see that it does not depend on props, everytime useStyles is called and no matter if the props are the exacly the same the stylesheet is updated. Any reason for this?

Because when you call useStyles(props) with the same props or even using something like this:

const {
    height,
    width,
    ...rest
} = props;

// using only props that affect the styles generated
const memoizedStyleProps = useMemo(() => ({ height, width }), [height, width]);
const classes = useStyles(memoizedStyleProps);

Doesn't seem necessary to update the stylesheets.

Expected Behavior

Just one class name per dynamic style, e.g. makeStyles-animate-10
Be able to reference a static class from a dynamic class
Be able to rerender the component with complex classes without ending up with duplicate css rules in the style sheet.
Be able to use a key frame as a result of an arrow function.
Be able to use a static key frame from a dynamic style.

Current Behavior

Two class names per dynamic style, e.g. makeStyles-animate-4 makeStyles-animate-10
Cannot reference a static class from a dynamic class. In the example shows index.js:26 Warning: [JSS] Could not find the referenced rule simple in makeStyles.
Using a complex classname and rerendering the component causes the style sheet gets polluted
Not able to use a key frame as a result of an arrow function.
Not able to use a static key frame from a dynamic style. In the example shows index.js:26 Warning: [JSS] Referenced keyframes rule "rotate1" is not defined.

Steps to Reproduce

Inspect any of the elements using a dynamic class name and it will have a two classnames for.
In the example, if you change the definition of the class simple from simple: {} to simple: () => ({}) the title Hello CodeSandbox will have the expected green color.
Click one of the buttons in the example multiple times and then either inspect the h2 containing You clicked X times! or checking the rules in the sandbox document.styleSheets there will be multiple rule definitions repeated.
In the example the keyframe named rotate2 that is inside an arrow function is generated with empty body. The animation is not being applied.
In the example the keyframe named rotate1 is generated correctly but it is not referenced. The animation is not being applied.

Your Environment

Tech	Version
Material-UI	v4.0.0-alpha.8 and v4.0.2
React	16.8.0
Browser	Chrome: version 73.0.3683.103 (Build official) (64 bits)
TypeScript	No

Not sure if all this points are really issues or some of them are not allowed and its behavior is undefined.

oliviertassinari commented 5 years ago

cc @kof

jmdsg commented 5 years ago

Any news on this?

joacub commented 5 years ago

Same here is called too many times unnecessary

Fleuv commented 4 years ago

Encountered this problem as well.

I noticed that this problem only occurs when using a nested selector. So adding a separate class to the element the nested selector points at and adding the dynamic styling to that class should solve the problem.

In this code sandbox I debugged the problem: https://codesandbox.io/s/styling-duplicates-on-state-change-0weti

Note: In my browser (FireFox) I had to close and open the inspector to see the affect, thus make sure you follow the debug instructions in the demo.

Conclusion

This doesn't work:

myParentElement: (props) => ({
  '& .myNestedElement': {
    // use props
  }
})

This does work:


myNestedElement: (props) => ({
  // use props
})

warren-liang-exa commented 4 years ago

Is there any update on this issue? Still getting more than one class when using props for adaptive styling.

schnerd commented 4 years ago

Also interested in item 6 – makeStyles shouldn't re-calculate and re-attach styles unless the props change.

I'd imagine it can be fixed with a small change like https://github.com/schnerd/material-ui/commit/21f98bd61c6ab10bc54bf8195e70cefcc96502bb.

oliviertassinari commented 3 years ago

These issues should have been solved in v5 thanks to #22342 and the new @material-ui/styled-engine package. You can play with it at: https://codesandbox.io/s/bold-goodall-2iltv?file=/src/App.tsx. By default, it wraps emotion.

wwahammy commented 3 years ago

So, what's the work around on v4?

ffjanhoeck commented 3 years ago

@oliviertassinari A workaround for v4 would be really awesome. We have huge application, which is using material-ui. That means, when v5 gets released, we have to prepare 400 files before we can really update...

oliviertassinari commented 3 years ago

@ffjanhoeck I would recommend trying react-jss out. The migration should be easy and they have a better handling of this problem. Also note that you don't need to migrate all the makeStyles and withStyles in v5, the module is still here.

ffjanhoeck commented 3 years ago

@oliviertassinari Is the makeStyles in material-ui v4 not a react-jss wrapper?

Or do you mean by trying out react-jss, that I should directly use react-jss, without the makeStyles from material-ui ?

oliviertassinari commented 3 years ago

@ffjanhoeck Material-UI v4 uses jss, not react-jss. My suggestion is to use react-jss

oliviertassinari commented 3 years ago

An update, we have now made enough progress with the new @material-ui/styled-engine package in v5 to move toward a progressive removal of the @material-ui/styles package (based on JSS). The current plan:

In v5.0.beta.0, this package will come standalone, in complete isolation from the rest.
In v5.0.0, this package will be soft deprecated, not promoted in the docs, nor very actively supported.
During v5, work on making the migration easier to the new style API (sx prop + styled() API). We might for instance, invest in the documentation for using react-jss that has more or less the same API. We could also invest in an adapter to restore the previous API but with emotion, not JSS.
In v6.0.0, this package will be discontinued (withStyles/makeStyles API).

This was made possible by the awesome work of @mnajdova.

maticrivo commented 2 years ago

Hi @oliviertassinari How do we change the usage of jss in favor of react-jss in v4 to solve this issue temporarily until we can upgrade to v5?

I couldn't find anything about it in the docs.

Thanks.

ezbeazy commented 1 month ago

@maticrivo this worked for me.

https://stackoverflow.com/questions/62902832/why-do-you-need-to-apply-the-two-generated-class-names-root-disabled-to-the

add to createStyles in makeStyles disabled: { // Define any styles for the disabled state if needed },

mui / material-ui