-
Notifications
You must be signed in to change notification settings - Fork 220
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.
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
Reduce number of media query event listeners attached to window #2800
base: main
Are you sure you want to change the base?
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.