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.

Sign up for GitHub

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

[code-infra] Add use-sync-external-store shim for useMediaQuery #43476

Closed
wants to merge 3 commits into from
Closed

[code-infra] Add use-sync-external-store shim for useMediaQuery #43476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c88e494
[code-infra] Add use-sync-external-store shim
Janpot Aug 27, 2024
f510f36
try externalizing it
Janpot Aug 27, 2024
d368562
Update webpack.config.js
Janpot Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/mui-system/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
"@mui/utils": "workspace:^",
"clsx": "^2.1.1",
"csstype": "^3.1.3",
"prop-types": "^15.8.1"
"prop-types": "^15.8.1",
"use-sync-external-store": "^1.2.2"
},
"devDependencies": {
"@emotion/react": "^11.13.3",
Expand All @@ -58,6 +59,7 @@
"@types/prop-types": "^15.7.12",
"@types/react": "^18.3.4",
"@types/sinon": "^17.0.3",
"@types/use-sync-external-store": "^0.0.6",
"chai": "^4.5.0",
"fast-glob": "^3.3.2",
"lodash": "^4.17.21",
Expand Down
125 changes: 30 additions & 95 deletions packages/mui-system/src/useMediaQuery/useMediaQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use client';
import * as React from 'react';
import useEnhancedEffect from '@mui/utils/useEnhancedEffect';
import { useSyncExternalStore } from 'use-sync-external-store/shim';
import { getThemeProps } from '../useThemeProps';
import useTheme from '../useThemeWithoutDefault';

Expand Down Expand Up @@ -30,57 +30,39 @@ export interface UseMediaQueryOptions {
ssrMatchMedia?: (query: string) => { matches: boolean };
}

function useMediaQueryOld(
query: string,
defaultMatches: boolean,
matchMedia: typeof window.matchMedia | null,
ssrMatchMedia: ((query: string) => { matches: boolean }) | null,
noSsr: boolean,
export default function useMediaQuery<Theme = unknown>(
queryInput: string | ((theme: Theme) => string),
options: UseMediaQueryOptions = {},
): boolean {
const [match, setMatch] = React.useState(() => {
if (noSsr && matchMedia) {
return matchMedia!(query).matches;
}
if (ssrMatchMedia) {
return ssrMatchMedia(query).matches;
}

// Once the component is mounted, we rely on the
// event listeners to return the correct matches value.
return defaultMatches;
});
const theme = useTheme<Theme>();
// Wait for jsdom to support the match media feature.
// All the browsers MUI support have this built-in.
// This defensive check is here for simplicity.
// Most of the time, the match media logic isn't central to people tests.
const supportMatchMedia =
typeof window !== 'undefined' && typeof window.matchMedia !== 'undefined';
const {
defaultMatches = false,
matchMedia = supportMatchMedia ? window.matchMedia : null,
ssrMatchMedia = null,
noSsr = false,
} = getThemeProps({ name: 'MuiUseMediaQuery', props: options, theme });

useEnhancedEffect(() => {
if (!matchMedia) {
return undefined;
if (process.env.NODE_ENV !== 'production') {
if (typeof queryInput === 'function' && theme === null) {
console.error(
[
'MUI: The `query` argument provided is invalid.',
'You are providing a function without a theme in the context.',
'One of the parent elements needs to use a ThemeProvider.',
].join('\n'),
);
}
}

const queryList = matchMedia!(query);
const updateMatch = () => {
setMatch(queryList.matches);
};

updateMatch();
queryList.addEventListener('change', updateMatch);

return () => {
queryList.removeEventListener('change', updateMatch);
};
}, [query, matchMedia]);

return match;
}

// eslint-disable-next-line no-useless-concat -- Workaround for https://github.com/webpack/webpack/issues/14814
const maybeReactUseSyncExternalStore: undefined | any = (React as any)['useSyncExternalStore' + ''];
let query = typeof queryInput === 'function' ? queryInput(theme) : queryInput;
query = query.replace(/^@media( ?)/m, '');

function useMediaQueryNew(
query: string,
defaultMatches: boolean,
matchMedia: typeof window.matchMedia | null,
ssrMatchMedia: ((query: string) => { matches: boolean }) | null,
noSsr: boolean,
): boolean {
const getDefaultSnapshot = React.useCallback(() => defaultMatches, [defaultMatches]);
const getServerSnapshot = React.useMemo(() => {
if (noSsr && matchMedia) {
Expand Down Expand Up @@ -110,54 +92,7 @@ function useMediaQueryNew(
},
];
}, [getDefaultSnapshot, matchMedia, query]);
const match = maybeReactUseSyncExternalStore(subscribe, getSnapshot, getServerSnapshot);

return match;
}

export default function useMediaQuery<Theme = unknown>(
queryInput: string | ((theme: Theme) => string),
options: UseMediaQueryOptions = {},
): boolean {
const theme = useTheme<Theme>();
// Wait for jsdom to support the match media feature.
// All the browsers MUI support have this built-in.
// This defensive check is here for simplicity.
// Most of the time, the match media logic isn't central to people tests.
const supportMatchMedia =
typeof window !== 'undefined' && typeof window.matchMedia !== 'undefined';
const {
defaultMatches = false,
matchMedia = supportMatchMedia ? window.matchMedia : null,
ssrMatchMedia = null,
noSsr = false,
} = getThemeProps({ name: 'MuiUseMediaQuery', props: options, theme });

if (process.env.NODE_ENV !== 'production') {
if (typeof queryInput === 'function' && theme === null) {
console.error(
[
'MUI: The `query` argument provided is invalid.',
'You are providing a function without a theme in the context.',
'One of the parent elements needs to use a ThemeProvider.',
].join('\n'),
);
}
}

let query = typeof queryInput === 'function' ? queryInput(theme) : queryInput;
query = query.replace(/^@media( ?)/m, '');

// TODO: Drop `useMediaQueryOld` and use `use-sync-external-store` shim in `useMediaQueryNew` once the package is stable
Copy link
Member

@oliviertassinari oliviertassinari Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't quite understand this comment Sebastian left, is useMediaQueryOld bigger than use-sync-external-store?

Copy link
Member Author

@Janpot Janpot Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

useMediaQueryOld is smaller. But use-sync-external-store is not huge(*), and it's likely already present in our user's bundles. which in that specific case means us depending on it doesn't add extra cost.

(*)
Screenshot 2024-08-28 at 05 13 58

Copy link
Member

@oliviertassinari oliviertassinari Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tricky, it's used a lot indeed: https://npm-stat.com/charts.html?package=use-sync-external-store,react-dom.

I'm worried about harming people who make the effort to keep their stack up to date, for the benefit of those who want to use the latest features without paying the price for it. It will take a while for use-sync-external-store to be gone from the bundle of end-users.

We could have:

Suggested change
// TODO: Drop `useMediaQueryOld` and use `use-sync-external-store` shim in `useMediaQueryNew` once the package is stable
// TODO: Remove `useMediaQueryOld` once React 17 support is removed

Copy link
Member Author

@Janpot Janpot Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, that's why I'm ultimately abandoning this. I just wanted to capture the impact.
One solution I was pondering is to provide a shim for older older React APIs. Users would have to manually import a module in the top-level if they want to use modern @mui/material on outdated react. e.g.

// import this before anything else in user code if you're on legacy react
import '@mui/shims/setup-react-17'

implementation:

// we import this where we need `useSyncExternalStore`
import { useSyncExternalStore } from '@mui/shims/useSyncExternalStore'
// @mui/shims/setup-react-17
import { useSyncExternalStore } from 'use-sync-external-store'
globalThis._muiShims ?= {}
globalThis._muiShims.useSyncExternalStore = useSyncExternalStore
globalThis._muiShims.useId = () => {
  // ...
}
// @mui/shims/useSyncExternalStore
import * as React from 'react'
import invariant from 'invariant'
const _React = { ...React }
const useSyncExternalStore = _React.useSyncExternalStore || globalThis._muiShims?.useSyncExternalStore
invariant(useSyncExternalStore, "MUI shims not installed, If you're on legacy React, please follow the guide at https://mui.com/...")
export { useSyncExternalStore }

That way you only pay the cost of a shim if you choose to stay on react 17. Have to say though that it's probably way overkill just to save 0.5kB.

Copy link
Member

Choose a reason for hiding this comment

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

Have to say though that it's probably way overkill just to save 0.5kB.

😄

const useMediaQueryImplementation =
maybeReactUseSyncExternalStore !== undefined ? useMediaQueryNew : useMediaQueryOld;
const match = useMediaQueryImplementation(
query,
defaultMatches,
matchMedia,
ssrMatchMedia,
noSsr,
);
const match = useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot);

if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line react-hooks/rules-of-hooks
Expand Down
Loading