Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[docs] Improving the sx prop documentation #42164

Closed
jisotalo opened this issue May 8, 2024 · 4 comments · Fixed by #42239
Closed

[docs] Improving the sx prop documentation #42164

jisotalo opened this issue May 8, 2024 · 4 comments · Fixed by #42239
Assignees
@aarongarciah
Labels
docs Improvements or additions to the documentation package: system Specific to @mui/system support: docs-feedback Feedback from documentation page

Comments

@jisotalo
Copy link

jisotalo commented May 8, 2024 •
edited
Loading

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:

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 jisotalo added status: waiting for maintainer These issues haven't been looked at yet by a maintainer support: docs-feedback Feedback from documentation page labels May 8, 2024
@zannager zannager added docs Improvements or additions to the documentation package: system Specific to @mui/system labels May 8, 2024
@brijeshb42
Copy link
Contributor

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 brijeshb42 removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label May 9, 2024
@mnajdova
Copy link
Member

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: #36749, #36294. @aarongarciah would you be up to updating the docs to include this information?

@mnajdova mnajdova assigned aarongarciah and unassigned brijeshb42 May 14, 2024
@aarongarciah
Copy link
Member

@mnajdova I'll do it 👍

@aarongarciah aarongarciah mentioned this issue May 14, 2024
Merged
@aarongarciah
Copy link
Member

Docs improvements up for review #42239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aarongarciah aarongarciah

Labels
docs Improvements or additions to the documentation package: system Specific to @mui/system support: docs-feedback Feedback from documentation page
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[system][docs] Add "dynamic values" section to sx prop page aarongarciah/material-ui
5 participants
@brijeshb42 @mnajdova @aarongarciah @jisotalo @zannager