material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

Published 20 hours ago • verified publisher icon mui

[docs] Improving the sx prop documentation

Open jisotalo opened this issue 9 months ago • 4 comments

Related page

https://mui.com/system/getting-started/the-sx-prop

Kind of issue

Missing information

Issue description

Currently, the sx prop documentation has no information about possible issues with memory usage when dealing with dynamic values.

Background

I recently encountered a memory issue, which looked like a memory leak.

After long investigation, the reason was following component, that was created based on StackOverflow answer. The component was a custom LinearProgress that was rendered vertically.

<LinearProgressWithLabel
  variant="determinate"
  color="primary"
  value={activeValue}
  sx={{
    [`& .${linearProgressClasses.bar}`]: {
      transform: `translateY(${100 - activeValue}%)!important`
    },
    ml: 'auto',
    mr: 'auto'
  }}
/>

At first, I thought it was a memory leak in MUI component. However, after investigation, I found the following discussions:

  • https://stackoverflow.com/questions/72527461/when-should-i-use-style-instead-of-sx-prop-in-material-ui/72527462#72527462
  • https://github.com/mui/material-ui/issues/36294#issuecomment-1462495889

If I understood correctly, the sx should not be used like this with a dynamic value. Using it like this causes more and more <style> tags created in the page.

As the activeValue is a constantly changing decimal value between 0 and 100, it has thousands and thousands of possible values in between. With multiple LinearProgressWithLabel components , it eventially caused hundreds of thousands of generated <style> tags added in the page <head>.

image

EDIT: I just noticed 30 minutes later, that in production version, there aren't multiple <style> tags. It seems they are combined in production. However the memory issue is the same in both production and development, it's just not visible in devtools DOM.

This eventually crashed the web browser, as the application is a long-running kiosk app. Even when user doesn't touch the page at all. The web browser memory usage increases from 200mb to 2000mb in just around 24 hours.

I fixed this issue by recreating the component from scratch (and also used style prop instead).

Suggestion

I suggest the sx prop page should be updated to warn about using dynamic values. As @markmanx commented in 2023, there seems to be no guidance for this.

  • Adding information of the correct sx prop use cases.
  • Explaining that that each generated <style> is kept in DOM and memory.
  • Adding a warning, that having constantly changing (random) dynamic values inside sx prop could cause heavy increase in memory usage in the long run, and in the worst case, a browser crash.
  • Explaining, that the style prop should be used instead of sx in situations like this.

I would also like to have a confirmation if my conclusion is correct.

Context

No response

Search keywords: sx, memory, memory leak, performance

jisotalo avatar May 08 '24 11:05 jisotalo

I agree with the general sentiments here. This particular example is an extreme case though and we should mention such cases in our docs. In this case, it might be beneficial to just use inline style with a css variable so that the generated style doesn't need to change with each change in the value. We might have to take a perf for such similar cases and weed out actual issue as I also think it might not always be related to the dynamic values (unless the value here changes at the decimal level as well instead of whole numbers 1-100).

cc: @mnajdova Your thoughts here ?

brijeshb42 avatar May 09 '24 07:05 brijeshb42

use inline style with a css variable so that the generated style doesn't need to change with each change in the value.

This is what I would propose too. I remember similar issues in the past: https://github.com/mui/material-ui/issues/36749, https://github.com/mui/material-ui/issues/36294. @aarongarciah would you be up to updating the docs to include this information?

mnajdova avatar May 14 '24 10:05 mnajdova

@mnajdova I'll do it 👍

aarongarciah avatar May 14 '24 11:05 aarongarciah

Docs improvements up for review https://github.com/mui/material-ui/pull/42239

aarongarciah avatar May 14 '24 16:05 aarongarciah