[useMediaQuery] hydrationCompleted is true before hydrated

[toddmazierski]

• 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 😯

The hydrationCompleted variable in useMediaQuery() can become true before all components in the document are actually hydrated.

Expected Behavior 🤔

useMediaQuery() should not have side-effects across components in a single document.

Steps to Reproduce 🕹

https://codesandbox.io/s/sweet-wilbur-5t0l6

In this sandbox, please observe that hydrationCompleted is set to true after ReactDOM.render() (app 1) but before ReactDOM.hydrate() (app 2).

Unfortunately, I'm having trouble reproducing the visual bug in our app that caused us to take notice of this issue in the first place. I'll keep at it!

Context 🔦

The impact on our app is Prop `className` did not match warnings and mismatches between CSS class names.

I'm able to work around the issue by:

• changing the load order so the SSR component is hydrated first
• manually resetting hydrationCompleted back to false after it's set to true
• forcing the app to re-render (by interacting with the app)

Your Environment 🌎

Tech	Version
Material-UI	v4.5.0
React	v16.8.6

[eps1lon]

These singleton variables are always problematic since they assume a whole lot about app and react implementation details.

I don't think these variables are ever correct. What we should do is identify if we actually can use the browser API during render or not. And I think the answer is No for all media queries.

An argument can be made to read it during render since it's very unlikely that these value change between render and commit which would produce an inconsistent API.

Either way in your particular example useMediaQuery doesn't seem appropriate in the first place. You can use css media queries within the styles declaration.

[oliviertassinari]

@eps1lon What do you think of removing this hydrationCompleted logic?

I think that it will make the logic behave consistently between two renders, the source will be simpler to follow, it would solve this edge case, and hopefully be more resilient to future React changes.

[oliviertassinari]

@toddmazierski We have discussed this concern a bit internally. What do you think of this patch?

diff --git a/packages/material-ui/src/useMediaQuery/useMediaQuery.js b/packages/material-ui/src/useMediaQuery/useMediaQuery.js
index e969caa2f..2cb368de0 100644
--- a/packages/material-ui/src/useMediaQuery/useMediaQuery.js
+++ b/packages/material-ui/src/useMediaQuery/useMediaQuery.js
@@ -1,9 +1,6 @@
 import React from 'react';
 import { getThemeProps, useTheme } from '@material-ui/styles';

-// This variable will be true once the server-side hydration is completed.
-let hydrationCompleted = false;
-
 function useMediaQuery(queryInput, options = {}) {
   const theme = useTheme();
   const props = getThemeProps({
@@ -40,7 +37,7 @@ function useMediaQuery(queryInput, options = {}) {
   };

   const [match, setMatch] = React.useState(() => {
-    if ((hydrationCompleted || noSsr) && supportMatchMedia) {
+    if (noSsr && supportMatchMedia) {
       return window.matchMedia(query).matches;
     }
     if (ssrMatchMedia) {
@@ -54,7 +51,6 @@ function useMediaQuery(queryInput, options = {}) {

   React.useEffect(() => {
     let active = true;
-    hydrationCompleted = true;

     if (!supportMatchMedia) {
       return undefined;
@@ -80,8 +76,4 @@ function useMediaQuery(queryInput, options = {}) {
   return match;
 }

-export function testReset() {
-  hydrationCompleted = false;
-}
-
 export default useMediaQuery;

[toddmazierski]

@toddmazierski We have discussed this concern a bit internally. What do you think of this patch?

Love it. It's less code, and I've confirmed this patch fixes the edge case in our app. Thank you, @oliviertassinari and @eps1lon. 👍

[oliviertassinari]

@toddmazierski Do you want to submit a pull request? We would still need to update the tests to have it land :).

[toddmazierski]

@toddmazierski Do you want to submit a pull request? We would still need to update the tests to have it land :).

You got it! Please see #18683.