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

Material UI styled component not releasing DOM Nodes after rerender #36294

Closed
2 tasks done
handoiditu1508 opened this issue Feb 22, 2023 · 6 comments
Closed
2 tasks done
Assignees
Labels
external dependency Blocked by external dependency, we can’t do anything about it package: system Specific to @mui/system

Comments

@handoiditu1508
Copy link

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Repository to live example: https://github.com/handoiditu1508/mui-memory-leak-bug

Steps:

  1. npx create-react-app my-app --template typescript
  2. cd my-app
  3. npm install @emotion/react @emotion/styled @mui/material
  4. update App.tsx:
import { Box, Button, styled } from "@mui/material";
import { useState } from 'react';
import './App.css';

type MyModel = {
  id: number;
  color: string;
}

const getRandomColor = (): string => {
  var letters = '0123456789ABCDEF';
  var color = '#';
  for (var i = 0; i < 6; i++) {
    color += letters[Math.floor(Math.random() * 16)];
  }
  return color;
}

const StyledBox1 = styled(Box)(({ theme }) => ({
  backgroundColor: "red"
}));

const StyledBox2 = styled(Box, { shouldForwardProp: prop => prop !== "currentColor" })<{
  currentColor: string;
}>(({ theme, currentColor }) => ({
  backgroundColor: currentColor
}));

function App() {
  const [models, setModels] = useState<MyModel[]>([
    { id: 1, color: getRandomColor() },
    { id: 2, color: getRandomColor() },
    { id: 3, color: getRandomColor() },
    { id: 4, color: getRandomColor() },
    { id: 5, color: getRandomColor() },
    { id: 6, color: getRandomColor() },
    { id: 7, color: getRandomColor() },
    { id: 8, color: getRandomColor() },
    { id: 9, color: getRandomColor() },
    { id: 10, color: getRandomColor() },
  ]);

  const clickHandler = () => {
    const newModels: MyModel[] = [...models];
    newModels[0].color = getRandomColor();
    setModels(newModels);
  }

  return (
    <div className="App">
      {/* ✅ */}
      {/* {models.map(model => <Box key={model.id}>{model.id}</Box>)} */}

      {/* ✅ */}
      {/* {models.map(model => <StyledBox1 key={model.id}>{model.id}</StyledBox1>)} */}

      {/* ❌ */}
      {models.map(model => <StyledBox2 key={model.id} currentColor={model.color}>{model.id}</StyledBox2>)}

      {/* ❌ */}
      {/* <StyledBox2 currentColor={models[0].color}>{models[0].id}</StyledBox2>
      <StyledBox2 currentColor={models[1].color}>{models[1].id}</StyledBox2>
      <StyledBox2 currentColor={models[2].color}>{models[2].id}</StyledBox2>
      <StyledBox2 currentColor={models[3].color}>{models[3].id}</StyledBox2>
      <StyledBox2 currentColor={models[4].color}>{models[4].id}</StyledBox2>
      <StyledBox2 currentColor={models[5].color}>{models[5].id}</StyledBox2>
      <StyledBox2 currentColor={models[6].color}>{models[6].id}</StyledBox2>
      <StyledBox2 currentColor={models[7].color}>{models[7].id}</StyledBox2>
      <StyledBox2 currentColor={models[8].color}>{models[8].id}</StyledBox2>
      <StyledBox2 currentColor={models[9].color}>{models[9].id}</StyledBox2> */}

      {/* ❌ */}
      {/* <Box sx={{ backgroundColor: models[0].color }}>{models[0].id}</Box>
      <Box sx={{ backgroundColor: models[1].color }}>{models[1].id}</Box>
      <Box sx={{ backgroundColor: models[2].color }}>{models[2].id}</Box>
      <Box sx={{ backgroundColor: models[3].color }}>{models[3].id}</Box>
      <Box sx={{ backgroundColor: models[4].color }}>{models[4].id}</Box>
      <Box sx={{ backgroundColor: models[5].color }}>{models[5].id}</Box>
      <Box sx={{ backgroundColor: models[6].color }}>{models[6].id}</Box>
      <Box sx={{ backgroundColor: models[7].color }}>{models[7].id}</Box>
      <Box sx={{ backgroundColor: models[8].color }}>{models[8].id}</Box>
      <Box sx={{ backgroundColor: models[9].color }}>{models[9].id}</Box> */}

      {/* ✅ */}
      {/* <div style={{ backgroundColor: models[0].color }}>{models[0].id}</div>
      <div style={{ backgroundColor: models[1].color }}>{models[1].id}</div>
      <div style={{ backgroundColor: models[2].color }}>{models[2].id}</div>
      <div style={{ backgroundColor: models[3].color }}>{models[3].id}</div>
      <div style={{ backgroundColor: models[4].color }}>{models[4].id}</div>
      <div style={{ backgroundColor: models[5].color }}>{models[5].id}</div>
      <div style={{ backgroundColor: models[6].color }}>{models[6].id}</div>
      <div style={{ backgroundColor: models[7].color }}>{models[7].id}</div>
      <div style={{ backgroundColor: models[8].color }}>{models[8].id}</div>
      <div style={{ backgroundColor: models[9].color }}>{models[9].id}</div> */}

      <Button variant="contained" onClick={clickHandler}>Update Color</Button>
    </div>
  );
}

export default App;

my package.json file:

{
  "name": "my-app",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "@emotion/react": "^11.10.6",
    "@emotion/styled": "^11.10.6",
    "@mui/material": "^5.11.9",
    "@testing-library/jest-dom": "^5.16.5",
    "@testing-library/react": "^13.4.0",
    "@testing-library/user-event": "^13.5.0",
    "@types/jest": "^27.5.2",
    "@types/node": "^16.18.12",
    "@types/react": "^18.0.28",
    "@types/react-dom": "^18.0.11",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "react-scripts": "5.0.1",
    "typescript": "^4.9.5",
    "web-vitals": "^2.1.4"
  },
  "scripts": {
    "start": "react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test",
    "eject": "react-scripts eject"
  },
  "eslintConfig": {
    "extends": [
      "react-app",
      "react-app/jest"
    ]
  },
  "browserslist": {
    "production": [
      ">0.2%",
      "not dead",
      "not op_mini all"
    ],
    "development": [
      "last 1 chrome version",
      "last 1 firefox version",
      "last 1 safari version"
    ]
  }
}
  1. npm start
  2. click the only button on the screen multiple times to cause the components to rerender and watch Performance monitor on browser

I using Microsoft Edge Version 110.0.1587.50 (Official build) (64-bit).
I didn't test on other browser.

Current behavior 😯

The DOM Nodes keep increasing without decreasing, in the pictures, DOM Nodes start at 182, I click the button multiple times to cause components trigger rerender gradually, after few minutes, DOM Nodes can reach 1710.
This is just a very simple app with no special effect.
Screenshot 2023-02-22 202412
Screenshot 2023-02-22 202642

Expected behavior 🤔

The DOM Nodes should be released after sometime.

Context 🔦

When I test it with non-styled or styled component without shouldForwardProp option, the DOM Nodes are released after few minutes. (See all the cases I tested in my App.tsx file)

This might doesn't seem that bad, just more than 1000 DOM Nodes unreleased.

But I'm having an app with multiple components that use styled() function with shouldForwardProp option and some MUISlider with very small step, those styled components will rerender whenever I change the Slider value. My app can reach up to 50.000+ DOM Nodes easily and getting lagger everytime I change the slider value.

Your environment 🌎

npx @mui/envinfo
  System:
    OS: Windows 10 10.0.19044
  Binaries:
    Node: 18.12.1 - D:\ProgramFiles\nodejs\node.EXE
    Yarn: Not Found
    npm: 8.19.2 - D:\ProgramFiles\nodejs\npm.CMD   
  Browsers:
    Chrome: Not Found
    Edge: Spartan (44.19041.1266.0), Chromium (110.0.1587.50)
  npmPackages:
    @emotion/react: ^11.10.6 => 11.10.6
    @emotion/styled: ^11.10.6 => 11.10.6
    @mui/base:  5.0.0-alpha.118
    @mui/core-downloads-tracker:  5.11.9
    @mui/material: ^5.11.9 => 5.11.9
    @mui/private-theming:  5.11.9
    @mui/styled-engine:  5.11.9
    @mui/system:  5.11.9
    @mui/types:  7.2.3
    @mui/utils:  5.11.9
    @types/react: ^18.0.28 => 18.0.28
    react: ^18.2.0 => 18.2.0
    react-dom: ^18.2.0 => 18.2.0
    typescript: ^4.9.5 => 4.9.5
tsconfig.json
{
  "compilerOptions": {
    "target": "es5",
    "lib": [
      "dom",
      "dom.iterable",
      "esnext"
    ],
    "allowJs": true,
    "skipLibCheck": true,
    "esModuleInterop": true,
    "allowSyntheticDefaultImports": true,
    "strict": true,
    "forceConsistentCasingInFileNames": true,
    "noFallthroughCasesInSwitch": true,
    "module": "esnext",
    "moduleResolution": "node",
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react-jsx"
  },
  "include": [
    "src"
  ]
}

@handoiditu1508 handoiditu1508 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 22, 2023
@zannager zannager added the package: system Specific to @mui/system label Feb 23, 2023
@mnajdova
Copy link
Member

Can you please share a repository that can be cloned? I know you have listed all steps, but it is much easier for maintainers if we don't need to set up from start new project for every issue. Sharing a codesandbox would also work.

@handoiditu1508
Copy link
Author

Can you please share a repository that can be cloned? I know you have listed all steps, but it is much easier for maintainers if we don't need to set up from start new project for every issue. Sharing a codesandbox would also work.

sure, here the link to my repository: https://github.com/handoiditu1508/mui-memory-leak-bug

@mnajdova
Copy link
Member

Perfect, thanks! I will have a look tomorrow.

@mnajdova
Copy link
Member

mnajdova commented Mar 9, 2023

I can't reproduce this in production. However, the styles changing on each drag means that new class names are being generated. Honestly I would not recommend doing this, if a style depends on something this dynamic, it should probably be defined inside the style prop.

Sorry for tagging you earlier @Andarist :D

@mnajdova mnajdova added external dependency Blocked by external dependency, we can’t do anything about it and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Mar 9, 2023
@handoiditu1508
Copy link
Author

Thanks @mnajdova! You are right, after I update my code to use style prop, the DOM Nodes are released normally.
It make more sense to me now that I know that new css class is created if the style in styled function or sx prop is different.

@markmanx
Copy link

markmanx commented Oct 9, 2023

I can't reproduce this in production. However, the styles changing on each drag means that new class names are being generated. Honestly I would not recommend doing this, if a style depends on something this dynamic, it should probably be defined inside the style prop.

Sorry for tagging you earlier @Andarist :D

Thanks @mnajdova for looking into this. Is the guidance then not to use the sx prop for dynamic styles? If so, is this documented anywhere as a massive potential pitfall (people unknowingly demolishing their app's performance). Also, do we know why retaining just the styles cause the DOM node to be retained itself? Or is the generated styles node the DOM node itself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external dependency Blocked by external dependency, we can’t do anything about it package: system Specific to @mui/system
Projects
None yet
Development

No branches or pull requests

4 participants