WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.3k stars 4.11k forks source link

Dequeue wp-polyfill for browsers that don't need it. #21616

Closed simison closed 3 years ago

simison commented 4 years ago

Is your feature request related to a problem? Please describe. We often enqueue wp-polyfill to ensure JS is compatible with older versions of browsers. Polyfills are a lot of KBs (wp-polyfill alone is ~97kb runtime, or ~34kb gzipped) and other polyfills like fetch on top of it. It's wasteful in modern browsers that don't need it.

Describe the solution you'd like Let plugins and themes register wp-polyfill regularly, but dequeue it if we detect that request is coming from fairly modern browser, based on user-agent.

Describe alternatives you've considered Async loading polyfills only when they are needed, but that gets pretty complicated quickly.

blowery commented 4 years ago

Some of the polyfills apply to fix bugs in implementations of modern language features. See https://babeljs.io/blog/2020/03/16/7.9.0. How would you know that the browser making the request requires none of the polyfills?

sgomes commented 4 years ago

In general the approach sounds feasible, but it's all about the details. It's hard to comment too much on this without knowing what exactly lurks inside wp-polyfill; I can't seem to find an authoritative list of what it includes, nor any sort of readable, adequately commented source.

I also don't know how often wp-polyfill gets updated. Is it a static thing, built for a single point in time, or does it keep getting new functionality added? Is there any sort of versioning?

sgomes commented 4 years ago

Describe alternatives you've considered Async loading polyfills only when they are needed, but that gets pretty complicated quickly.

That's not a feasible approach unless the code that requires those polyfills is async too, which is almost certainly not the case everywhere.

simison commented 4 years ago

Wp-polyfill itself is just @babel/polyfill if I've understood correct, and then it in turn enques some additional polyfills (such as fetch).

@blowery would it be possible to create lightweight polyfill to target modern browsers?

@gziolo @aduth you two strike as people who might be familiar with this?

blowery commented 4 years ago

would it be possible to create lightweight polyfill to target modern browsers?

Sure, then you could enqueue that instead of the full polyfill. You'd have to match on the UA and pick which one to enqueue. You could even have a few (chrome / safari / firefox / etc) and pick the right one.

You'll still have to transform things that cannot be polyfill'd though, like async functions / optional chaining / etc unless WP wants to go full differential serving and have multiple js bundles to choose from.

aduth commented 4 years ago

Wp-polyfill itself is just @babel/polyfill if I've understood correct, and then it in turn enques some additional polyfills (such as fetch).

More specifically, the additional polyfills are loaded optionally based on browser support.

See also:

https://github.com/WordPress/wordpress-develop/blob/90b0f49/src/wp-includes/script-loader.php#L122-L135

These additional polyfills are not loaded if the browser supports the required feature.

wp_get_script_polyfill does some... "magic"... to synchronously load the polyfill based on the result of a candidate test.

https://github.com/WordPress/wordpress-develop/blob/90b0f49/src/wp-includes/script-loader.php#L168-L219

sgomes commented 4 years ago

Wp-polyfill itself is just @babel/polyfill if I've understood correct, and then it in turn enques some additional polyfills (such as fetch).

And how often does it get updated? Can we count on it being @babel/polyfill@7.7.7 or whatever version for a long time, or does it get updated regularly? This would have an impact on how we differentiate between "older" and "newer" browsers on the server side, while looking at UAs.

sgomes commented 4 years ago

wp_get_script_polyfill does some... "magic"... to synchronously load the polyfill based on the result of a candidate test.

If I'm understanding this correctly, this isn't a great approach in terms of performance. It basically means that we need to wait for the JS engine to boot, then run some tests, then wait a roundtrip + download time to fetch the appropriate polyfill JS file before we can boot the app?

The proposed UA approach would be much better, as fetching the polyfill would happen in parallel with all the other loading work that's happening, and result in a potentially much lower TTI (and maybe FCP/FMP too).

aduth commented 4 years ago

It basically means that we need to wait for the JS engine to boot, then run some tests, then wait a roundtrip + download time to fetch the appropriate polyfill JS file before we can boot the app?

The proposed UA approach would be much better, as fetching the polyfill would happen in parallel with all the other loading work that's happening, and result in a potentially much lower TTI (and maybe FCP/FMP too).

Do you have any measurements you can share for the real-world impact this has?

For the vast majority of browsers, the current logic amounts to a property check which aborts immediately. The load is however much time it takes to evaluate:

<script>document.fetch</script>

Script loading occurs only in legacy environments. Loading the script may incur some additional delay, but (a) the script needs to be loaded anyways and (b) micro-optimizing Internet Explorer load time is not a high priority.

sgomes commented 4 years ago

Do you have any measurements you can share for the real-world impact this has?

I'm happy to take some measurements for clarity. 👍 Could you point me to a good test URL?

Script loading occurs only in legacy environments. Loading the script may incur some additional delay, but (a) the script needs to be loaded anyways and (b) micro-optimizing Internet Explorer load time is not a high priority.

If that is indeed the case, then I think that's quite acceptable; lower performance fits neatly into graceful degradation. It contradicts the previous discussion, however, where there were claims that a smaller set of polyfills still needs to be loaded?

sgomes commented 4 years ago

(Deleted a comment of mine that was based in incorrectly interpreting the code and didn't contribute to the discussion)

aduth commented 4 years ago

It may help to consider an example: https://wordpress.org/gutenberg/

You can see in the source the full extent of what's sent over the wire for the purposes of polyfills:†

<script type='text/javascript' src='https://wordpress.org/gutenberg/wp-includes/js/dist/vendor/wp-polyfill.min.js?ver=7.4.4'></script>
<script type='text/javascript'>
( 'fetch' in window ) || document.write( '<script src="https://wordpress.org/gutenberg/wp-includes/js/dist/vendor/wp-polyfill-fetch.min.js?ver=3.0.0"></scr' + 'ipt>' );( document.contains ) || document.write( '<script src="https://wordpress.org/gutenberg/wp-includes/js/dist/vendor/wp-polyfill-node-contains.min.js?ver=3.42.0"></scr' + 'ipt>' );( window.DOMRect ) || document.write( '<script src="https://wordpress.org/gutenberg/wp-includes/js/dist/vendor/wp-polyfill-dom-rect.min.js?ver=3.42.0"></scr' + 'ipt>' );( window.URL && window.URL.prototype && window.URLSearchParams ) || document.write( '<script src="https://wordpress.org/gutenberg/wp-includes/js/dist/vendor/wp-polyfill-url.min.js?ver=3.6.4"></scr' + 'ipt>' );( window.FormData && window.FormData.prototype.keys ) || document.write( '<script src="https://wordpress.org/gutenberg/wp-includes/js/dist/vendor/wp-polyfill-formdata.min.js?ver=3.0.12"></scr' + 'ipt>' );( Element.prototype.matches && Element.prototype.closest ) || document.write( '<script src="https://wordpress.org/gutenberg/wp-includes/js/dist/vendor/wp-polyfill-element-closest.min.js?ver=2.0.2"></scr' + 'ipt>' );
( window.URL && window.URL.prototype && window.URLSearchParams ) || document.write( '<script src="https://wordpress.org/gutenberg/wp-content/plugins/gutenberg/vendor/wp-polyfill-url.min.7490158b.js?ver=3.6.4"></scr' + 'ipt>' );
( window.DOMRect ) || document.write( '<script src="https://wordpress.org/gutenberg/wp-content/plugins/gutenberg/vendor/wp-polyfill-dom-rect.7e21c103.js?ver=3.42.0"></scr' + 'ipt>' );
</script>

Consulting the Network tab in a capable browser, you should only see wp-polyfill.min.js is loaded, and none of these additional specific polyfill scripts.

there were claims that a smaller set of polyfills still needs to be loaded?

I'm not entirely clear which specific comment this is referring to in the discussion.


† There's a lot of redundant code in the output still. I sense it could probably be trimmed down a fair bit.

sgomes commented 4 years ago

Looks like I misread some of the discussion and may have contributed to a lot of confusion, sorry about that. It's clear to me that wp-polyfill.min.js is always loaded, and the other polyfills shouldn't be loaded in modern browsers 👍

@aduth Looking at your example (and at https://wordpress.org/gutenberg/), we can confirm that wp-polyfill.min.js is always loaded, and it comes to 99KB, or 33KB compressed over the wire, as the original message in the PR indicates. This translates to 2-3s being added to the critical path in "Fast 3G" (as defined by Chrome). There's likely a lot of room for improvement there.

Would your suggestion be to apply the same mechanism that's already in place for the other polyfills in conditionally loading (or splitting up into multiple conditional loads) wp-polyfill.min.js as well? Or would you favour the UA-based approach originally suggested?

aduth commented 4 years ago

The loading of wp-polyfill.min.js could definitely be improved. My understanding is that it was largely a convenience to bundle many polyfills in a single package, and one which is provided directly from Babel. To my understanding, the Babel maintainers have since deprecated the polyfill, so it might be something we'd have to move away from at some point regardless.

Would your suggestion be to apply the same mechanism that's already in place for the other polyfills in conditionally loading (or splitting up into multiple conditional loads) wp-polyfill.min.js as well? Or would you favour the UA-based approach originally suggested?

Before today, my answer may have been: Yes, and it's only not been done largely because of the sheer number of polyfills involved.

I was a bit surprised by the markup I shared in https://github.com/WordPress/gutenberg/issues/21616#issuecomment-614677674 in just how poorly the conditional polyfill loading scales with the number of tests. It's not too bad in its current form, but if this were to scale up to 100 or more polyfills, I think we'd want to at least consider one or both of:

aduth commented 4 years ago

I'm also not opposed to the idea of user-agent-based testing. Speaking for myself, I've avoided it largely by as a combination of (a) fear of the level of accuracy and maintenance involved and (b) appeal in the simplicity of the current solution.

Historically, I've been a big fan of Polyfill.io and how it works to create polyfill scripts dynamically. I doubt we'd be able to use it directly, but there may be parts of it that we can leverage (e.g. mapping features to browser versions).

sgomes commented 4 years ago

UA-based testing definitely has accuracy issues, but it has worked well in the Calypso side of things in particular for one reason: because Calypso went with a whitelist-based approach. That is, it sends the evergreen stuff to only the browsers it recognises, and nothing else.

So there may be some maintenance involved in keeping the whitelist up to date (particularly if Chrome goes ahead with its UA-string deprecation plans), but the failure mode is still just unnecessarily sending more JS to a new browser, rather than accidentally breaking an old one.

In Calypso, we use browserslist-useragent in Node to identify browsers that fit our evergreen browserslist configuration. This can work well with either a relative configuration (last 2 chrome versions) or with an absolute one (chrome >= 80), the latter probably being a better fit for Gutenberg. This is assuming there exists a similar library for PHP, of course.

The other option would be to use the module/nomodule approach. The client could determine at runtime whether to load a smaller or larger set of polyfills based on whether it has support for ES modules. The problem with using this approach for a polyfill script is that <script type=module> behaves as if the defer attribute were included, which changes assumptions about script ordering and execution timelines.

aarontgrogg commented 4 years ago

I don't understand how more people are not commenting on this: Uncaught SyntaxError: missing ) after argument list

It points to this code: <script id="wp-polyfill-js-after"> ( "fetch" in window ) || document.write( "<script src="https://demenz-kl.de/wp-includes/js/dist/vendor/wp-polyfill-fetch.min.js"></scr" + "ipt>" );( document.contains ) || document.write( "<script src="https://demenz-kl.de/wp-includes/js/dist/vendor/wp-polyfill-node-contains.min.js"></scr" + "ipt>" );( window.DOMRect ) || document.write( "<script src="https://demenz-kl.de/wp-includes/js/dist/vendor/wp-polyfill-dom-rect.min.js"></scr" + "ipt>" );( window.URL && window.URL.prototype && window.URLSearchParams ) || document.write( "<script src="https://demenz-kl.de/wp-includes/js/dist/vendor/wp-polyfill-url.min.js"></scr" + "ipt>" );( window.FormData && window.FormData.prototype.keys ) || document.write( "<script src="https://demenz-kl.de/wp-includes/js/dist/vendor/wp-polyfill-formdata.min.js"></scr" + "ipt>" );( Element.prototype.matches && Element.prototype.closest ) || document.write( "<script src="https://demenz-kl.de/wp-includes/js/dist/vendor/wp-polyfill-element-closest.min.js"></scr" + "ipt>" ); </script>

Note that document.write uses double-quotes, but so does the script src, which breaks in the browser...

This is a big bug, that has been happening for a long time...

ciampo commented 3 years ago

The problem with using this approach for a polyfill script is that Githubissues.

  • Githubissues is a development platform for aggregating issues.