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

[useMediaQuery] Workaround Safari wrong implementation of matchMedia #17315

Merged
merged 2 commits into from
Sep 5, 2019

Conversation

momentpaul
Copy link
Contributor

For useMediaQuery(), don't call setState() once unmounting begins. This may have happened in cases such as if a component unmounts when a window resizes.

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 4, 2019

Details of bundle changes.

Comparing: 1eff1cf...a52f004

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.01% 🔺 +0.01% 🔺 331,220 331,237 90,473 90,484
@material-ui/core/Paper 0.00% 0.00% 68,774 68,774 20,485 20,485
@material-ui/core/Paper.esm 0.00% 0.00% 62,148 62,148 19,214 19,214
@material-ui/core/Popper 0.00% 0.00% 28,466 28,466 10,188 10,188
@material-ui/core/Textarea 0.00% 0.00% 5,094 5,094 2,135 2,135
@material-ui/core/TrapFocus 0.00% 0.00% 3,834 3,834 1,617 1,617
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,385 16,385 5,827 5,827
@material-ui/core/useMediaQuery +0.67% 🔺 +0.85% 🔺 2,541 2,558 1,058 1,067
@material-ui/lab 0.00% 0.00% 153,209 153,209 46,684 46,684
@material-ui/styles 0.00% 0.00% 51,494 51,494 15,304 15,304
@material-ui/system 0.00% 0.00% 15,668 15,668 4,361 4,361
Button 0.00% 0.00% 78,663 78,663 24,048 24,048
Modal 0.00% 0.00% 14,335 14,335 5,013 5,013
Portal 0.00% 0.00% 2,907 2,907 1,322 1,322
Rating 0.00% 0.00% 70,016 70,016 21,864 21,864
Slider 0.00% 0.00% 74,282 74,282 23,015 23,015
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 52,253 52,253 13,780 13,780
docs.main 0.00% +0.01% 🔺 597,376 597,393 190,809 190,820
packages/material-ui/build/umd/material-ui.production.min.js +0.01% 🔺 +0.01% 🔺 302,086 302,103 86,797 86,803

Generated by 🚫 dangerJS against a52f004

@eps1lon
Copy link
Member

eps1lon commented Sep 4, 2019

Thank you for opening this pull request. Do you have a reproduction? I thought the handler won't be called once I called removeEventListener?

@momentpaul
Copy link
Contributor Author

Thank you for opening this pull request. Do you have a reproduction? I thought the handler won't be called once I called removeEventListener?

This issue seems to be happening in Safari and not Chrome, at least on macOS. It seems that the listener is being triggered after the last render, but before the cleanup.

Here is a sample Michael put together. In Safari, if you resize the window, you'll see a console error:
https://codepen.io/retval/pen/MWgOqpa

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 4, 2019

I could reproduce with the link. We might want to report this issue to the Safari team and add a comment about it, so we could remove the defensive check, at some point, in the future.

@oliviertassinari oliviertassinari changed the title useMediaQuery: don't call setState() once unmounting begins [useMediaQuery] Workaround Safari wrong implementation of matchMedia Sep 5, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 5, 2019

I have frozen the reproduction version of Material-UI to v4.4.0 so we can test the reproduction somewhere in the future without the fix and a more recent version of Safari: https://codepen.io/oliviertassinari/pen/mdbqNLQ.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check if this one of the cases where we should use the useSubscription hook. I think I remember that this was one of the use cases that hook already prevented.

@omBratteng
Copy link

I tried to run this on Safari 14.0 (16610.1.21.1.2), and I could not see any errors in the console. Except some errors I think might be from CodePen itself, so I tried in CodeSandBox, and works there too (https://codesandbox.io/s/runtime-wildflower-6bzns)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work hook: useMediaQuery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants