[code-infra] Add use-sync-external-store shim for useMediaQuery
#43476
Conversation
Netlify deploy previewhttps://deploy-preview-43476--material-ui.netlify.app/ Hidden: parsed: +0.99% , gzip: +1.16% Bundle size reportDetails of bundle changes (Toolpad) |
Janpot
changed the title
use-sync-external-store shim for useMediaQuery
It's changing in #43360. |
| 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 |
There was a problem hiding this comment.
I don't quite understand this comment Sebastian left, is useMediaQueryOld bigger than use-sync-external-store?
There was a problem hiding this comment.
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.
(*)
There was a problem hiding this comment.
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:
| // 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Have to say though that it's probably way overkill just to save 0.5kB.
😄
That solution causes the same webpack helper to be used. I think it's unavoidable. We can close this PR and just wait until we deprecate React 17. |
We can keep this as long as we're supporting React <18.
I noticed a bundle size increase in #43264. Caused by a webpack interop helper that gets included when you access module bindings in a weird way. The reason is a backwards compatible shim that was created around the absent
useSyncExternalStorehook.material-ui/packages/mui-system/src/useMediaQuery/useMediaQuery.ts
Line 75 in eab1b9e
This PR to assess the impact of using this shim. It looks like it's an increase, but the results need to be interpreted.
use-sync-external-storeis used by many well known packages. There is a great chance it already appears in our user's bundles. In which case we may as well choose to ignore it in our bundle size calculations. It's coincidence, but the increase thatuse-sync-external-storebrings in this change is about equal to the increase the webpack interop helper brings in #43264.without externalizing it:
with externalizing it:
Pretty sure the
useIdshim is causing the same problemmaterial-ui/packages/mui-utils/src/useId/useId.ts
Line 22 in eab1b9e
@mui/code-infra What's your opinion on this? I'm inclined to abandon this PR and instead accept the bundle size increase of #43264 as a bundler specific implementation detail.