fix(useMediaQuery): remove asymmetric addListener/removeListener fallback in unsubscribe#1663
fix(useMediaQuery): remove asymmetric addListener/removeListener fallback in unsubscribe#1663lennondotw wants to merge 1 commit intoreact-hookz:masterfrom
Conversation
lennondotw
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes a compatibility issue in the useMediaQuery hook where event listener subscription and unsubscription were handled asymmetrically, causing errors in Safari versions < 14 that only support the deprecated addListener/removeListener methods.
Changes:
- Restored the
addEventListener/addListenercompatibility check in thecreateQueryEntryfunction to match the existing compatibility logic inqueryUnsubscribe - Ensures symmetric handling of event listeners for both subscription and unsubscription operations
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/useMediaQuery/index.ts
Outdated
| if (mql.addEventListener) { | ||
| mql.addEventListener('change', listener, {passive: true}); | ||
| } else { | ||
| mql.addListener(listener); | ||
| } |
There was a problem hiding this comment.
Consider adding test coverage for the legacy addListener/removeListener compatibility path. The current tests only mock addEventListener and removeEventListener. Adding a test that mocks a MediaQueryList without addEventListener would ensure this compatibility code path is tested.
…back in unsubscribe createQueryEntry uses addEventListener unconditionally, but queryUnsubscribe still had a removeEventListener/removeListener fallback. This asymmetry is unnecessary since all supported browsers now have addEventListener on MediaQueryList.
lennondotw
changed the title
|
Rebased on latest master and updated the fix — the original PR was adding back the @xobotyi would you mind taking a look? It's a small cleanup — just removing 4 lines. |
Summary
createQueryEntryusesaddEventListenerunconditionally (as of #1661), butqueryUnsubscribestill had aremoveEventListener/removeListenerfallback for Safari < 14. This asymmetry is unnecessary — if subscribe doesn't handle old browsers, unsubscribe shouldn't either.Changes
removeEventListener/removeListenerconditional inqueryUnsubscribe, now usesremoveEventListenerdirectly to matchcreateQueryEntry