polyfill flatMap for Chrome < 69 #2822
Conversation
github-actions
bot
added
🧱 stack: documentation
openverse-bot
added
🟨 priority: medium
|
Size Change: +518 B (0%) Total Size: 868 kB
ℹ️ View Unchanged
|
|
Full-stack documentation: https://docs.openverse.org/_preview/2822 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
There was a problem hiding this comment.
Rather than manually adding the polyfill (and potentially playing whack-a-mole if other newer APIs get used by dependencies), would it be cleaner to expand our browserslist to accurately reflect the browsers we wish to support (in this case, Chrome < 69)? As I understand it, if we do it that way we might only see a bundle size increase for the "legacy" bundle rather than in all bundles, in addition to the benefit of not needing to manually manage polyfills.
Would adding something like "chrome >= 68" to the "browserslist" in |
There was a problem hiding this comment.
I'm going to approve and merge this now, actually, because the bundle size increase is relatively small and browserslist doesn't support complex usage of not ("or not" does not work the way we would think it does). But I will say that if we're adding a polyfill for this, we should consider how our browserslist target is configured. The number of actual clients that are effected by this, based on Chrome numbers on our Plausible, is probably ~200. That 200 clients of 578000. In other words, very few people.
I think a preferrable alternative to always including the flat-map polyfill could be an asynchronous import of https://polyfill.io/v3/polyfill.min.js?version=3.111.0&features=Array.prototype.flatMap%2CArray.prototype.flat. Polyfill.io only returns the polyfills if the requesting browser needs it. We'd add await import('https://polyfill.io/v3/polyfill.min.js?version=3.111.0&features=Array.prototype.flatMap%2CArray.prototype.flat') before using the particular vueuse core composable that requires it.
In any case, we can go with this option for now, but we could open to change it to use a conditional polyfill. Half a kB doesn't seem like heaps when our total bundle size is 868 kB... but that rationale would have us never make bundle size improvements that incrementally add up in the end. From a carbon perspective, on the other hand, it might be better to always send that half a kB rather than creating another request to the polyfill.io CDN. Hard trade offs to make 🤔 I haven't gone ahead and opened the issue because I don't know exactly which action to recommend myself.
|
Oops, I was writing my approving review while you were writing your comment, Olga. The approach with browserslist I was hoping would work is the In any case, approved and merged. I think my comment about the actual number of people effected by this is valid though and we should consider strongly what kind of coverage we consider as standard. Our browserslist configuration is actually narrower than the default browsers list config, so there isn't anything to currently suggest we have an intention of very wide client support. |
Fixes
Fixes #2745 by the sentry bot
Description
Polyfills the Array.prototype.flatMap method so it works in versions of Chrome < 69. This has occurred enough times to warrant the polyfilling.
Testing Instructions
Build locally and confirm the presence of the polyfill in the client bundle by searching for
flatMap:and confirming it's there.We should also see a minuscule increase to the bundle size.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin