BpkBreakpoint: Fix Server Side Rendering (#3299)

* BpkBreakpoint: Fix & Optimize SSR rendering * small tweak to useMediaQuery + fix tests * Re-add snapshot * Remove isClient block within useMediaQuery useEffect * mock useMediaQuery in tests * remove console log * Add to readme * Improve guidance * Update type and README * Small correction to guidance snipper * Add dummy import * Removed isClient from dependency array
Skyscanner · Mar 26, 2024 · fb45a6c · fb45a6c
1 parent 1e802fe
commit fb45a6c
Show file tree

Hide file tree

Showing 13 changed files with 199 additions and 116 deletions.
diff --git a/packages/bpk-component-bottom-sheet/src/BpkBottomSheet-test.tsx b/packages/bpk-component-bottom-sheet/src/BpkBottomSheet-test.tsx
@@ -21,7 +21,8 @@ import { renderToString } from 'react-dom/server';
 import { render } from '@testing-library/react';
 
 import BpkBottomSheet from './BpkBottomSheet';
-
+// mock breakpoint to always match
+jest.mock('../../bpk-component-breakpoint/src/useMediaQuery', () => jest.fn(() => true));
 describe('BpkBottomSheet', () => {
   it('renders without crashing with all props', () => {
     expect(() => renderToString(

diff --git a/packages/bpk-component-bottom-sheet/src/accessibility-test.tsx b/packages/bpk-component-bottom-sheet/src/accessibility-test.tsx
@@ -20,7 +20,8 @@ import { render } from '@testing-library/react';
 import { axe } from 'jest-axe';
 
 import BpkBottomSheet from './BpkBottomSheet';
-
+// mock breakpoint to always match
+jest.mock('../../bpk-component-breakpoint/src/useMediaQuery', () => jest.fn(() => true));
 describe('BpkBottomSheet accessibility tests', () => {
   it('should not have programmatically-detectable accessibility issues', async () => {
     const { container } = render(

diff --git a/packages/bpk-component-breakpoint/README.md b/packages/bpk-component-breakpoint/README.md
@@ -24,17 +24,57 @@ export default () => (
 
 ### Server Side Render (SSR) Support
 
-You can pass `matchSSR` which will instruct the breakpoint to match any time it is rendered on the server.
+You can pass `matchSSR` which will instruct the breakpoint to match any time it is rendered on the server. You can use
+this to estimate what breakpoint is likely to match on the client-side.
 
 ```js
-<BpkBreakpoint query={BREAKPOINTS.TABLET} matchSSR>
+import { isTablet, isMobilePhone } from 'some-device-detection';
+
+<BpkBreakpoint query={BREAKPOINTS.TABLET} matchSSR={isTablet}>
   <span>Tablet viewport is active OR we are rendering on the server-side</span>
 </BpkBreakpoint>
-<BpkBreakpoint query={BREAKPOINTS.MOBILE}>
+<BpkBreakpoint query={BREAKPOINTS.MOBILE} matchSSR={isMobilePhone}>
   <span>Mobile viewport is active AND we are rendering on the client-side</span>
 </BpkBreakpoint>
 ```
 
+If you match to a different breakpoint when rendering on the server, than what is matched to in the traveller's browser,
+then React will print a warning saying there is a mismatch.
+
+### Testing
+
+When writing tests for any components that use BpkBreakpoint, you will have to mock either the `BpkBreakpoint` component
+or the underlying `useMediaQuery`. This is because the `window.matchMedia` function that we rely on does not exist in
+the jest testing environment.
+
+A mock were you only wanted your mobile BpkBreakpoint to render:
+```js
+import { useMediaQuery, BREAKPOINTS } from '@skyscanner/backpack-web/bpk-component-breakpoint';
+
+jest.mock('@skyscanner/backpack-web/bpk-component-breakpoint', () => {
+  __esModule: true,
+  ...jest.requireActual('@skyscanner/backpack-web/bpk-component-breakpoint'),
+  useMediaQuery: jest.fn(),
+});
+describe('tests', () => {
+  it('my test', () => {
+    (useMediaQuery as jest.Mock).mockImplementation(
+      (query: string) => query === BREAKPOINTS.MOBILE,
+    );
+  })
+})
+```
+
+A simpler mock were you want all BpkBreakpoints to render:
+```js
+jest.mock('@skyscanner/backpack-web/bpk-component-breakpoint', () => {
+  __esModule: true,
+  ...jest.requireActual('@skyscanner/backpack-web/bpk-component-breakpoint'),
+  useMediaQuery: () => true,
+});
+```
+
+
 ## Props
 
 Check out the full list of props on Skyscanner's [design system documentation website](https://www.skyscanner.design/latest/components/breakpoint/web-5sPWfgsH#section-props-32).
diff --git a/packages/bpk-component-breakpoint/index.d.ts b/packages/bpk-component-breakpoint/index.d.ts
@@ -17,5 +17,6 @@
  */
 
 import BpkBreakpoint, { BREAKPOINTS } from './src/BpkBreakpoint';
-export { BREAKPOINTS };
+import useMediaQuery from './src/useMediaQuery';
+export { BREAKPOINTS, useMediaQuery };
 export default BpkBreakpoint;
diff --git a/packages/bpk-component-breakpoint/src/BpkBreakpoint-test.tsx b/packages/bpk-component-breakpoint/src/BpkBreakpoint-test.tsx
@@ -23,99 +23,111 @@ import { render } from '@testing-library/react';
 import { BREAKPOINTS } from './BpkBreakpoint';
 
 describe('BpkBreakpoint', () => {
-  it('should render if the breakpoint is matched', () => {
-    jest.resetModules();
-    const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
-    jest.mock('./useMediaQuery', () => () => true);
-
-    const { asFragment } = render(
-      <BpkBreakpoint query={BREAKPOINTS.MOBILE}>
-        {(matches: boolean) =>
-          matches ? <div>matches</div> : <div>does not match</div>
-        }
-      </BpkBreakpoint>,
-    );
-    expect(asFragment()).toMatchSnapshot();
-  });
-
-  it('should render if the breakpoint is not matched', () => {
-    jest.resetModules();
-    const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
-    jest.mock('./useMediaQuery', () => () => false);
-
-    const { asFragment } = render(
-      <BpkBreakpoint query={BREAKPOINTS.MOBILE}>
-        {(matches: boolean) =>
-          matches ? <div>matches</div> : <div>does not match</div>
-        }
-      </BpkBreakpoint>,
-    );
-    expect(asFragment()).toMatchSnapshot();
-  });
-
-  describe('SSR mode', () => {
-    beforeEach(() => {
+  describe('children as component', () => {
+    it('should render when breakpoint matches', () => {
+      jest.resetModules();
+      const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
       jest.mock('./useMediaQuery', () => () => true);
 
-      jest.resetModules();
+      const { asFragment } = render(
+        <BpkBreakpoint query={BREAKPOINTS.MOBILE}>
+          <div>matches</div>
+        </BpkBreakpoint>,
+      );
+      expect(asFragment()).toMatchSnapshot();
     });
 
-    afterEach(() => {
-      jest.restoreAllMocks();
+    it('should not render when breakpoint does not match', () => {
+      jest.resetModules();
+      const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
+      jest.mock('./useMediaQuery', () => () => false);
+
+      const { asFragment } = render(
+        <BpkBreakpoint query={BREAKPOINTS.MOBILE}>
+          <div>matches</div>
+        </BpkBreakpoint>,
+      );
+      expect(asFragment()).toMatchSnapshot();
     });
+  });
 
-    it('should render when matchSSR=true', () => {
+  describe('children as a callback function', () => {
+    it('should call function with matches=false if the breakpoint is not matched', () => {
+      jest.resetModules();
       const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
+      jest.mock('./useMediaQuery', () => () => false);
 
-      const html = ReactDOMServer.renderToString(
-        <BpkBreakpoint query={BREAKPOINTS.MOBILE} matchSSR>
+      const { asFragment } = render(
+        <BpkBreakpoint query={BREAKPOINTS.MOBILE}>
           {(matches: boolean) =>
             matches ? <div>matches</div> : <div>does not match</div>
           }
         </BpkBreakpoint>,
       );
-
-      expect(html).toMatchSnapshot();
+      expect(asFragment()).toMatchSnapshot();
     });
 
-    it('should not render on SSR until hydrated when matchSSR=false', async () => {
+    it('should call function with matches=true if the breakpoint is matched', () => {
+      jest.resetModules();
       const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
+      jest.mock('./useMediaQuery', () => () => true);
 
-      const components = (
-        <BpkBreakpoint query={BREAKPOINTS.MOBILE} matchSSR={false}>
+      const { asFragment } = render(
+        <BpkBreakpoint query={BREAKPOINTS.MOBILE}>
           {(matches: boolean) =>
             matches ? <div>matches</div> : <div>does not match</div>
           }
-        </BpkBreakpoint>
+        </BpkBreakpoint>,
       );
+      expect(asFragment()).toMatchSnapshot();
+    });
+  });
+
+  describe('SSR mode', () => {
+    beforeEach(() => {
+      jest.mock('./useMediaQuery', () => () => true);
+
+      jest.resetModules();
+    });
+
+    afterEach(() => {
+      jest.restoreAllMocks();
+    });
 
-      // Checking SSR
-      const html = ReactDOMServer.renderToString(components);
+    it('should pass matchSSR=true to useMediaQuery when matchSSR=true', () => {
+      const mockUseMediaQuery = jest.fn();
+      jest.mock('./useMediaQuery', () => mockUseMediaQuery);
+      const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
 
-      expect(html).toMatchSnapshot('server rendered');
+      ReactDOMServer.renderToString(
+        <BpkBreakpoint query={BREAKPOINTS.MOBILE} matchSSR />,
+      );
 
-      const container = document.createElement('div');
-      document.body.appendChild(container);
-      container.innerHTML = html;
+      expect(mockUseMediaQuery).toHaveBeenCalledWith(BREAKPOINTS.MOBILE, true);
+    });
 
-      // Hydrating and CSR
-      const { asFragment } = render(components, { hydrate: true, container });
+    it('should pass matchSSR=false to useMediaQuery when matchSSR=false', () => {
+      const mockUseMediaQuery = jest.fn();
+      jest.mock('./useMediaQuery', () => mockUseMediaQuery);
+      const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
 
-      expect(asFragment()).toMatchSnapshot('hydrated');
+      ReactDOMServer.renderToString(
+        <BpkBreakpoint query={BREAKPOINTS.MOBILE} matchSSR={false} />,
+      );
+
+      expect(mockUseMediaQuery).toHaveBeenCalledWith(BREAKPOINTS.MOBILE, false);
     });
 
-    it('should not render when matchSSR is not defined', () => {
+    it('should pass matchSSR=false to useMediaQuery when matchSSR not defined', () => {
+      const mockUseMediaQuery = jest.fn();
+      jest.mock('./useMediaQuery', () => mockUseMediaQuery);
       const BpkBreakpoint = require('./BpkBreakpoint').default; // eslint-disable-line global-require
 
-      const html = ReactDOMServer.renderToString(
-        <BpkBreakpoint query={BREAKPOINTS.MOBILE}>
-          {(matches: boolean) =>
-            matches ? <div>matches</div> : <div>does not match</div>
-          }
-        </BpkBreakpoint>,
+      ReactDOMServer.renderToString(
+        <BpkBreakpoint query={BREAKPOINTS.MOBILE} />,
       );
 
-      expect(html).toMatchSnapshot();
+      expect(mockUseMediaQuery).toHaveBeenCalledWith(BREAKPOINTS.MOBILE, false);
     });
   });
 

diff --git a/packages/bpk-component-breakpoint/src/BpkBreakpoint.tsx b/packages/bpk-component-breakpoint/src/BpkBreakpoint.tsx
@@ -16,7 +16,6 @@
  * limitations under the License.
  */
 
-import { useState, useEffect } from 'react';
 import type { ReactElement, ReactNode } from 'react';
 
 // @ts-expect-error Untyped import. See `decisions/imports-ts-suppressions.md`.
@@ -53,32 +52,18 @@ const BpkBreakpoint = ({
   matchSSR = false,
   query,
 }: Props) => {
-  const [isClient, setIsClient] = useState(false);
-  const matches = useMediaQuery(query);
+  const matches = useMediaQuery(query, matchSSR);
 
-  useEffect(() => {
-    setIsClient(true);
-  }, []);
-
-  if (isClient) {
-    if (!legacy && !Object.values(BREAKPOINTS).includes(query)) {
-      console.warn(
-        `Invalid query ${query}. Use one of the supported queries or pass the legacy prop.`,
-      );
-    }
-
-    if (typeof children === 'function') {
-      return children(matches) as ReactElement;
-    }
-    return matches ? (children as ReactElement) : null;
+  if (!legacy && !Object.values(BREAKPOINTS).includes(query)) {
+    console.warn(
+      `Invalid query ${query}. Use one of the supported queries or pass the legacy prop.`,
+    );
   }
 
-  // Below code is executed when running in SSR mode
-
   if (typeof children === 'function') {
-    return children(matchSSR);
+    return children(matches) as ReactElement;
   }
-  return matchSSR ? children : null;
+  return matches ? (children as ReactElement) : null;
 };
 
 export { BREAKPOINTS };

diff --git a/packages/bpk-component-breakpoint/src/__snapshots__/BpkBreakpoint-test.tsx.snap b/packages/bpk-component-breakpoint/src/__snapshots__/BpkBreakpoint-test.tsx.snap
@@ -1,33 +1,27 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`BpkBreakpoint SSR mode should not render on SSR until hydrated when matchSSR=false: hydrated 1`] = `
+exports[`BpkBreakpoint children as a callback function should call function with matches=false if the breakpoint is not matched 1`] = `
 <DocumentFragment>
-  <div
-    data-reactroot=""
-  >
-    matches
+  <div>
+    does not match
   </div>
 </DocumentFragment>
 `;
 
-exports[`BpkBreakpoint SSR mode should not render on SSR until hydrated when matchSSR=false: server rendered 1`] = `"<div data-reactroot="">does not match</div>"`;
-
-exports[`BpkBreakpoint SSR mode should not render when matchSSR is not defined 1`] = `"<div data-reactroot="">does not match</div>"`;
-
-exports[`BpkBreakpoint SSR mode should render when matchSSR=true 1`] = `"<div data-reactroot="">matches</div>"`;
-
-exports[`BpkBreakpoint should render if the breakpoint is matched 1`] = `
+exports[`BpkBreakpoint children as a callback function should call function with matches=true if the breakpoint is matched 1`] = `
 <DocumentFragment>
   <div>
     matches
   </div>
 </DocumentFragment>
 `;
 
-exports[`BpkBreakpoint should render if the breakpoint is not matched 1`] = `
+exports[`BpkBreakpoint children as component should not render when breakpoint does not match 1`] = `<DocumentFragment />`;
+
+exports[`BpkBreakpoint children as component should render when breakpoint matches 1`] = `
 <DocumentFragment>
   <div>
-    does not match
+    matches
   </div>
 </DocumentFragment>
 `;
diff --git a/packages/bpk-component-breakpoint/src/__snapshots__/useMediaQuery-test.tsx.snap b/packages/bpk-component-breakpoint/src/__snapshots__/useMediaQuery-test.tsx.snap
@@ -0,0 +1,7 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`useMediaQuery SSR mode should match when matchSSR=true 1`] = `"<div data-reactroot="">matches</div>"`;
+
+exports[`useMediaQuery SSR mode should not match when matchSSR not explicitly set 1`] = `"<div data-reactroot="">no match</div>"`;
+
+exports[`useMediaQuery SSR mode should not match when matchSSR=false 1`] = `"<div data-reactroot="">no match</div>"`;
diff --git a/packages/bpk-component-breakpoint/src/accessibility-test.tsx b/packages/bpk-component-breakpoint/src/accessibility-test.tsx
@@ -21,6 +21,9 @@ import { axe } from 'jest-axe';
 
 import BpkBreakpoint, { BREAKPOINTS } from './BpkBreakpoint';
 
+// mock breakpoint to always match
+jest.mock('./useMediaQuery', () => jest.fn(() => true));
+
 describe('BpkBreakpoint accessibility tests', () => {
   it('should not have programmatically-detectable accessibility issues', async () => {
     const { container } = render(