[NoSsr] Fix defer prop regression

I suspect an issue in React itself. It was working a year ago.
mui · Oct 18, 2019 · 7115cc2 · 7115cc2
1 parent 6ab45d2
commit 7115cc2
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 78 deletions.
diff --git a/docs/src/pages/components/no-ssr/FrameDeferring.js b/docs/src/pages/components/no-ssr/FrameDeferring.js
@@ -4,16 +4,17 @@ import NoSsr from '@material-ui/core/NoSsr';
 
 const useStyles = makeStyles({
   container: {
-    maxWidth: 300,
-    wordBreak: 'break-all',
+    width: 300,
+    display: 'flex',
+    flexWrap: 'wrap',
   },
 });
 
 function LargeTree() {
-  return Array.from(new Array(3000)).map((_, index) => <span key={index}>.</span>);
+  return Array.from(new Array(5000)).map((_, index) => <span key={index}>.</span>);
 }
 
-function FrameDeferring() {
+export default function FrameDeferring() {
   const classes = useStyles();
   const [state, setState] = React.useState({ open: false, defer: false });
 
@@ -30,6 +31,7 @@ function FrameDeferring() {
       >
         {'Render NoSsr defer="false"'}
       </button>
+      <br />
       <button
         type="button"
         onClick={() =>
@@ -41,17 +43,19 @@ function FrameDeferring() {
       >
         {'Render NoSsr defer="true"'}
       </button>
-      {state.open ? (
-        <div className={classes.container}>
-          <span>Outside NoSsr</span>
-          <NoSsr defer={state.defer}>
-            ....Inside NoSsr
-            <LargeTree />
-          </NoSsr>
-        </div>
-      ) : null}
+      <br />
+      <br />
+      <div className={classes.container}>
+        {state.open ? (
+          <React.Fragment>
+            <div>Outside NoSsr</div>
+            <NoSsr defer={state.defer}>
+              .....Inside NoSsr
+              <LargeTree />
+            </NoSsr>
+          </React.Fragment>
+        ) : null}
+      </div>
     </div>
   );
 }
-
-export default FrameDeferring;
diff --git a/docs/src/pages/components/no-ssr/FrameDeferring.tsx b/docs/src/pages/components/no-ssr/FrameDeferring.tsx
@@ -4,16 +4,17 @@ import NoSsr from '@material-ui/core/NoSsr';
 
 const useStyles = makeStyles({
   container: {
-    maxWidth: 300,
-    wordBreak: 'break-all',
+    width: 300,
+    display: 'flex',
+    flexWrap: 'wrap',
   },
 });
 
 function LargeTree(): any {
-  return Array.from(new Array(3000)).map((_, index) => <span key={index}>.</span>);
+  return Array.from(new Array(5000)).map((_, index) => <span key={index}>.</span>);
 }
 
-function FrameDeferring() {
+export default function FrameDeferring() {
   const classes = useStyles();
   const [state, setState] = React.useState({ open: false, defer: false });
 
@@ -30,6 +31,7 @@ function FrameDeferring() {
       >
         {'Render NoSsr defer="false"'}
       </button>
+      <br />
       <button
         type="button"
         onClick={() =>
@@ -41,17 +43,19 @@ function FrameDeferring() {
       >
         {'Render NoSsr defer="true"'}
       </button>
-      {state.open ? (
-        <div className={classes.container}>
-          <span>Outside NoSsr</span>
-          <NoSsr defer={state.defer}>
-            ....Inside NoSsr
-            <LargeTree />
-          </NoSsr>
-        </div>
-      ) : null}
+      <br />
+      <br />
+      <div className={classes.container}>
+        {state.open ? (
+          <React.Fragment>
+            <div>Outside NoSsr</div>
+            <NoSsr defer={state.defer}>
+              .....Inside NoSsr
+              <LargeTree />
+            </NoSsr>
+          </React.Fragment>
+        ) : null}
+      </div>
     </div>
   );
 }
-
-export default FrameDeferring;
diff --git a/docs/src/pages/components/no-ssr/SimpleNoSsr.js b/docs/src/pages/components/no-ssr/SimpleNoSsr.js
@@ -1,35 +1,18 @@
 import React from 'react';
-import PropTypes from 'prop-types';
-import { withStyles } from '@material-ui/core/styles';
 import NoSsr from '@material-ui/core/NoSsr';
-import Button from '@material-ui/core/Button';
-
-const styles = theme => ({
-  button: {
-    display: 'block',
-    margin: theme.spacing(2),
-  },
-});
-
-function SimpleNoSsr(props) {
-  const { classes } = props;
+import Box from '@material-ui/core/Box';
 
+export default function SimpleNoSsr() {
   return (
     <div>
-      <Button className={classes.button} variant="contained" color="primary">
-        Server & Client
-      </Button>
+      <Box p={2} bgcolor="primary.main" color="primary.contrastText">
+        Server and Client
+      </Box>
       <NoSsr>
-        <Button className={classes.button} variant="contained" color="secondary">
+        <Box p={2} bgcolor="secondary.main" color="primary.contrastText">
           Client only
-        </Button>
+        </Box>
       </NoSsr>
     </div>
   );
 }
-
-SimpleNoSsr.propTypes = {
-  classes: PropTypes.object.isRequired,
-};
-
-export default withStyles(styles)(SimpleNoSsr);
diff --git a/docs/src/pages/components/no-ssr/SimpleNoSsr.tsx b/docs/src/pages/components/no-ssr/SimpleNoSsr.tsx
@@ -1,32 +1,18 @@
 import React from 'react';
-import { withStyles, WithStyles } from '@material-ui/core/styles';
 import NoSsr from '@material-ui/core/NoSsr';
-import Button from '@material-ui/core/Button';
-
-const styles = (theme: any) => ({
-  button: {
-    display: 'block',
-    margin: theme.spacing(2),
-  },
-});
-
-export interface iProps extends WithStyles<typeof styles> {}
-
-function SimpleNoSsr(props: iProps) {
-  const { classes } = props;
+import Box from '@material-ui/core/Box';
 
+export default function SimpleNoSsr() {
   return (
     <div>
-      <Button className={classes.button} variant="contained" color="primary">
-        Server & Client
-      </Button>
+      <Box p={2} bgcolor="primary.main" color="primary.contrastText">
+        Server and Client
+      </Box>
       <NoSsr>
-        <Button className={classes.button} variant="contained" color="secondary">
+        <Box p={2} bgcolor="secondary.main" color="primary.contrastText">
           Client only
-        </Button>
+        </Box>
       </NoSsr>
     </div>
   );
 }
-
-export default withStyles(styles)(SimpleNoSsr);
diff --git a/packages/material-ui/src/NoSsr/NoSsr.js b/packages/material-ui/src/NoSsr/NoSsr.js
@@ -19,6 +19,7 @@ const useEnhancedEffect =
 function NoSsr(props) {
   const { children, defer = false, fallback = null } = props;
   const [mountedState, setMountedState] = React.useState(false);
+  const mounted = React.useRef();
 
   useEnhancedEffect(() => {
     if (!defer) {
@@ -28,8 +29,31 @@ function NoSsr(props) {
 
   React.useEffect(() => {
     if (defer) {
-      setMountedState(true);
+      mounted.current = true;
+      // Wondering why we use two RAFs? Check this video out:
+      // https://www.youtube.com/watch?v=cCOL7MC4Pl0
+      //
+      // The useEffect() method is called after the DOM nodes are inserted.
+      // The UI might not have rendering the changes. We request a frame.
+      requestAnimationFrame(() => {
+        // The browser should be about to render the DOM nodes
+        // that React committed at this point.
+        // We don't want to interrupt. Let's wait the next frame.
+        requestAnimationFrame(() => {
+          // The UI is up-to-date at this point.
+          // We can continue rendering the children.
+          if (mounted.current) {
+            setMountedState(true);
+          }
+        });
+      });
+
+      return () => {
+        mounted.current = false;
+      };
     }
+
+    return undefined;
   }, [defer]);
 
   // We need the Fragment here to force react-docgen to recognise NoSsr as a component.

diff --git a/packages/material-ui/src/NoSsr/NoSsr.test.js b/packages/material-ui/src/NoSsr/NoSsr.test.js
@@ -52,13 +52,18 @@ describe('<NoSsr />', () => {
   });
 
   describe('prop: defer', () => {
-    it('should defer the rendering', () => {
+    it('should defer the rendering', done => {
       const wrapper = mount(
         <NoSsr defer>
           <span id="client-only">Hello</span>
         </NoSsr>,
       );
-      assert.strictEqual(wrapper.find('#client-only').exists(), true);
+      assert.strictEqual(wrapper.find('#client-only').exists(), false);
+      setTimeout(() => {
+        wrapper.update();
+        assert.strictEqual(wrapper.find('#client-only').exists(), true);
+        done();
+      }, 300);
     });
   });
 });