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

setState leak after unmount #150

Closed
oliviertassinari opened this issue Aug 4, 2020 · 5 comments · Fixed by #152 or #179
Closed

setState leak after unmount #150

oliviertassinari opened this issue Aug 4, 2020 · 5 comments · Fixed by #152 or #179
Labels
bug 🐛 Something doesn't work
Milestone

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 4, 2020

@dtassone The issue was caught in the CI (Karma), something is calling setState after the component is unmounted. It leaks.

Capture d’écran 2020-08-05 à 00 32 02


@eps1lon this makes me think that we might have an improvement opportunity in the CI, shouldn't we fail if a component call console.warn or console.error during the tests? I believe it works when we run the tests in JSDOM, but not with Karma. I don't know why yet. The data grid might be an exception as all JSDOM tests are skipped (require layout).

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 4, 2020

@eps1lon Actually, I think that it's a regression from mui/material-ui#21581.

  1. test/utils/init.js runs for karma and jsdom
  2. test/utils/setup.js runs for jsdom only

We moved the console throw logic from 1. to 2. hence left karma without coverage

@eps1lon
Copy link
Member

eps1lon commented Aug 4, 2020

We moved the console throw logic from 1. to 2. hence left karma without coverage

If karma had no warn/error logic then every test using toWarnDev/toErrorDev would throw.

Why would code from the main repo affect code in this repo?

@eps1lon
Copy link
Member

eps1lon commented Aug 4, 2020

Yeah so I had to investigate this more since you didn't communicate what you discovered: It's about unexpected calls not covering console warn/error, right? We should move this around, yes. PRs welcome.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Aug 4, 2020

@eps1lon I will open a PR :)

@oliviertassinari
Copy link
Member Author

Fixed in mui/material-ui#22075

oliviertassinari added a commit to oliviertassinari/mui-x that referenced this issue Aug 5, 2020
@oliviertassinari oliviertassinari added this to the Post MVP milestone Aug 5, 2020
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Aug 8, 2020
oliviertassinari added a commit to oliviertassinari/mui-x that referenced this issue Aug 16, 2020
oliviertassinari added a commit to oliviertassinari/mui-x that referenced this issue Aug 16, 2020
oliviertassinari added a commit to oliviertassinari/mui-x that referenced this issue Aug 16, 2020
oliviertassinari added a commit to oliviertassinari/mui-x that referenced this issue Aug 16, 2020
oliviertassinari added a commit that referenced this issue Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work
Projects
None yet
2 participants