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.

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

[styles] My app have memory leak on browser after using material-ui. #21528

Closed
2 tasks done
qhxin opened this issue Jun 21, 2020 · 40 comments
Closed
2 tasks done

[styles] My app have memory leak on browser after using material-ui. #21528

qhxin opened this issue Jun 21, 2020 · 40 comments
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. performance v4.x
Milestone

Comments

@qhxin
Copy link

qhxin commented Jun 21, 2020

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

Current Behavior 😯

My APP has memory leak on Firefox, after using material-ui.

Expected Behavior 🤔

My APP must render very frequently by our realtime data.

Steps to Reproduce 🕹

Steps:

  1. use makeStyles to create useStyles hook;
  2. use useStyles hook in components.
  3. re calculate components frequently.
  4. memory is rising.

Context 🔦

https://codesandbox.io/s/inspiring-waterfall-e197h?file=/src/App.js
codesandbox.io/s/dazzling-visvesvaraya-eczqe?file=/src/Tab.js

Your Environment 🌎

Edit @eps1lon: Added codesandbox.io/s/dazzling-visvesvaraya-eczqe?file=/src/Tab.js as repro.

@qhxin qhxin added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 21, 2020
@qhxin
Copy link
Author

qhxin commented Jun 21, 2020

performance on chrome:

屏幕快照 2020-06-21 下午1 12 30

memory info on firefox:

屏幕快照 2020-06-21 上午11 55 58

屏幕快照 2020-06-21 上午11 52 06

@oliviertassinari oliviertassinari added package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 22, 2020
@joshwooding
Copy link
Member

I can't recreate this using the codesandbox on Firefox 78.0b9 (64-bit) or Chrome 83.0.4103.116 (Official Build) (64-bit)
Windows 10 1903.

What environment did you encounter this in and can you definitely reproduce with the codesandbox?

@joshwooding joshwooding added the status: waiting for author Issue with insufficient information label Jun 22, 2020
@eps1lon
Copy link
Member

eps1lon commented Jun 23, 2020

For memory leak related issues please make sure to include whether

  1. this is affected by StrictMode or concurrent mode i.e. can this be reproduced regardless of the mode react is in
  2. this is reproducible with a production build. While a leak in development is problematic and indicative of an underlying issue it is far less severe than a leak in prod environments where older machines are more prevalent.

@qhxin
Copy link
Author

qhxin commented Jun 24, 2020

For memory leak related issues please make sure to include whether

  1. this is affected by StrictMode or concurrent mode i.e. can this be reproduced regardless of the mode react is in
  2. this is reproducible with a production build. While a leak in development is problematic and indicative of an underlying issue it is far less severe than a leak in prod environments where older machines are more prevalent.

@eps1lon tks for reply me, i hav'nt use StrictMode, and leak is caused in dev & prod Firefox/Safari env.

@joshwooding
Copy link
Member

@qhxin Can you post your version of Firefox and confirm you could reproduce this using the codesandbox?

@eps1lon
Copy link
Member

eps1lon commented Jun 25, 2020

@qhxin Can you post your version of Firefox and confirm you could reproduce this using the codesandbox?

I've forked it and deployed it: https://csb-qvhqs.netlify.app/. Please make sure that we know if this reproduces in prod or dev.

@joshwooding
Copy link
Member

Tried using @eps1lon's deployment to reproduce it and still can't after 4 hours. It's amazing to have a visual representation of how much production mode increases performance.

@qhxin
Copy link
Author

qhxin commented Jun 30, 2020

@joshwooding @eps1lon after long time debug, i found it's caused by my Tab style. The demo is: https://codesandbox.io/s/dazzling-visvesvaraya-eczqe?file=/src/Tab.js

This code is the reason that makes Firefox rule tree leak.


          "&:not(:first-child)": {
            borderLeft: "none"
          }

@qhxin
Copy link
Author

qhxin commented Jun 30, 2020

There are so many inline style sheets .

image

image

@qhxin
Copy link
Author

qhxin commented Jun 30, 2020

@oliviertassinari pls open this issue again.

@joshwooding joshwooding reopened this Jun 30, 2020
@joshwooding joshwooding removed the status: waiting for author Issue with insufficient information label Jun 30, 2020
@joshwooding
Copy link
Member

joshwooding commented Jun 30, 2020

I can reproduce the memory leak but I believe this is a jss issue.

Edit: This might be a duplicate of #21143

@eps1lon
Copy link
Member

eps1lon commented Jul 1, 2020

Edit: This might be a duplicate of #21143

Is about dynamic styles. https://codesandbox.io/s/dazzling-visvesvaraya-eczqe?file=/src/Tab.js only uses static styles.

Updated the initial post to use https://codesandbox.io/s/dazzling-visvesvaraya-eczqe?file=/src/Tab.js. The original sandbox could not reproduce the issue.

@qhxin
Copy link
Author

qhxin commented Jul 3, 2020

So how can i fix this?

@qhxin
Copy link
Author

qhxin commented Jul 6, 2020

okk, i think it's about jss plugin issue , reproduced in cssinjs/jss#1360

@oliviertassinari
Copy link
Member

An update, this issue is being resolved in v5 thanks to #22342 and the new @material-ui/styled-engine package.

@oliviertassinari oliviertassinari added this to the v5 milestone Nov 11, 2020
@oliviertassinari oliviertassinari changed the title My app have memory leak on browser after using material-ui. [styles] My app have memory leak on browser after using material-ui. Nov 11, 2020
@qhxin
Copy link
Author

qhxin commented Nov 16, 2020

An update, this issue is being resolved in v5 thanks to #22342 and the new @material-ui/styled-engine package.

is there any code example to fix this issue? upgrade @material/ui to v5 or other way?

@oliviertassinari
Copy link
Member

@qhxin You can use the new styled() method:

import { experimentalStyled as styled } from '@material-ui/core/styles';

It wraps emotion.

@qhxin
Copy link
Author

qhxin commented Nov 16, 2020

@qhxin You can use the new styled() method:

import { experimentalStyled as styled } from '@material-ui/core/styles';

It wraps emotion.

so, it will be a styled-component ---- not a react hook?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 16, 2020

@qhxin Yes, so far the plan is to remove the makeStyles and withStyles API progressively (v6).

@qhxin
Copy link
Author

qhxin commented Nov 16, 2020

@qhxin Yes, so far the plan is to remove the makeStyles and withStyles API progressively.

ohhh, it will cause many extra working

@mainfraame
Copy link

@oliviertassinari is makeStyles/withStyles going to be replaced with experimentalStyled?

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 8, 2020

@mentierd It's the long-term direction, yes, also replaced with the sx prop.

@oliviertassinari
Copy link
Member

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.

@jasonburnell98
Copy link

import { experimentalStyled as styled } from '@material-ui/core/styles';

using this causes this error:

Attempted import error: 'experimentalStyled' is not exported from '@material-ui/styled-engine' (imported as 'styled'). My current versions are :

  "@material-ui/styled-engine": "^5.0.0-alpha.34",
  "react": "^17.0.0",
  "react-scripts": "^4.0.3",
  "@material-ui/core": "^4.11.3",

Also, am using typescript and have added this in tsconfig.json :

"paths": {
"@material-ui/styled-engine": [
"./node_modules/@material-ui/styled-engine-sc"
]
}

Any help is appreciated.

@mnajdova
Copy link
Member

mnajdova commented Jun 1, 2021

@jasonburnell98 you can reference the examples we have in the repo for instructions on how to setup a starter project.

@jasonburnell98
Copy link

Hi @mnajdova , I will take a look however I am trying to implement this in an existing project

@jasonburnell98
Copy link

I am able to get this running fine in a startup project but whenever I try to migrate from makeStyles to styled in my existing project I run in to all sorts of dependency issues.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 3, 2021

I run into all sorts of dependency issues.

@jasonburnell98 What's the error message. Is it withStyles and makeStyles import related?

@mnajdova A thought: What if:

  • we had @material-ui/styles as a dependency of @material-ui/core,
  • we had makeStyles and withStyles still available for deep import with a deprecation message (but not available in the main barrel index)

Could it make the migration easier with a relatively low cost for us?

@jasonburnell98
Copy link

@oliviertassinari Yes it is with makeStyles, I fixed most of the dependency problems and after updating my project and running some of the migration scripts, for the files in node_modules, found in the docs (v4 to v5), I am now getting this error:

Failed to compile.

./node_modules/notistack/dist/notistack.esm.js
Attempted import error: 'makeStyles' is not exported from '@material-ui/core/styles'.

I'm going to look through the docs to see if there is some help for this specific error but I appreciate your staying in touch with me on this.

@mnajdova
Copy link
Member

mnajdova commented Jun 4, 2021

@mnajdova A thought: What if:

  • we had @material-ui/styles as a dependency of @material-ui/core,
  • we had makeStyles and withStyles still available for deep import with a deprecation message (but not available in the main barrel index)

Could it make the migration easier with a relatively low cost for us?

@oliviertassinari just to clarify, are you suggesting this only during the alpha/beta stage of v5? If so, yes I think we can add them. When you say deep import, do you mean this?

import { makeStyles, withStyles } from '@material-ui/core/styles;

Also, should we then just re-export them so that they won't have the default theme?


./node_modules/notistack/dist/notistack.esm.js
Attempted import error: 'makeStyles' is not exported from '@material-ui/core/styles'.

@jasonburnell98 see iamhosseindhv/notistack#387, hopefully I will manage to test out everything today and get it closer to the stage where it can be merged.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 4, 2021

@mnajdova My bad, I forgot that we don't import makeStyles and withStyles with a direct path. My proposal won't work 😁.

@jasonburnell98
Copy link

Thanks, @mnajdova , I'll review that, is there some sort of migration script/process I can refer to to update the imports to be from @material-ui/styles rather than @material-ui/core/styles or will notistack be removed ? as referenced from #26382

@jasonburnell98
Copy link

@mnajdova Or I suppose your plan was to update the import for notistack.

@jasonburnell98
Copy link

Thanks for getting back to me!

@MarvinThiele
Copy link

@oliviertassinari

Hello, I just had a very similar issue with , which caused a memory leak in our application where elements frequently changed due to live data streamed by socket-io. The changing elements were within the ThemeProvider. It took me a very long time to figure out that the ThemeProvider was causing these issues.

With ThemeProvider:
image

Without ThemeProvider:
image

It would be great if this issue could be fixed in the future because it is super unexpected and frustrating. I can also create a separate issue if that makes sense.

Used Version:
"@material-ui/core": "^4.5.0",
"@material-ui/data-grid": "^4.0.0-alpha.21",
"@material-ui/icons": "^4.11.2",
"@material-ui/lab": "^4.0.0-alpha.57",
"@material-ui/pickers": "^3.2.10",

@oliviertassinari
Copy link
Member

@MarvinThiele We have closed this issue because fixed in v5. Please upgrade and test the leak again.

@MarvinThiele
Copy link

@oliviertassinari

Thank you for the quick response. I tried your suggestion and the issue disappeared after updating.

I'm not sure, but such an issue might be worth a hotfix or patch on the v4.x release besides being fixed in the new v5 beta branch? This bug is potentially site-breaking and it is not clear that a ThemeProvider could cause such an issue.

Anyways, thanks for your help and I'm really grateful for your efforts with MaterialUI in general! :)

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 24, 2021

By default, we don't backport, we leverage these types of improvements as counter parts to breaking changes. We occasionally backport important changes, for instance, when we need them for internal purposes or when a third party introduces a regressions e.g. a change in Chrome.

@AdiGutner
Copy link

I'm experiencing this issue after migrating to @mui v5
The app worked perfectly with @material-ui v4
Any known issues or ideas what could be the RC?

@mnajdova
Copy link
Member

@AdiGutner can you share some more information? Are you still using makeStyles? If yes, that can be the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. performance v4.x
Projects
None yet
Development

No branches or pull requests

9 participants