Reduce number of media query event listeners attached to window #2800
Conversation
jesstelford
force-pushed
the
fewer-media-queries
branch
from
ec1c360
to
90a03d7
Compare
|
/snapit |
|
🫰✨ Thanks @jesstelford! Your snapshots have been published to npm. Test the snapshots by updating your "@shopify/address-mocks": "0.0.0-snapshot-20240620045713",
"graphql-fixtures": "0.0.0-snapshot-20240620045713",
"graphql-mini-transforms": "0.0.0-snapshot-20240620045713",
"@shopify/graphql-testing": "0.0.0-snapshot-20240620045713",
"graphql-typed": "0.0.0-snapshot-20240620045713",
"graphql-typescript-definitions": "0.0.0-snapshot-20240620045713",
"graphql-validate-fixtures": "0.0.0-snapshot-20240620045713",
"@shopify/jest-dom-mocks": "0.0.0-snapshot-20240620045713",
"@shopify/jest-koa-mocks": "0.0.0-snapshot-20240620045713",
"@shopify/koa-metrics": "0.0.0-snapshot-20240620045713",
"@shopify/koa-performance": "0.0.0-snapshot-20240620045713",
"@shopify/name": "0.0.0-snapshot-20240620045713",
"@shopify/react-app-bridge-universal-provider": "0.0.0-snapshot-20240620045713",
"@shopify/react-async": "0.0.0-snapshot-20240620045713",
"@shopify/react-compose": "0.0.0-snapshot-20240620045713",
"@shopify/react-cookie": "0.0.0-snapshot-20240620045713",
"@shopify/react-csrf-universal-provider": "0.0.0-snapshot-20240620045713",
"@shopify/react-effect": "0.0.0-snapshot-20240620045713",
"@shopify/react-form": "0.0.0-snapshot-20240620045713",
"@shopify/react-form-state": "0.0.0-snapshot-20240620045713",
"@shopify/react-google-analytics": "0.0.0-snapshot-20240620045713",
"@shopify/react-graphql": "0.0.0-snapshot-20240620045713",
"@shopify/react-graphql-universal-provider": "0.0.0-snapshot-20240620045713",
"@shopify/react-hooks": "0.0.0-snapshot-20240620045713",
"@shopify/react-html": "0.0.0-snapshot-20240620045713",
"@shopify/react-hydrate": "0.0.0-snapshot-20240620045713",
"@shopify/react-i18n": "0.0.0-snapshot-20240620045713",
"@shopify/react-i18n-universal-provider": "0.0.0-snapshot-20240620045713",
"@shopify/react-import-remote": "0.0.0-snapshot-20240620045713",
"@shopify/react-network": "0.0.0-snapshot-20240620045713",
"@shopify/react-router": "0.0.0-snapshot-20240620045713",
"@shopify/react-server": "0.0.0-snapshot-20240620045713",
"@shopify/react-testing": "0.0.0-snapshot-20240620045713",
"@shopify/react-universal-provider": "0.0.0-snapshot-20240620045713",
"@shopify/react-web-worker": "0.0.0-snapshot-20240620045713",
"@shopify/useful-types": "0.0.0-snapshot-20240620045713",
"@shopify/web-worker": "0.0.0-snapshot-20240620045713" |
| type EventListener = (event: {matches: boolean}) => void; | ||
| type Callback = (matches: boolean) => void; | ||
|
|
||
| const hookCallbacks: { |
There was a problem hiding this comment.
This should be moved into the createUseMediaFactory function, but before it creates the new function. Will prevent this value from being at the module-scope which would prevent any garbage collection (and will make testing easier)
| hookCallback(event.matches); | ||
| } catch (error) { | ||
| // eslint-disable-next-line no-console | ||
| console.error(error); |
There was a problem hiding this comment.
We should be allowing the error to raise instead of using console.error here, which would probably not be desirable for an application which has its own error handling and external logging
|
|
||
| setMatch(matchMedia.matches); | ||
| // Setup the event listener for this query | ||
| eventListener: (event: {matches: boolean}) => { |
There was a problem hiding this comment.
It looks like we're doing a fair bit of work here to re-create the API of useEffect and addEventListener. The main fix here would primarily be about deduplicating the window.matchMedia(query) and ensuring that we don't create a new one for each. Does the custom eventListener here which goes through a list of callbacks have a meaningful improvement in Safari that isn't achieved by the individual callbacks registered with addEventListener (assuming we dedupe matchMedia)?
Description
Fixes #2801
Deduplicates media query event listeners to improve performance in Safari.