[styles] Performance issue when using styled components extensively

yemi commented 5 years ago

Intro I just migrated my whole app to use styled from @material-ui/styles in place of withStyles HoC, however, by doing this my <head> gets polluted by large amounts of <style> tags.

Issue I understand this is on purpose, however, it seems that my app performance get affected as the style tags can grow up to numbers of 100s or 1000s, especially when on a page rendering an infinite scroll of e.g. blog posts, each containing its own set of styled components.

Question It seems that each instance of any styled component gets its own unique class, even though its not always needed since it could easily be reused around the entire app, e.g. a <PostTitle> styled component. Is this possible to achieve or is there any other performance-related things I can do in order to speed up/skip rendering using styled? Whats the preferred way of using styled for large number of components?

oliviertassinari commented 5 years ago

@yemi Have you considered using styled-components? We expose @material-ui/styles so people can avoid bundling two CSS-in-JS runtimes. If a 14kB gzipped overhead is fine for you, we recommend styled-components. (#6115)

yemi commented 5 years ago

@oliviertassinari I have but didn't know if it would solve the problem, are you saying it might or that there is a good chance it will? Would I be able to use the same theme provider or two theme providers to incorporate styled-components?

oliviertassinari commented 5 years ago

@yemi Right now, you would need to use 2 theme providers, with the same theme object. It will solve the performance issue, we could solve the problem here too, but the resources we have have been forwarded to different topics.

tho-graf commented 5 years ago

We encountered this problem as well. We were styling a lot of svg elements and the performance was horrible.

Some rerenders took up to ~16s with @material-ui/styles.

We tried several styling methods for out hot paths.

With emotion-js we could reduce it to ~2s. With plain style={{/css object/}} we could reduce it to ~1s.

oliviertassinari commented 5 years ago

@tho-graf See #6115.

tho-graf commented 5 years ago

Thank you! This looks interesting, looking forward. Until then we have good workarounds. :pray:

jsakas commented 5 years ago

We are experiencing the same issue after building a large components library and then react application on top of it. Using only @material-ui/styles (not @material-ui/core). We notice a very bad performance degradation when we get up to 3-4 thousand style tags in the head of the document.

Is there a workaround to increase performance without needing to re-write the entire thing in a different syntax (e.g. styled components) ..?

Is there a way to bundle all CSS to a static assets in some type of production mode?

oliviertassinari commented 5 years ago

@jsakas Did you try to switch to react-jss? Are you using style functions or static styles exclusively?

jsakas commented 5 years ago

@oliviertassinari we are using style functions exclusively, here is what a simple component looks like:

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

import Props from "./Option.props";
import { Theme } from "../../themes";

export default makeStyles(
  (theme: Theme) =>
    createStyles({
      option: {
        backgroundColor: ({ active }: Props) => {
          return active ? "#eee" : "#fff";
        },
        "&:hover": {
          backgroundColor: "rgba(238,238,238,0.6)"
        }
      }
    }),
  { defaultTheme: Theme }
);

import React from "react";
import clsx from "clsx";

import Props from "./Option.props";
import useStyles from "./Option.styles";

const Option: React.FC<Props> = (props: Props) => {
  const classes = useStyles(props);

  return (...);
};

export default React.memo(Option);

Can you help me understand what the refactor path looks like to use react-jss?

oliviertassinari commented 5 years ago

@jsakas Thanks for the extra details. Material-UI usage of the style functions is limited to the Box component for this very problem, and even then, people can easily switch the Box to styled-components or emotion if they find it to slow. So yeah, I understand your frustration. It prevents Material-UI to implement new important features our user asks for.

Regarding the solution, because the whole community seems to converge to styled-components, and because it has more monetization potential for the project, we have decided to give up on our custom CSS-in-JS implementation. In the coming months, we will progressively provide a styled-components, unstyled, and hopefully emotion version of our components. Now, because we don't want to force our users to rewrite all their styles, I think that we should keep the JSS implementation around. As far as I know, react-jss uses a single style sheet per component which solves a lot of different problems. This problem is definitely up for grab, but we had no contributor making progress yet.

jsakas commented 5 years ago

@oliviertassinari thanks so much for the info. Refactoring to use react-jss wasn't too bad, since it is mostly changing the functions that wrap the styles, and the style declarations don't need any intervention.

I'm seeing a massive improvement in performance. On one page, where we had 2687 style tags, we now have 73.

yemi commented 5 years ago

@oliviertassinari Just a question, will the app performance be improved by migrating to styled-components even if 20% of the components use dynamic props like:

const Header = styled('header')(({ theme, isSticky, topOffset }) => ({
  display: 'flex',
  flexDirection: 'column',
  borderBottomWidth: '1px',
  borderBottomStyle: 'solid',
  borderBottomColor: '#efefef',
  background: `rgba(255,255,255, 0.97)`,
  zIndex: theme.zIndex.appBar,
  top: 0,
  ...(isSticky && {
    borderBottomWidth: 0,
    top: -topOffset,
  }),
  [theme.breakpoints.up('sm')]: {
    flexDirection: 'row',
    padding: `0 ${theme.spacing(6)}px`,
    borderBottom: 'none',
  },
  [theme.breakpoints.up('lg')]: {
    padding: `0 ${theme.spacing(8)}px`,
  },
}));

oliviertassinari commented 5 years ago

@yemi Probably not, it's noticeable if you rerender the component frequently.

jayarjo commented 4 years ago

@oliviertassinari so has material-ui switched to styled-components, or still not yet?

oliviertassinari commented 4 years ago

It's one of the objectives of v5. I think that we will soon initial the effort for the next major.

knitesh commented 4 years ago

@oliviertassinari does moving to styled-components has been finalized ?

kodonnell-heila commented 4 years ago

It would be nice if this was added to your docs:

we have decided to give up on our custom CSS-in-JS implementation

It's frustrating to find this post after implementing your jss implementation thinking it is a better option than scss files....

oliviertassinari commented 4 years ago

An update, this issue is being resolved in v5 thanks to #22342. So far, we have migrated the Slider in the lab, where this can be tested.

yemi commented 4 years ago

@oliviertassinari cool! I would like to add though, that when using the production build I don't have the same performance issues as I did initially, meaning that I currently don't have any performance issues. Reason being that the extra style tags in do get removed when not used anymore. The main reason I really enjoy working with JSS in Material UI is that it provides me a great DX, far surpassing that of styled-components, mainly because JSS I can use pure js logic, and styled components stick with CSS syntax, which is not optimal in a js environment (i know you can use styled-components with jss-like syntax, but this seems quite unstable and its clearly not its main way of using it).

So, in terms om perf, I'm ok, and as #22342 points out, DX is a big deal, this is why I wouldn't want anything else

oliviertassinari commented 4 years ago

It's also worth mentioning that we have recently invested time in improving our benchmark test to help us move in the right direction: https://github.com/mui-org/material-ui/tree/next/benchmark.

mbret commented 3 years ago

Not only the performance are a problem but they also keep piling up and never get garbage collected.

Here is a quick video of the problem. Check the number of node which keeps increasing. Our app become unresponsive after only a couple of click on view mode icon.

https://imgur.com/a/HOxg5Sd

I have hard time thinking the problem comes from the library because literally everyone would create an issue since it's a pretty serious bug (at least for us) so there is a big chance I am doing something wrong

oliviertassinari commented 3 years ago

@mbret Definitely coming from Material-UI & JSS. It's fixed in v5.

mbret commented 3 years ago

v5 not being stable yet, is there really no way to get it fixed in current version ?

oliviertassinari commented 3 years ago

@mbret The only downside of v5 is that there are more breaking changes coming. No fix on v4 coming. We will automatically close all these issues https://github.com/mui-org/material-ui/labels/package%3A%20styles once v5 has made enough progress as they concern the soon to be legacy package @material-ui/styles.

mbret commented 3 years ago

I just tried the v5 (@next) and fixed the few things that were in breaking change and I can confirm the problem is still very present.

Is there anything specific that needs to be changed with v5 to fix the issue ?

oliviertassinari commented 3 years ago

Is there anything specific that needs to be changed with v5 to fix the issue ?

It should come for free, make sure you are no using makeStyles or withStyles with props, we are deprecating this API

mbret commented 3 years ago

make sure you are no using makeStyles or withStyles with props, we are deprecating this API

Indeed I am using them with props.

I was looking at the page and there is no mention of deprecation at https://next.material-ui.com/styles/basics/.

What do you suggest to use instead ?

oliviertassinari commented 3 years ago

I was looking at the page and there is no mention of deprecation at

True, we haven't had the chance to work on adding it yet. I would suggest using the styled API instead (currently exposed as experimentalStyled).

JeremyColton commented 3 years ago

Hi, I have found this thread since I have noticed my web-app popup on Android 8.x takes approx 8 seconds to render! The popup uses the Drawer component as the parent container. Inside is Cart, approx 50 Typography, approx 40 x Box, approx 40 x IconButton, approx 30 Chip components. When I throttle down my laptop's Chrome by 6x I see a similar slowness when the popup is opened.

I am using: "@material-ui/core": "4.11.0", "@material-ui/icons": "4.9.1",

My components are all functional components. I don't memoize them (shouldComponentUpdate for pure component??) since I see they are not re-rendering (I place breakpoints in each component and they are only hit once each) and each component contains unique props (via MobX).

I followed the Material example for styling and I use makeStyles and withStyles where needed. Eg

const useStyles = makeStyles((theme) => ({
    rowSummary: {
        '&:last-child': {
            fontWeight: 800,
        },
    },
}))

and

const StyledBadge = withStyles((theme) => ({
    badge: {
        right: 33,
        top: -10,
    },
}))(Badge)

The above examples pass the 'theme' props but it's not used.

This example below from my code uses the theme prop:

const useStyles = makeStyles((theme) => ({
    root: {
        padding: '10px 10px 0 10px',
        borderBottom: '10px solid #E7E7E7',
    },
    hasParentAccordion: {
        marginTop: '10px',
        borderLeftStyle: 'solid',
        borderLeftColor: theme.palette.primary.main,
        paddingTop: '0',
        paddingRight: '0',
        marginBottom: '10px',
    },
}))

In the Head tag I see 170 x

mui / material-ui

[styles] Performance issue when using styled components extensively #17370