fix: remove legacy browser API fallbacks#1663
Conversation
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.
|
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. |
|
@xobotyi I rebased this PR onto the latest Current Validation on the rebased branch:
The previous Copilot review thread is outdated now, since this PR intentionally removes the legacy fallback rather than preserving it. |
|
Rebased onto current master and broadened the scope: the same "standard API might be missing" defensiveness existed in |
…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.
mozConnection and webkitConnection existed only on old Firefox for Android and pre-61 Chromium; every browser exposing the Network Information API today ships unprefixed navigator.connection. Firefox and Safari do not ship the API at all.
on/off called addEventListener/removeEventListener via optional chaining, turning a call on a non-EventTarget into a silent no-op that hides subscription bugs. The type constraint already guarantees the methods exist; invalid runtime targets now throw. usePermission test mocks gain the listener methods a real PermissionStatus always has.
|
Thank you for your contribution. Will be released somewhat today, along with deps upgrade |
|
🎉 This PR is included in version 25.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
Removes the remaining code paths that pretend standard browser APIs may be missing.
createQueryEntryinuseMediaQuerysubscribes withaddEventListenerunconditionally, so the deprecatedremoveListenerfallback inqueryUnsubscribewas asymmetric dead code — and the same "API might not exist" defensiveness lingered inuseNetworkStateand the sharedon/offhelpers.Changes
useMediaQuery:queryUnsubscribeusesremoveEventListenerdirectly, matchingcreateQueryEntry(Safari < 14 fallback removed)useNetworkState: droppedmozConnection/webkitConnection— every browser shipping the Network Information API exposes unprefixednavigator.connection; Firefox and Safari don't ship it at allutil/misc.ts:on/offno longer optional-chainaddEventListener/removeEventListener— theEventTargetconstraint guarantees the methods exist; a non-EventTarget target now throws instead of silently doing nothingusePermissiontests: mocks gained the listener methods a realPermissionStatusalways has (the silent no-op inon/offwas masking the incomplete mocks)Testing
yarn lintyarn test— full suite, 527 tests across 117 filesyarn build