fix: set matchMedia on window to fix issue on IE11, fixes #11358

[DanielRuf]

Description

This fixes an issue in IE11 with a conflict between window.matchMedia and matchMedia.

• Closes Polyfill for mediaQuery breaks in IE11 #11358

Motivation and Context

Screenshots (if appropriate):

Types of changes

• Documentation
• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing
  functionality to change)

Checklist (all required):

• I have read and follow the CONTRIBUTING document.
• There are no other pull request similar to this one.
• The pull request title is descriptive.
• The template is fully and correctly filled.
• [] The pull request targets the right branch (develop or develop-v...).
• My commits are correctly titled and contain all relevant information.
• My code follows the code style of this project.
• I have updated the documentation accordingly to my changes (if relevant).
• I have added tests to cover my changes (if relevant).
• All new and existing tests passed.

[DanielRuf]

See https://github.com/paulirish/matchMedia.js/blob/master/matchMedia.js#L3

[ncoden]

https://github.com/paulirish/matchMedia.js/blob/cd9df4c529a3c5c95c6d66b77bb7cc3d2419c460/matchMedia.js#L3 is a polyfill that will often be explicitely imported in the project. This is not the case with the embed version inside Foundation. Some could consider this as an undisired and dangerous side-effect, and that this matchMedia should only be used within the Foundation scope without affecting window.

If I understand the issue correctly, renaming matchMedia to _matchMedia would work too without introducing any side-effect, right ?

[ncoden]

We could use the opportunity to update this polyfill to the latest version 😉

[DanielRuf]

If I understand the issue correctly, renaming matchMedia to _matchMedia would work too without introducing any side-effect, right ?

Why would you do that? A polyfill is meant to directly polyfill the window object, it is now correct in the upstream version but not ours ;-)

This polyfills the native matchMedia.

[DanielRuf]

It just polyfills / sets it if it is not yet available on the window object.

[ncoden]

Does this polyfill perfectly reproduce the native matchMedia specs ? If yes, ok that's fine 👍. However, this could introduce bugs in an other package relying on its own polyfill but that would use our (buggy) one instead thinking it is the native one.

[DanielRuf]

Does this polyfill perfectly reproduce the native matchMedia specs ? If yes, ok that's fine

It should. It is only used if the browser does not support it.
paulirish/matchMedia.js#65 (comment)

https://caniuse.com/#feat=matchmedia

It should still work after these years. I trust @paulirish

There are not many polyfills, see https://github.com/search?o=desc&q=matchmedia&s=stars&type=Repositories and https://github.com/Financial-Times/polyfill-service/tree/master/packages/polyfill-library/polyfills/matchMedia

[kb3eua]

I still don't understand why Foundation is forcing this polyfill on users. Some sites may only be targeting evergreen browsers that already support this natively, in which case this polyfill adds unnecessary bloat to the JavaScript file. And I agree with @ncoden about the possibility of a conflict with other polyfills (not to mention that in this case you'd shipping 2 polyfills to do the same job... more unnecessary code bloat users will have to download). The Foundation installation guide should mention that a polyfill is required for older versions of IE should the user want to support them, but it should put the onus on the developer to install the polyfill themselves separate from Foundation.

[DanielRuf]

that already support this natively, in which case this polyfill adds unnecessary bloat to the JavaScript file.

Polyfills are only applied if the polyfilled feature is not supported so this is not the case.

[DanielRuf]

And it is not much code ;-)

[ncoden]

Polyfills are only applied if the polyfilled feature is not supported so this is not the case.

Polyfills may be ignored during execution but they are still bloating the Javascript file.

[kb3eua]

Also, the browsers that require this polyfill are so ancient and have such a small percentage of market share that it makes no sense to ship this polyfill. IE11 supports this natively and has ~1.9% market share... older IEs that might need this polyfill have ~0.1% market share... at what point do you drop support for older IEs?

[ncoden]

@kb3eua For now, Foundation support IE >= 9 (unless for flex features). It is already planned to deprecate IE 9/10 in future versions, but keep in mind that Foundation is known for its very good browser support and often used for it.

[DanielRuf]

Also, the browsers that require this polyfill are so ancient and have such a small percentage of market share that it makes no sense to ship this polyfill. IE11 supports this natively and has ~1.9% market share... older IEs that might need this polyfill have ~0.1% market share... at what point do you drop support for older IEs?

This is totally different in different countries.

[DanielRuf]

Polyfills may be ignored during execution but they are still bloating the Javascript file

I know but this is not much compared to the rest. We can also apply a patch using patch-package to remove the code / polyfill ;-)

[ncoden]

We can also apply a patch using patch-package to remove the code / polyfill ;-)

It works, but we should not rely on it as package maintainers, this is not a "healthy" and maintainable solution in the long term for our users.

[DanielRuf]

Sure, this is why I proposed the polyfill-service and then drop internal polyfills ;-)