Make WordPress Core

Opened 15 years ago

Closed 18 months ago

Last modified 13 months ago

#12009 closed enhancement (fixed)

Add support for HTML 5 "async" and "defer" attributes

Reported by: otto42's profile Otto42 Owned by: joemcgill's profile joemcgill
Milestone: 6.3 Priority: high
Severity: normal Version: 4.6
Component: Script Loader Keywords: has-patch commit has-dev-note
Focuses: performance Cc:

Description (last modified by scribu)

HTML5 supports async and defer attributes on script tags: http://www.w3.org/TR/html5/semantics.html#attr-script-async

Basic usage of these:

  • "async" scripts get executed asyncronously, as soon as they're loaded. This lets them run without slowing down the page parsing (normally, all page processing stops while the javascript code is executing).
  • "defer" scripts get deferred from running until page processing is complete. Sorta like jQuery(document).ready() does, except without pre-definitions. Faster, in other words, since it's built into the browser.

Correct usage would dictate that "libraries" like jQuery and such would get the async attribute, while bits of code that use the current DOM would get deferred. The defer bit is basically optional though, since most all code that exists uses something like jQuery(document).ready() already, when it's necessary, and so there's not a lot of benefit there.

The just released Firefox 3.6 supports the async attributes, so you can do testing with these immediately. I've noticed a speedup on the wp-admin side of things by using it, but I have not measured this and cannot be sure I'm not imagining it. Still, it does seem like it makes the page appear faster.

Attachments (6)

defer and async.patch (2.5 KB) - added by scep 13 years ago.
async_defer_scripts.patch (6.4 KB) - added by wpnook 8 years ago.
Adds defer/async attribute option for wp_enqueue_scripts
async_defer_scripts_new.patch (3.4 KB) - added by wpnook 8 years ago.
Removes code unrelated to this ticket
async_defer_scripts_new.2.patch (3.4 KB) - added by wpnook 8 years ago.
Removes leftover print_r
async_defer_12009.patch (2.5 KB) - added by vanaf1979 5 years ago.
async_defer_12009-2.patch (2.5 KB) - added by vanaf1979 5 years ago.
Diff in the correct direction

Download all attachments as: .zip

Change History (172)

#1 @scribu
15 years ago

  • Milestone changed from Unassigned to 3.0

We could add a new parameter to the script loader: load_type, with one of the following values:

  • (normal loading)
  • 'defer'
  • 'async'

#2 @nacin
15 years ago

  • Milestone changed from 3.0 to Future Release

This seems like a good idea to eventually do, but no patch or traction yet. Future release for now.

#3 @ptahdunbar
14 years ago

  • Cc trac@… added
  • Keywords needs-patch added

#4 @azaozz
14 years ago

This sounds good but to implement it and actually use it we would need at least the majority of the browsers in use to support it properly (as per the HTML5 specs). This seems to be "very far in the future" for now.

#5 follow-up: @scribu
13 years ago

  • Description modified (diff)

Alternatively, we could just add a filter to wp_print_scripts(), allowing users to add attributes when they need to.

As always, users find weird workarounds when the API doesn't oblige. For example, using the clean_url filter:

http://wordpress.stackexchange.com/a/38335/205

#6 @toscho
13 years ago

  • Cc info@… added

#7 @scep
13 years ago

  • Cc scep added
  • Keywords has-patch added; needs-patch removed

I was also looking for this feature, saw that it did not exist so i took a shot at it writing it in, I attached a patch for what I did.

#8 @scep
13 years ago

  • Keywords needs-testing added

#9 in reply to: ↑ 5 @azaozz
13 years ago

Replying to scribu:

As always, users find weird workarounds when the API doesn't oblige...

Agreed. However I'm still feeling a bit "uneasy" about adding support for a feature that is not supported by more than half of the browsers currently in use: http://en.wikipedia.org/wiki/Usage_share_of_web_browsers.

#10 follow-up: @toscho
13 years ago

These attributes are backwards compatible. I don’t think supporting them would do any harm.

But I don’t like more arguments for wp_register_script(). We have already four, that’s more than enough. How about a very simple extra string in WP_Scripts::do_item()?

$src = esc_url( apply_filters( 'script_loader_src', $src, $handle ) );
		
$extra = apply_filters( 'script_extra', '', $src, $handle );

if ( $this->do_concat )
	$this->print_html .= "<script type='text/javascript' src='$src' $extra></script>\n";
else
	echo "<script type='text/javascript' src='$src' $extra></script>\n";

#11 @azaozz
13 years ago

Yes, another filter would do the trick but has many drawbacks. If we decide to use these attributes in core, there will be no good way to add them: it will interfere with what plugins are adding (or removing) in all cases.

#12 in reply to: ↑ 10 ; follow-up: @scep
13 years ago

Replying to toscho:

These attributes are backwards compatible. I don’t think supporting them would do any harm.

But I don’t like more arguments for wp_register_script(). We have already four, that’s more than enough. How about a very simple extra string in WP_Scripts::do_item()?

I agree that since they are backwards compatible they should be supported.

I also agree that my patch adds a lot of extra arguments for the register and enqueue functions. I like you filter idea but it seems like it would lead to having a lot of code in the return function to identify which scripts should have extra info attached to them.

#13 in reply to: ↑ 12 @scribu
13 years ago

Replying to scep:

I also agree that my patch adds a lot of extra arguments for the register and enqueue functions. I like you filter idea but it seems like it would lead to having a lot of code in the return function to identify which scripts should have extra info attached to them.

Every script has a unique ID which it can be identified by (the first parameter to wp_enqueue_script()).

#14 @wycks
12 years ago

Here is the current Compatibility (Aug 2012):

Firefox: yes
Chrome: yes
Safari: yes
Andriod: yes
Blackberry: yes
IE version 10: yes


IE 6, 7, 8, 9: no

Opera: no (and no future plans)


http://caniuse.com/#search=async

ps. For reference the much used google analytic code uses async by setting var ga = document.createElement('script'); ga.type = 'text/javascript'; ga.async = true;

#15 @georgestephanis
12 years ago

Leaving a comment to remind myself to come back and write a patch for this later. This is a great example of graceful degradation that probably should be supported.

#16 @Otto42
12 years ago

I like the patch, but instead of adding new params to the function calls, lets change the $in_footer parameter to be allowed as an array of attributes.

Basically, instead of $in_footer = false, have $attr = false.

For backward compatibility, you could do if $attr === true, then $attr = array('in_footer'=>true).

Then, you can have $attr = an array of the various settings, including potential future ones, allowing you to define whether it's in the footer or not, defer or not, async or not, etc.

Last edited 12 years ago by Otto42 (previous) (diff)

#17 @scribu
12 years ago

Or, we could just throw our hands in the air: add a 'script_loader_tag' and let plugins add whatever attributes they see fit: #13592

#18 @ryanve
12 years ago

The ideal way to handle script/style attributes is via #22249 because it is future-proof for any new attributes that are invented.

The 'script_loader_tag' should be added for consistency with the already existing 'style_loader_tag' to allow for customizations that go beyond attributes, such as wrapping in IE conditionals (#16024) or adding embedded scripts. (#13592 and #22245)

Last edited 12 years ago by ryanve (previous) (diff)

#19 @kevinlangleyjr
11 years ago

  • Cc kevinlangleyjr added

#20 @lkraav
11 years ago

  • Cc leho@… added

#21 @nacin
11 years ago

The 'script_loader_tag' should be added

I agree.

#22 @pothi
11 years ago

  • Cc pothi added

#23 @nacin
11 years ago

  • Component changed from JavaScript to Script Loader

#24 @webaware
11 years ago

A further complication: CloudFlare Rocketscript. This is a service that compresses and bundles scripts from your website into a single download. It does this by replacing your script elements with ones that look like this:

<script type='text/rocketscript' data-rocketsrc='http://example.com/path/to/src.js?ver=1.0'></script>

After the page has loaded, Rocketscript collects those elements all together and loads / executes the scripts. Which means scripts that don't like being loaded asynchronously break of course.

To disable Rocketscript for specific scripts, you can add an attribute data-cfasync="false" as per this support note:

https://support.cloudflare.com/hc/en-us/articles/200169436

However: it only works if the attribute comes before the src attribute.

So I like Toscho's solution, but with $extra added into the script element before the src attribute, not after it.

FYI: this is how I solved the problem for a plugin user, without such extra attribute support; nasty but effective. I'd love to offer a better solution :)

https://gist.github.com/webaware/8949605

#25 @SergeyBiryukov
11 years ago

#27672 was marked as a duplicate.

#26 @grapplerulrich
10 years ago

Could this be closed as the filter script_loader_tag has been added? #13592

#27 @wonderboymusic
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

#28 @ocean90
10 years ago

  • Milestone Future Release deleted
  • Resolution changed from fixed to worksforme

#29 @wpnook
8 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened
  • Version set to trunk

Is there any interest in reviving this topic? I was going to create a ticket, but found this ticket, so I'll submit my patch here. Browser support seemed to be a big part of the debate previously and shouldn't be a concern now.

The lack of options for using the async and/or defer attributes with script elements is a bit of a pain in WordPress, which has resulted in some users coming up with dirty/hackish ways of doing so, such as using the clean_url filter to add the attribute(s).

The script_loader_tag solves this to a degree, but it would be better to specify this option at the time of loading the script (the filter has other uses as well).

This patch adds two new arguments for {{{wp_enqueue_script}} and outputs the results in the printed <script> tag, and both default to false.

Using both tags simultaneously may be beneficial for legacy browser support, and some third-party documentation (Google Maps JS API for example) recommends using the attributes together.

@wpnook
8 years ago

Adds defer/async attribute option for wp_enqueue_scripts

@wpnook
8 years ago

Removes code unrelated to this ticket

#30 @SergeyBiryukov
8 years ago

  • Milestone set to Awaiting Review

@wpnook
8 years ago

Removes leftover print_r

#31 @azaozz
8 years ago

  • Keywords has-patch needs-testing removed

FTR async and defer do pretty much the same thing: begin to download the script in parallel immediately. The difference is that each async script executes after it has finished downloading. It’s likely that async scripts are not executed in the order in which they occur in the page. The defer scripts are guaranteed to be executed in the order they occur in the page (https://webkit.org/blog/1395/running-scripts-in-webkit/).

Then logically:

  • async and defer make little sense when scripts are concatenated.
  • async scripts cannot be used as dependencies.
  • defer scripts can be used as dependencies only if all dependent scripts also have the defer attribute.
  • Neither async nor defer scripts can have any inline scripts.

In that terms the above patch is a good start but is missing a lot. Looks like this will be quite complex to implement in WP_Scripts. Perhaps it can be made a bit simpler by supporting these attributes only for third party scripts, etc.

#32 @mor10
8 years ago

FWIW I support adding the option of async / defer to a regular enqueue call at the individual script level. This should not be an automated process but a developer decision on a script-by-script basis to ensure proper hierarchy etc. Requiring a filter seems excessive considering all we're talking about is two possible properties without variables or values. As HTTP/2 rolls out, this is becoming a more relevant issue because script concatenation is an anti-pattern in the new multiplexed reality.

#33 @seancjones
8 years ago

I think this is an important issue to resolve. As of now, Google Page Speed Insights failing for a lot of WordPress installs can almost exclusively be blamed on render-blocking JavaScript. Fixing that increases speed by wide margins.

Plugin developers should have a way of deferring page loads. There are clear pitfalls to avoid - like people's websites breaking.

I'm going to play devil's advocate and argue why a filter might be better.

A filter allows a user with a code sample to defer scripts, or remove scripts if they appear to break their current setup. Plugin developers can send an array of script handles to defer via a plugin, rather than rewrite their code with a new parameter.

If a user wanted to disable all deferring:

add_filter ( 'wp_defer_scripts', function($wp_defer) {
  return array();
}, 900, 2 );

add_filter ( 'wp_async_scripts', function($wp_async) {
  return array();
}, 900, 2 );

For someone to enable all hooks:

add_filter ( 'wp_async_scripts', function($wp_async) {
    global $wp_scripts;

    return $wp_scripts->queue;
} );

This would also make it far easier for plugin developers to patch their plugins.

#34 @georgestephanis
8 years ago

Personally I'd prefer not to have the defer / async as arguments in the register/enqueue script functions, but require a wp_script_add_data() call afterwards to flag it on -- the same way we do for rtl support.

Quick tangential gist I threw together for testing of something related, if anyone wants to play with deferring now:

https://gist.github.com/georgestephanis/2a84bc55ad23f4dec2cf2464109add59

#35 @LindsayBSC
8 years ago

Personally I would appreciate the allowance for setting script tag attributes when the script itself is registered but I can also see the need for allowing filtering of it. For that reason I put together this little bit of code to append to (and rewrite) the section of do_item in class.wp-scripts.php that does the actual generation of the tag.

My thinking is that the <script> is the actual tag, and everything else including the src and type are attributes. We should be treating attributes similarly and thus I felt it best to generate all of the attributes at the same time. This could also allow plugins to filter the order in which they are output.

It is somewhat of a combination of the methods suggested in #22249 and @Otto42 's suggestions

I didn't have access to my dev environment to create a diff patch file, but here is the code concepts in a multi-site gist with the appropriate file names listed alone with the general line number where the code would go.

https://gist.github.com/LinzardMac/dc951d7fabf3fbeb1f10794c3130fecc

Last edited 8 years ago by LindsayBSC (previous) (diff)

#36 @mor10
7 years ago

async / defer is now considered best practice yet we still can't do it in WordPress. Let's get this sorted.

This ticket was mentioned in Slack in #core-js by mor10. View the logs.


7 years ago

#38 @gziolo
7 years ago

Can we also include the support for nomodule and type.module when dealing with other script attributes? I think there are even more. See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script. I think that support for ES6 modules (https://hacks.mozilla.org/2015/08/es6-in-depth-modules/) would open some great ways to serve smaller bundles for developers writing their code with the latest language features. It still limited to a small number of browsers, but it has this nomodule fallback so it's more than okey to have it available as a progressive enhancement.

#39 follow-up: @azaozz
7 years ago

async / defer is now considered best practice...

Correct me if I'm wrong but both async and defer don't make much difference for scripts at the end of the HTML document (in the footer).

In addition they don't make sense when scripts are concatenated, i.e. in production.

In addition async cannot be used if the script has dependencies or is used as dependency for another script.

So, what are the user cases for these attributes with the current script-loader?

As far as I see the only thing we can do is add defer to the script tag that loads the concatenated scripts in the footer. We probably can add defer when outputting single script tags for scripts that are enqueued for the footer. Not sure if either of these will make any significant difference :)

Also thinking we should have another look at script-loader. Modern JavaScript is about modules and building (pre-concatenating). The current script-loader's dependency model and JIT concatenation may not be the best for it.

Last edited 7 years ago by azaozz (previous) (diff)

#40 in reply to: ↑ 39 ; follow-up: @mor10
7 years ago

Replying to azaozz:

async / defer is now considered best practice...

Correct me if I'm wrong but both async and defer don't make much difference for scripts at the end of the HTML document (in the footer).

The modern best practice is to load JavaScripts in the head section of the document and use async / defer to prevent render blocking. This allows the browser to pull in and cache the JavaScript files while waiting for the DOM to load. See https://developers.google.com/speed/docs/insights/BlockingJS

In addition they don't make sense when scripts are concatenated, i.e. in production.

Concatenating JavaScript is an old patch to get around single-stream transfer under HTTP/1.1. With HTTP/2 multiplexing it is an anti-pattern that slows down performance. Componentizing, loading, and async / defer scripts in head + theoretically leveraging server-push dramatically improves performance.

In addition async cannot be used if the script has dependencies or is used as dependency for another script.

Not entirely accurate. If you defer a group of scripts, they will load and run in the document order, so dependencies still work. Putting that aside, this is the reason why async and defer need to be optional attributes that can be added at enqueue level.

So, what are the user cases for these attributes with the current script-loader?

One immediate use case: navigation.js in _s should be loaded in head and deferred. Same with skip-link-focus-fix.js. Doing so removes render blocking and improves performance. As I said, async / defer is now best-practice.

As far as I see the only thing we can do is add defer to the script tag that loads the concatenated scripts in the footer. We probably can add defer when outputting single script tags for scripts that are enqueued for the footer. Not sure if either of these will make any significant difference :)

IMO async / defer should have their own optional parameters in wp_enqueue_script() and wp_register_script() to give developers the ability to choose whether scripts should be loaded by default behavior, asynced, or deferred. Not doing so stands in the way of modern best practices.

Also thinking we should have another look at script-loader. Modern JavaScript is about modules and building (pre-concatenating). The current script-loader's dependency model and JIT concatenation may not be the best for it.

Probably, though I think that's a separate conversation. async / defer should be handled immediately.

#41 @mor10
7 years ago

There is significant overlap between this ticket and #22249.

In my view, async / defer is a separate conversation from general script attributes as these specific attributes are booleans and play unique roles in how the browser renders scripts. Separating the two conversations will allow us to move forward on solutions for each of them in a more meaningful way. Right now there is a lot of crosstalk between the tickets and the two different issues are discussed as if they are one and the same, which in my opinion they are not.

#42 follow-up: @westonruter
7 years ago

  • Milestone changed from Awaiting Review to Future Release

In terms of async_defer_scripts_new.2.patch, and as @Otto42 commented, I think that we should be getting away from adding new positional parameters to wp_enqueue_script(). Already they are hard to remember. Adding two additional ones will just make it harder to use. I suggest we rework wp_register_script() to allow passing the $deps, $ver, and $in_footer as part of an $args array, which would allow for them all to be omitted (since they are optional) as well as to easily allow for new parameters including async, defer, and module. For example:

<?php
wp_register_script( 'foo', plugins_url( 'foo.js', __FILE__ ), array(
    // Look ma, no $deps or $ver!
    'in_footer' => true,
    'async' => true,
    // ... 'defer', 'module', etc.
) );

Same pattern can be applied to wp_enqueue_script().

Last edited 7 years ago by westonruter (previous) (diff)

#43 in reply to: ↑ 40 ; follow-ups: @azaozz
7 years ago

Replying to mor10:

Concatenating JavaScript is an old patch to get around single-stream transfer under HTTP/1.1. With HTTP/2 multiplexing...

All of this sounds pretty good, in theory. I wish we could do that right now. The problem is that less than 25% of the websites currently support HTTP/2: https://w3techs.com/technologies/details/ce-http2/all/all.

In addition async cannot be used if the script has dependencies or is used as dependency for another script.

Not entirely accurate.

The async attribute cannot be used in script-loader because the script doesn't follow execution order. If you are adding a stand-alone script that has no dependencies and other scripts don't depend on it, there is no point adding it to script loader at all.

Right now plugins and themes can load all scripts in the head and add defer if they really want by using the 'script_loader_tag' filter. This will make 75% of sites slower. Whether we should make this easier is another question. I'd rather we don't.

#44 in reply to: ↑ 43 ; follow-up: @mor10
7 years ago

Replying to azaozz:

All of this sounds pretty good, in theory. I wish we could do that right now. The problem is that less than 25% of the websites currently support HTTP/2: https://w3techs.com/technologies/details/ce-http2/all/all.

This is where I say we (WordPress, 30% of the web if you believe the stats) have a role to play in moving the web forward. We also have a role to play in making sure we don't hold the web back. While HTTP/2 is still emerging, it is rapidly doing so, and the uptick will increase once Chrome and Firefox release their "this site is not secure" flags and GDPR come into effect. The way WordPress handles scripts today is not the way the web of right now handles scripts, nor the way modern browsers and servers want us to handle scripts. We need to push forward on this to establish best practices and grant those who want to build modern web applications the capability to do so without having to build around how WordPress handles JavaScript.

#45 in reply to: ↑ 42 @mor10
7 years ago

Replying to westonruter:

For example:

<?php
wp_register_script( 'foo', plugins_url( 'foo.js', __FILE__ ), array(
    // Look ma, no $deps or $ver!
    'in_footer' => true,
    'async' => true,
    // ... 'defer', 'module', etc.
) );

Same pattern can be applied to wp_enqueue_script().

This would be a solid step forward IMO. It makes it easier to understand what's happening, and allows deeper level of control. The one thing I'd add is we need a way of preventing someone from accidentally setting both async and defer to true at the same time as they conflict.

#46 in reply to: ↑ 44 ; follow-up: @azaozz
7 years ago

Replying to mor10:

While HTTP/2 is still emerging, it is rapidly doing so, and the uptick will increase once Chrome and Firefox release their "this site is not secure" flags and GDPR come into effect. The way WordPress handles scripts today is not the way the web of right now handles scripts, nor the way modern browsers and servers want us to handle scripts.

I agree, in principle. But then why do this a teaspoon at a time? Lets implement it "properly" for all current scripts.

Currently there are few tickets that touch on some parts of updating/refactoring script-loader. IMHO best would be to combine them and really refactor it. Implement HTTP/2 + defer, output all in the head, retire load-scripts.php, etc. etc. Ideally this should be done before Gutenberg lands in core.

Last edited 7 years ago by azaozz (previous) (diff)

#47 in reply to: ↑ 46 ; follow-up: @westonruter
7 years ago

Replying to azaozz:

Currently there are few tickets that touch on some parts of updating/refactoring script-loader. IMHO best would be to combine them and really refactor it. Implement HTTP/2 + defer, output all in the head, retire load-scripts.php, etc. etc. Ideally this should be done before Gutenberg lands in core.

Retiring load-scripts.php I think can also/rather be accomplished via service workers: #36995.

The async attribute cannot be used in script-loader because the script doesn't follow execution order. If you are adding a stand-alone script that has no dependencies and other scripts don't depend on it, there is no point adding it to script loader at all.

It would make sense to me that if you register a script with an async flag and a non-empty deps that this should result in a _doing_it_wrong().

It makes sense to me to use the WP dependency system even if there are no dependencies because it provides a standard programatic interface for registering and outputting scripts. Core or a plugin could register an async script and then it or other plugins could just simply enqueue it and be done with it, as is standard in WP. A plugin could check to see if a script is already registered and prevent registering and enqueueing their own if it already exists. Also no worrying about adding an action to print a manually-crafted script tag. No (less) worrying about duplicated scripts being output.

Currently there are few tickets that touch on some parts of updating/refactoring script-loader. IMHO best would be to combine them and really refactor it. Implement HTTP/2 + defer, output all in the head, retire load-scripts.php, etc. etc. Ideally this should be done before Gutenberg lands in core.

Just to be clear, I'm talking about WP script dependencies generally not load-scripts.php specifically. I think there is good value in adding async and defer support to the existing WP_Scripts system so that we can take advantage of them now. Since you noted that less than 25% of the Web is on HTTP/2, I don't think we should let it be the blocker for us to encourage best practices now for script loading for themes and plugins via async and defer. HTTP/2 definitely won't be available before Gutenberg.

#48 follow-up: @mor10
7 years ago

For clarity: We should not blanket defer all scripts. A decision as to what approach is best (default, async, or defer) needs to be done on a script-by-script basis to optimize performance. The behavior of the two options is significantly different and can be taken advantage of in different circumstances.

Separately, just to add more numbers to the convo, the Can I Use stats on support are relevant: https://caniuse.com/#search=http2

#49 in reply to: ↑ 48 @azaozz
7 years ago

Replying to mor10:

For clarity: We should not blanket defer all scripts. A decision as to what approach is best (default, async, or defer) needs to be done on a script-by-script basis to optimize performance.

I don't think so. Mixing the "old" and "new" way of adding scripts will result in a big mess. A typical user case:

  • I enqueue myscript.js to load the old fashioned way in the footer.
  • It depends on jquery-ui-dialog which depends on most of jQuery UI.
  • However another plugin decides it wants to load jquery-ui-position with defer. Deferred scripts will be executed after the footer scripts which breaks the execution order for myscript.js and possibly some of the other dependencies.

To prevent this from happening we will have to actively ban use of defer for all default scripts in core (not even sure we can do that easily).

In that case, what would be the benefits of a plugin adding its own script with defer?

  • All core dependencies will still have to be loaded in the footer.
  • HTTP2 won't work on 75% of the sites, it will end up slowing them down.

Where is the optimization here? Isn't that rather saying: "We added support for defer but it will not work the way it should".

I'm going to repeat myself: if we want to make this better we should "go for broke", refactor script-loader and fully implement loading of scripts as it should be in 2018. No point of doing little things that don't make much of a difference :)

Last edited 7 years ago by azaozz (previous) (diff)

#50 in reply to: ↑ 47 ; follow-up: @azaozz
7 years ago

Replying to westonruter:

Retiring load-scripts.php I think can also/rather be accomplished via service workers: #36995.

Right. This can be done in couple of different ways. The point is that it makes sense as part of refactoring how we load scripts in general.

It would make sense to me that if you register a script with an async flag and a non-empty deps that this should result in a _doing_it_wrong().

I agree we can add async when registering scripts that don't have any dependencies and can never be used as a dependency. This makes most sense for default core scripts that plugins can use.

Just to be clear, I'm talking about WP script dependencies generally not load-scripts.php specifically. I think there is good value in adding async and defer support to the existing WP_Scripts system so that we can take advantage of them now.

Yes but load-scripts.php is an integral part of how we load scripts. Using async and defer will have to pull a script out of there (prevent concatenation) and break the execution order. We can't do that for any of the default scripts, see the previous comment.

Since you noted that less than 25% of the Web is on HTTP/2, I don't think we should let it be the blocker for us to encourage best practices now for script loading for themes and plugins via async and defer. HTTP/2 definitely won't be available before Gutenberg.

Yep, I realise that. Refactoring how we load scripts will slow down 3/4 of the sites currently. However looking at the speed HTTP/2 is being implemented, that number will be quite lower in six months, and possibly be less than 50% by the end of the year.

I also agree with @mor10 that implementing it will give another push to faster switching to HTTP/2 everywhere.

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


7 years ago

#52 @mor10
7 years ago

Relevant: Introducing HTTP/2 Server Push with NGINX 1.13.9 https://www.nginx.com/blog/nginx-1-13-9-http2-server-push/

#53 @markcallen
7 years ago

I just wanted to add a comment about filters on output. It is perhaps the subject of a new discussion or ticket.

If any output is a html tag, then I believe it should be an array of attribute => value pairs that is filtered.

This gives full control to developers to add, view, edit or remove attributes based on their specific needs. It is more resilient to successive filters being used and more importantly it works for any attributes - current or future.

String manipulation which is relied on too much for a lot of filters is just ugly and messy and often gets implemented assuming that no previous filter has been called.

In the case of this ticket, this approach would allow developers to add async/defer attributes based on their specific implementations. It wouldn't need other updates to core and is future proof to any other cool attributes that are created in the future.

Whether developers use that to enhance or degrade performance is a matter for the developer, in my opinion. I agree with @azaozz, better to refactor script-loader for 2018 - with my point above included on eventual output :)

Last edited 7 years ago by markcallen (previous) (diff)

#54 in reply to: ↑ 50 @westonruter
7 years ago

Replying to azaozz:

Yes but load-scripts.php is an integral part of how we load scripts. Using async and defer will have to pull a script out of there (prevent concatenation) and break the execution order. We can't do that for any of the default scripts, see the previous comment.

Something else to clarify here is that I think @mor10 is primarily eyeing use of async and defer on the frontend where wp-admin/load-scripts.php is not used (or at least I've never seen it used), and in which theme/plugin scripts are not included for concatenation anyway (since they are not in_default_dir).

But in the admin this is also where I think service workers should be used to cache assets instead (as I noted above), and here too this would allow for the scripts to cease being concatenated and so there would be no need to pull out async/defer scripts.

I see value in pushing forward with async/defer support in core for the immediate term, as these are standard performance best practices today. They'd be used primarily by themes and plugins: I can see some core scripts like comment-reply.js get async, but we'd just need to work into the script-loader logic to strip async from any script that is a dependency for another script.

Then as HTTP/2 and service workers become more well supported we can abandon load-scripts.php entirely and potentially make async/defer conditional based on whether HTTP/2 is available, for example.

#55 @westonruter
7 years ago

Also, in regards to concatenation and load-scripts.php... I don't believe there is any reason why an async/defer script would have to be pulled out from being concatenated. Browsers should still execute such scripts just fine if they are loaded synchronously, as these attributes were introduced and could be used even before all browsers supported them. So we could keep serving the scripts inside of load-scripts.php.

#56 @ocean90
7 years ago

In terms of implementation, please let no change the signature of the current functions. Neither adding another argument nor changing the third argument to support an array. I think that's a bad practice even if we have done this in the past.

As @georgestephanis has already suggested in comment:34, using wp_script_add_data() should be the preferred way to add support for this.

But instead of using boolean values we can use a specific key like script-execution which can be defer or async:

wp_script_add_data( 'my-script', 'script-execution', 'defer' );
wp_script_add_data( 'my-script', 'script-execution', 'async' );

#57 follow-up: @westonruter
7 years ago

@ocean90 here's a(nother) plugin implementation (and polyfill) for what you describe:

<?php
/**
 * Add async/defer attributes to enqueued scripts that have the specified script_execution flag.
 *
 * @link https://core.trac.wordpress.org/ticket/12009
 * @param string $tag    The script tag.
 * @param string $handle The script handle.
 * @return string
 */
function wp12009_filter_script_loader_tag( $tag, $handle ) {
        $script_execution = wp_scripts()->get_data( $handle, 'script_execution' );
        if ( ! $script_execution ) {
                return $tag;
        }
        if ( 'async' !== $script_execution && 'defer' !== $script_execution ) {
                return $tag; // _doing_it_wrong()?
        }

        // Abort adding async/defer for scripts that have this script as a dependency. _doing_it_wrong()?
        foreach ( wp_scripts()->registered as $script ) {
                if ( in_array( $handle, $script->deps, true ) ) {
                        return $tag;
                }
        }

        // Add the attribute if it hasn't already been added.
        if ( ! preg_match( ":\s$script_execution(=|>|\s):", $tag ) ) {
                $tag = preg_replace( ':(?=></script>):', " $script_execution", $tag, 1 );
        }

        return $tag;
}
add_filter( 'script_loader_tag', 'wp12009_filter_script_loader_tag', 10, 2 );

Then to make comment-reply.js async all that is needed is as you showed and @georgestephanis suggested:

<?php
add_action( 'wp_enqueue_scripts', function() {
        wp_script_add_data( 'comment-reply', 'script_execution', 'async' );
} );

I like not using a boolean attribute, but the script_execution name doesn't feel quite right yet.

#58 in reply to: ↑ 57 @westonruter
7 years ago

Replying to westonruter:

Abort adding async/defer for scripts that have this script as a dependency. _doing_it_wrong()?

On second thought, I don't think it should do this error auto-detection. Async scripts can still have dependencies, but they are not synchronous dependencies. In other words, there should be a dependency in place so that the async script gets printed as well as its (async) dependency; it's just that in the case of async scripts, the order doesn't matter. If we are going to do any such detection, we perhaps should flag when a non-async script gets printed which itself has an async script dependency. Or rather we should just not worry about warning at all since the developer will find a JS error when they test anyway.

#59 @mihai2u
7 years ago

I concur with weston's train of thought that doing this type of validation seems to go beyond the scope of this ticket.
If you're setting up async, you should know what you're doing and what you're applying it to.

There's a different path this can go in as well...
If you're deferring a script which is usually setup as a dependency (let's think jquery), it would be nice to defer all of the scripts that have it as a dependency, as that will keep everything in functioning order even when one more plug-in is added which has a jQuery dependency, instead of it breaking.
But that's a whole other story altogether, we can find improvements to these in time. It's already been 8 years!!! since this ticket was opened, we should get a simple implementation in.

#60 @nikolay.yordanov
7 years ago

When this is implemented, core inline scripts should be refactored, so that they do not depend on other scripts before DOMContentLoaded. For example, this won't work if jquery.js is deferred: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/script-loader.php#L1286

#61 in reply to: ↑ description @firefly2000
6 years ago

I just want to add my support for the comment by @wpnook that, although the script_loader_tag filter is an acceptable way to add extra attributes like "async" and "defer", it isn't ideal. In my case, I was unable to use this filter because it unexpectedly conflicts with a plugin my client depends upon (UberMenu, for the record).

#62 in reply to: ↑ 43 @lkraav
5 years ago

Replying to azaozz:

All of this sounds pretty good, in theory. I wish we could do that right now. The problem is that less than 25% of the websites currently support HTTP/2: https://w3techs.com/technologies/details/ce-http2/all/all.

As I was looking around for what the latest is for <script module> attribute, I noticed that today (21 months later), "HTTP/2 is used by 41.7% of all the websites.". That's ca +1%/mo tempo or thereabouts. And HTTP/3 QUIC is already knocking on the door.

#63 @lkraav
5 years ago

Looks like @kylereicks has bothered to polyfill [no]module attribute https://github.com/kylereicks/wp-script-module-nomodule

#64 @apedog
5 years ago

New wp_enqueue_script() signature

This comment is about wp_enqueue_script() signature. Not about implementation.
I might be late to the party, but I've not seen any serious discussion on this.

I believe wp_enqueue_script() should get an additional $parsing_attr parameter. Like so:

wp_enqueue_script( $handle, $src, $deps, $ver, $in_footer, $parsing_attr=null )

Where parsing_attr defaults to null but can be set to 'defer' or 'async'. No need to allow for both. defer and async can be mutually exclusive IMO.

Ideally in_footer could have been overloaded, but since a truth-y in_footer value prints the script in the footer, this would invalidate the use of defer and async (which are also truth-y, but useless on scripts loaded in the footer).
So a separate parameter is, alas, the safer way to go.

This seems to me to be the least likely candidate to break old plugins. At the cost of a long, convoluted signature.

Does anything need to happen outside of this ticket in order to change the signature of such a widely-used function?

#65 @vanaf1979
5 years ago

Be “warned” that this is my first patch, so please let me know if I’m doing something wrong! I'm also not sure if there is any interest left in resolving this topic but I thought i’d have a go.

My patch adds two helper functions, wp_async_script and wp_defer_script that both take a handle or an array of handles and set is_async or is_defer meta values on the script objects.

These meta values are then used in the WP_Scripts->do_item() method to add the appropriate attributes only when a script is not added to the footer. (Async and defer are assumed to be pointless in the footer!?)

It also adds two filters, script_attr_async and script_attr_defer making it possible for others to change the values. I’m a bit in doubt here if two, or any, filters is a bit to much!? I tried to combine them but that makes for a pretty weird api.

Thanks for any feedback

@vanaf1979
5 years ago

Diff in the correct direction

#66 @rogeriomoreira
5 years ago

Just a ping to know the status of this ticket. We're only considering defer and async ?

For example, Rocket Loader from Cloudflare uses a different data attribute (data-cfasync="false"), can't we support that too with this patch?

#67 follow-up: @vanaf1979
5 years ago

@rogeriomoreira Maybe their are other (future) attributes. So instead of my patch we could consider a function wp_script_attributes(array('attr' => value)). So we can extend it's functionality when other attributes come on the scene!?

#68 in reply to: ↑ 67 ; follow-up: @rogeriomoreira
5 years ago

Replying to vanaf1979:

@rogeriomoreira Maybe their are other (future) attributes. So instead of my patch we could consider a function wp_script_attributes(array('attr' => value)). So we can extend it's functionality when other attributes come on the scene!?

That can be a solution but I was thinking in something like:

wp_enqueue_script( string $handle, string $src = '', string[] $deps = array(), string|bool|null $ver = false, bool $in_footer = false, array $attributes = [(attribute, value), (attribute, value)])

#69 in reply to: ↑ 68 ; follow-up: @vanaf1979
5 years ago

Replying to rogeriomoreira:

That can be a solution but I was thinking in something like:

wp_enqueue_script( string $handle, string $src = '', string[] $deps = array(), string|bool|null $ver = false, bool $in_footer = false, array $attributes = [(attribute, value), (attribute, value)])

That would also be a possibility. But reading back in this ticket I think an extra attribute for wp_enqueue_script is not the desired solution. That's why i choose the "additional functions" route.

Last edited 5 years ago by vanaf1979 (previous) (diff)

#70 in reply to: ↑ 69 ; follow-up: @rogeriomoreira
5 years ago

Replying to vanaf1979:

Replying to rogeriomoreira:

That can be a solution but I was thinking in something like:

wp_enqueue_script( string $handle, string $src = '', string[] $deps = array(), string|bool|null $ver = false, bool $in_footer = false, array $attributes = [(attribute, value), (attribute, value)])

That would also be a possibility. But reading back in this ticket I think an extra attribute for wp_enqueue_script is not the desired solution. That's why i choose the "additional functions" route.

Makes sense 👍, do you need any help with this patch?

#71 in reply to: ↑ 70 @vanaf1979
5 years ago

Replying to rogeriomoreira:

Makes sense 👍, do you need any help with this patch?

Thank you. I have a patch up above for wp_async_script and wp_defer_script functions and i'm basicaly waiting for feedback on that. I could also do a patch for a wp_script_attributes function. But i think it's best to get some feedback first!?

#72 follow-up: @apedog
5 years ago

I don't think there is going to be a problem with a new $attributes parameter.
It does not break back-compat.
And should be compatible with other script tag attributes, like type="module" etc.

Reading through this ticket, I don't think the implementation/patches are the bottle-neck. This seems to be blocked on the API. API needs to stay backward-compatible AND future-proof (ie. allow not only for defer/async but also for other attributes that might be relevant to the HTML script tag). No one's going to look at the patches as long as an API is not agreed upon.

#73 @vanaf1979
5 years ago

@apedog Fair enough. I did add my patch to "revamp" this api discussion, and see if there is any interest left in resolving this topic. Considering your feedback i think @rogeriomoreira sugestion would be a better solution :)

Last edited 5 years ago by vanaf1979 (previous) (diff)

#74 in reply to: ↑ 72 @azaozz
5 years ago

Replying to apedog:

I don't think the implementation/patches are the bottle-neck. This seems to be blocked on the API. API needs to stay backward-compatible AND future-proof

Exactly! As mentioned in earlier comments, there is no problem to add async and defer attributes. The problem is what happens after. See:

#75 follow-up: @azaozz
5 years ago

Quick tl;dr to get everybody on the same page (I know this repeats a lot of stuff, but may make it easier to understand this ticket):

Main functionality of Script Loader:

  • Manage loading order. When a script tag is outputted, make sure all of its dependencies are outputted before it. When scripts are concatenated, reorder them so dependencies are before dependents.
  • Concatenate the default scripts in production. This is a really nice page load speed increase for HTTP/1.1, but doesn't make difference for HTTP/2.

Additional functionality:

  • List all default scripts and their dependencies.
  • Allow plugins to add scripts to that list and use the default scripts as dependencies.
  • Support outputting of inline scripts before and after each script. (That's mostly used to add translation strings or settings before a script is outputted, or for things like jQuery.noConflict() after a script.)

In these terms:

  • Adding async to a script tag makes the script incompatible with Script Loader:
    • Cannot have dependencies.
    • Cannot be used as a dependency for other scripts.
    • Can be concatenated with other scripts only if all scripts have async. Then the script tag for the combined scripts will also have async.
  • Adding defer makes a script partially incompatible with Script Loader:
    • Can have dependencies.
    • Can be used as a dependency for other scripts but only if the other scripts also have defer.
    • Can be concatenated with other scripts only if all of the scripts have defer. Then the tag for the concatenated scripts will also have defer.

In order to support the async and defer attributes in Script Loader the following logic has to be added:

  1. Generally it doesn't make sense to have scripts with async. They cannot be used as dependencies when managing the load order, and won't gain anything by being part of Script Loader. Following the best practices (mentioned in previous comments), scripts with async should be outputted individually in the HTML head.
  1. Scripts with defer can be added to Script Loader but will need some additional logic when processing the dependencies.
    • It doesn't make sense to add defer to the default scripts that may be used as dependencies (that's most of them). The reason is that in production where scripts are concatenated defer can only be used when all concatenated scripts have it.
    • Default scripts that are not used as dependencies can have defer. However they either have to be concatenated separately, or outputted individually.
    • Scripts added by plugins can have defer if they are not used as dependencies, or when all of their dependent scripts also have defer.
    • The inline scripts that are outputted "after" will also need the defer attribute. It would be nice to also add defer to the "before" scripts but that's not critical.

A possible way to support defer would be to split the footer queue in two then concatenate and output the scripts without defer, and then either output the scripts with defer individually or concatenate them and output them in another script tag.

However Script Loader won't be able to output the deferred scripts in the HTML head. In some cases scripts may be added to the footer queue mid-page. When that happens, and the script doesn't have defer, and one of its dependencies is already outputted in the head with defer, the script will most likely fail to initialize correctly.

Last edited 5 years ago by azaozz (previous) (diff)

#76 @iandunn
4 years ago

I just noticed that HTTP/2 adoption recently passed the 50% mark. It looks like the rate of adoption has slowed down from 10% a year to about 7%.

#77 @lkraav
4 years ago

Was looking into current state of systemic solutions for this, until something lands in core.

https://github.com/mindkomm/theme-lib-script-loader-tags#use-name-suffixes looks like a solid approach, repo last updated 2 months ago.

Append any of the following suffixes to the name of your script to automatically set the attribute:

|async for an async attribute
|defer for a defer attribute
|nomodule for a nomodule attribute
|module for type="module" attribute

like

wp_enqueue_script( 'js-picturefill|async', get_theme_file_uri( 'build/js/picturefill.js' ) );

#78 @westonruter
3 years ago

In addition to comment-reply, another great script candidate for async/defer is wp-embed.

This ticket was mentioned in Slack in #core-js by gziolo. View the logs.


3 years ago

#80 in reply to: ↑ 75 @davilera
3 years ago

The inline scripts that are outputted after will also need the defer attribute.

According to this, defer "must not be used if the src attribute is absent (i.e. for inline scripts)," because "in this case it would have no effect."

This ticket was mentioned in Slack in #core-js by adamsilverstein. View the logs.


3 years ago

#82 @hellofromTonya
22 months ago

#43373 was marked as a duplicate.

#83 follow-up: @10upsimon
22 months ago

Work has commenced on a solution that takes into consideration the original essence of this Trac ticket, along with many of the suggestions made via various - and very valid - comments herein, particularly the comment at https://core.trac.wordpress.org/ticket/12009#comment:75.

Said work that is currently underway introduces enhancements to the Scripts API that allow loading strategies to be applied to scripts (and inline scripts) during registration. Supported strategies are as follows:

  • blocking (current default, not necessary to specify)
  • defer
  • async

Strategies will be applied in a “most eligible” manner, based on various business rules such as the scripts dependency tree & inline script associations to name but two.

More context on this feature can be found in the feature proposal blog post.

Interested parties are encouraged to follow the ongoing development in the 10up WordPress development repository by following issues & pull requests labeled “Script Loading Strategy”.

Any early feedback is much appreciated.

#84 @flixos90
22 months ago

  • Owner changed from azaozz to 10upsimon
  • Status changed from reopened to assigned

#85 @joemcgill
21 months ago

  • Milestone changed from Future Release to 6.3
  • Priority changed from normal to high

I'm adding this to the 6.3 milestone and marking as high priority, since this is one of the next important objectives from the core performance teams roadmap for this year. See: https://make.wordpress.org/performance/roadmap-2023/

#86 in reply to: ↑ 83 @azaozz
21 months ago

Replying to 10upsimon:

Work has commenced on a solution that takes into consideration the original essence of this Trac ticket, along with many of the suggestions

Sounds great!

Said work that is currently underway introduces enhancements to the Scripts API that allow loading strategies to be applied to scripts...

Any early feedback is much appreciated.

There have been some changes in priorities over the last few years. For example concatenation of scripts in wp-admin is likely to be retired soon-ish as more and more sites are HTTP/2 or HTTP/3 enabled (also it has been mostly broken since WP 5.0). So support for concatenation would not be a priority and probably won't be necessary. This would simplify it a bit.

It still doesn't seem to make sense to support scripts with async in Script Loader. These cannot have dependencies, cannot be used as a dependency, and cannot have inline scripts before or after. Ideally plugins and themes that want to use them would output the script tags themselves on one of the <head> actions. Alternatively Script Loader may have a separate queue for them just so wp_enqueue_script() can be used, but not sure how much sense that would make.

Seems scripts with defer can have dependencies that are "blocking" (i.e. without defer). They can also have inline scripts that load before but cannot have after scripts.

Also seems that scripts with defer can be used as dependencies, but the dependent script will have to have defer too. This seems to make it possible to have a mix of defer and "blocking" scripts as dependencies which may be a bit messy.

To prevent problems a "blocking" script should not be able to use a defer script as a dependency. In theory that may be possible if the defer attribute is removed, but that seems to additionally complicate things and may bring edge cases. Also attempting to add an after script to a script with defer should not be possible. Thinking these two should probably throw a doing_it_wrong.

Another big development is the possibility to use JavaScript modules, see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules. This is a significant change in how JS is handled by the browsers that brings many enhancements and new features like deferred by default, loading of modules when needed, etc.

It seems that JavaScript modules would be pretty nice for the block editor/Gutenberg, and are outside the scope of Script Loader.

#87 @joemcgill
20 months ago

  • Focuses performance added

This ticket was mentioned in PR #4391 on WordPress/wordpress-develop by 10upsimon.


20 months ago
#88

  • Keywords has-patch has-unit-tests added

#89 follow-up: @10upsimon
20 months ago

We have completed an initial implementation for this ticket and just opened a new pull request for feedback. For background on the approach we’ve taken, you can reference the feature proposal that @adamsilverstein posted last December.

The approach we’ve taken will enhance the Scripts API to allow developers to declare intended loading strategies for scripts (and inline scripts) during registration by replacing the current $in_footer param with a new $args param of type array that supports declaring both in_footer and strategy keys in wp_register_script and wp_enqueue_script functions. This change is fully backward compatible with the previous function signatures.

The logic automatically handles resolving conflicting strategies in the dependency tree so that scripts will be printed using the most eligible strategy to avoid breaking script execution. This should cover all of the concerns @azaozz has helpfully mentioned in this comment and elsewhere.

While this work does add attributes to the script tags in order to enable a loading strategy, it does not allow direct control of custom script attributes to ensure that dependencies always maintain their correct loading order. This has been discussed in a few comments within this Trac ticket.

I'm working on an update post with additional context to publish on the Make WordPress Core blog soon.

@westonruter commented on PR #4391:


20 months ago
#90

When reviewing this PR, I was noticing how methods in WP_Scripts construct a lot of <script> tags manually with string concatenation, printf, variable interpolation, and calls to esc_attr()/esc_url(). Since WordPress 5.7, there have been dedicated helper functions (wp_get_inline_script_tag() and wp_get_script_tag()) which do all of the heavy lifting, as well as adding features like allowing the attributes to be filterable (for the sake of CSP). I've opened a draft PR (https://github.com/10up/wordpress-develop/pull/58) for this branch which proposes using them. If desired, I can fix up the tests.

@adamsilverstein Do you know why these functions weren't utilized in WP_Scripts when they were introduced? I seem to recall this was punted for a later effort, but I can't find that info off hand.

@adamsilverstein commented on PR #4391:


20 months ago
#91

@adamsilverstein Do you know why these functions weren't utilized in WP_Scripts when they were introduced? I seem to recall this was punted for a later effort, but I can't find that info off hand.

@westonruter I don't recall but agree it would make sense to use the helpers if possible.

@westonruter commented on PR #4391:


20 months ago
#92

I've been doing a bunch of research this week on `wpLoadAfterScripts()`, specifically how it relates to Content-Security-Policy (CSP). Here are my findings and recommendations.

# Vulnerability with Strict CSP and Deferred Inline Script Nonces

Previously, this function relied on eval() which I was concerned about in relation to CSP because it requires the `'unsafe-eval'` source expression. So that was replaced with the current approach of cloning the script, setting the type to text/javascript, and replacing the original. See prior discussion. While this successfully eliminates eval() and the need for 'unsafe-eval' it actually introduces a separate CSP security problem which is quite subtle.

Google has published a recommended "Strict CSP" configuration which includes a nonce requirement for scripts as well as the `'strict-dynamic'` source expression (available in all current browsers):

<blockquote cite="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/script-src#strict-dynamic">The <code>'strict-dynamic'</code> source expression specifies that the trust explicitly given to a script present in the markup, by accompanying it with a nonce or a hash, shall be propagated to all the scripts loaded by that root script. At the same time, any allowlist or source expressions such as <code>'self'</code> or <code>'unsafe-inline'</code> will be ignored.</blockquote>

Let's say you have served a page with the following CSP (where the hash is for wpLoadAfterScripts('myasync')):

{{{http
Content-Security-Policy: script-src 'nonce-r4nd0m' 'unsafe-inline' 'strict-dynamic' 'unsafe-hashes' 'sha256-etusux0wzH3DpYQntoMjmI5AaInUBIQZ02VvSOUZqRM=' https: http:;
}}}

And your page contains the following HTML, in addition to the wpLoadAfterScripts() function:

{{{html
<script

id="myasync-js"
src="/async-script.js"
nonce="r4nd0m"
async
onload="wpLoadAfterScripts('myasync')"

</script>

<script

type="text/template"
data-wp-executes-after="myasync"
nonce="r4nd0m"

goodCode();

</script>

<script

type="text/template"
data-wp-executes-after="myasync"
nonce="bad"

badCode();

</script>
}}}

Notice how the first two scripts have the required nonce set to r4nd0m whereas the last one has the incorrect value of bad. Imagine the last one was injected by a bad plugin due to an XSS vulnerability. Surprisingly, when loading the page all three scripts are executed, including the bad one! The reason for this is that due to 'strict-dynamic', the script containing wpLoadAfterScripts() is trusted, and anything that it causes to be executed is then also marked as trusted. Because it is responsible for transforming the script containing badCode(), then it inherits the trust, resulting in a vulnerability.

How to resolve this? We'll need to manually make sure that the nonce value on any script[data-wp-executes-after] matches any nonce on the script that defines wpLoadAfterScripts(). If it doesn't match, then the transformation must be aborted with an error.

(In order for the nonce attributes to be inserted, this PR would need to incorporate the core helper functions for generating scripts, à la https://github.com/10up/wordpress-develop/pull/58.)

I made a playground where this issue can be experimented with: https://wp-scripts-csp-test.glitch.me/

# Strict CSP Blocked by unsafe-hashes Requirement

In Core-32067/Core-39941, the goal was to eliminate all event handler attributes so that CSP could be enabled without the `'unsafe-hashes'` source expression, again which Google has recommended in Strict CSP. Nevertheless, the current implementation relies on onload event handler attributes on async and defer scripts in order to determine when they have been evaluated. If you have three deferred scripts on your page with handles foo, bar, baz, you'd need to send the following hashes with each response along with the 'unsafe-hashes' source expression:

onload Attribute | Hash


wpLoadAfterScripts('foo') | 'sha256-Ee1+yJYa6yp69Bpyc8WzdW/BwXqYX6sN9QdghtWpji0='
wpLoadAfterScripts('bar') | 'sha256-0MPDmtlmqtrR1Si9bhk41jPvVisj3Ny04NPa9vJMp94='
wpLoadAfterScripts('baz') | 'sha256-2dMqbYMnQ/sqpnKP0R3wd5J8Ch7XLbuqL8MThCprFUI='

This adds to the response payload and it is cumbersome to account for when constructing the CSP. Not only this, but it also can be infeasible to actually send these hashes with the response in the first place since at the time that a script is enqueued/printed, the Content-Security-Policy header may have already been sent (or meta tag printed).

These issues can be mitigated by eliminating the script handle from being passed into wpLoadAfterScripts(). Instead, this can be passed and the handle can be obtained from the id of the script. In that way, wpLoadAfterScripts() could be changed as follows:

{{{diff

  • function wpLoadAfterScripts( handle ) {

+ function wpLoadAfterScripts( script ) {
+ var handle = script.id.replace(/-js$/, );
}}}

Then every deferred script on the page could include this one onload attribute wpLoadAfterScripts(this), and the only hash needing to be sent would be its 'sha256-Onzuq/5md033r0rQOHZyuLqaijp0YWPfo9J1i5b/iio='.

Nevertheless, ideally we'd eliminate the need for 'unsafe-hashes' entirely, which would mean getting rid of the onload event handler attribute. This turns out to not be so simple. Instead of:

{{{html
<script id="foo-js" async src="/foo.js" onload="wpLoadAfterScripts(this)"></script>
}}}

Consider the following code:

{{{html
<script id="foo-js" async src="/foo.js"></script>
<script>
document.getElementById("foo-js").addEventListener("load", function () {

wpLoadAfterScripts(this);

});
</script>
}}}

If you try this, it does seem to work. However, it is not entirely reliable. If there is a network delay when loading the HTML, and the parser pauses after the first script is printed, it may load and evaluate foo.js before the second script is parsed and evaluated. This would mean that the load event would have already fired, and the addEventListener call would be too late. I made a demo that shows how this can be consistently reproduced: https://async-script-load-event-listener-test.glitch.me/

I experimented with several ways of listening for the load event on deferred scripts: https://async-script-load-listeners.glitch.me/

What I think turns out to be the most robust is to leverage MutationObserver. For example, instead of the above code, the following can be employed:

{{{html
<script>
new MutationObserver((records, observer) => {

const script = document.getElementById("foo-js");
if (script) {

observer.disconnect();
script.addEventListener("load", function () {

wpLoadAfterScripts(this);

});

}

}).observe(document.currentScript.parentNode, { childList: true });
</script>
<script id="foo-js" async src="/foo.js"></script>
}}}

Since MutationObserver callbacks are run in microtasks when the parser encounters new DOM nodes, we can be assured that our observer will be invoked before the event loop advances to loading the script[https://async-script-load-listeners.glitch.me/blocking-script-mutationobserver-load-listener.html async]. Indeed, I also found this [works for blocking scripts]. The observer disconnects as soon as it encounters the <code>script</code>, which prevents the observer from negatively impacting the rest of the page's loading.

This would then allow for the Strict CSP to be applied without adding 'unsafe-hashes'.

joemcgill commented on PR #4391:


19 months ago
#93

@westonruter Thanks for the detailed research and proposed approach here:

I've been doing a bunch of research this week on wpLoadAfterScripts(), specifically how it relates to Content-Security-Policy (CSP). Here are my findings and recommendations.

I believe that the only reason any of this is necessary is to ensure that any inline script attached "after" an async script will execute in the correct order. However, considering the previous concerns you've raised regarding being overly conservative about our handling of async script loading here. I wonder if it would be better to avoid this complexity altogether by documenting that anyone attaching an inline script to an async script needs to take responsibility for ensuring loading order is handled correctly. We could potentially create an event bus that would broadcast when an async script has been loaded which would allow dependent scripts to have a consistent way of responding to loading order, if that would help the DX of this pattern.

I'm also curious if the problem you've identified is one we should even be worrying about. If a bad actor could inject the specially crafted markup to take advantage of the potential vulnerability here, what would prevent them from simply executing their script directly, rather than piggy-backing off of this async loading behavior?

@westonruter commented on PR #4391:


19 months ago
#94

I believe that the only reason any of this is necessary is to ensure that any inline script attached "after" an async script will execute in the correct order. However, considering the previous concerns you've raised regarding being overly conservative about our handling of async script loading here. I wonder if it would be better to avoid this complexity altogether by documenting that anyone attaching an inline script to an async script needs to take responsibility for ensuring loading order is handled correctly. We could potentially create an event bus that would broadcast when an async script has been loaded which would allow dependent scripts to have a consistent way of responding to loading order, if that would help the DX of this pattern.

Yes, that seems good to me. In reality, before and after inline positions don't really even make much sense for async scripts. In practice, _both_ inline script positions will be running after. Given that extensible async scripts seem to always have some mechanism for other scripts to call the logic in the async script once it is loaded (e.g. a global variable that is duck typed as an array). For a similar reason, I think async scripts should also be able to have deps since they by nature should be able to execute in arbitrary orders. An event bus could be nice to have, but perhaps it should be _deferred_ for later until there are clear use cases for it.

Note that it is safe to have an inline script printed _after_ a defer script that makes use of addEventListener('load'), so that approach can be taken without resorting to a MutationObserver in a before script which is required for async scripts. So instead of doing the current:

{{{html
<script id="foo-js" defer src="/foo.js" onload="wpLoadAfterScripts('foo')"></script>
}}}

To be compliant with Strict CSP, this should be changed to:

{{{html
<script id="foo-js" defer src="/foo.js"></script>
<script>
document.getElementById("foo-js").addEventListener("load", function () {

wpLoadAfterScripts("foo");

});
</script>
}}}

This would _only_ be done for defer scripts.

(And again, to enable insertion of the CSP nonce attributes in the first place, the script tags need to be printed with the helper functions as in https://github.com/10up/wordpress-develop/pull/58.)

I'm also curious if the problem you've identified is one we should even be worrying about. If a bad actor could inject the specially crafted markup to take advantage of the potential vulnerability here, what would prevent them from simply executing their script directly, rather than piggy-backing off of this async loading behavior?

Yes, it is still a concern, specifically because of the nonce. With an XSS attack, particularly a stored XSS attack, the malicious script being injected into the page is static. The security from CSP nonces comes from the fact that they _should_ be random for each request. So if legitimate scripts being added by the site include the correct nonce value, but the malicious one does not, then the malicious one would be ignored. This however is not the case in the current approach of wpLoadAfterScripts(), as the nonce on the script[type="text/template"] is ignored with 'strict-dynamic' since the trust is inherited by the script containing wpLoadAfterScripts().

#95 follow-ups: @joemcgill
19 months ago

  • Keywords 2nd-opinion added

Thank you to everyone who has provided such useful feedback to PR #4391. @10upsimon and I are currently reviewing this feedback and will be updating the PR in response to the feedback we've gotten thus far.

Of the feedback we've received, there are a few larger proposals that warrant additional conversation before we move forward.

Update WP_Scripts to make use of wp_get_inline_script_tag() and wp_get_script_tag() when constructing script tags

This seems like a natural enhancement to me, and given that @westonruter has already drafted a PR to implement this change, I see no reason not to move forward with this approach.

Reconsider changing the registered loading strategy for async and defer scripts based on their dependents

The current approach was taken with maintaining the execution order of scripts in the dependency tree as a top priority. As a simplified example, this means that if script-1 is registered with a defer strategy, and script-2 is registered with a blocking strategy (or none at all) but it depends on script-1, then WordPress would ensure that script-1 will also be enqueued with a blocking strategy. A similar approach is taken for async scripts.

For defer scripts, this approach probably makes sense. However, for async scripts, there is a valid argument to be made that anyone registering a script as async, or as a dependency of an async script should be responsible for anticipating the unknown execution order of an async script and handle that logic themselves, rather than WordPress trying to resolve conflicts on their behalf. For more, see this conversation on the PR. I'd appreciate additional opinions what WordPress' responsibility should be in this case.

Address CSP concerns with managing inline scripts printed after dependencies

In order to maintain execution order for inline scripts attached to defer and async scripts, we've attempted to control the loading order of those inline scripts by printing them with the type text/template so they're not initially executed, and then executing them after the script they're attached to has loaded. @westonruter added a great explanation about how this can lead to subtle CSP issues that we need to address.

However, as @azaozz suggested above, perhaps we should either not support printing inline scripts after non-blocking scripts at all, or resolve loading order conflicts by forcing a script to be enqueued with a blocking strategy if an inline script is attached that should be printed after. I'd appreciate feedback on these three options:

  1. WP should not support inline scripts printed after a script that is async or defer.
  2. WP should load non-blocking scripts with a blocking strategy if an inline script is registered with the after position.
  3. WP should allow inline scripts after non-blocking scripts, but should ensure the inline script is not executed until after the script it is attached to has loaded (i.e., the current approach) and address CSP concerns.

#96 in reply to: ↑ 95 ; follow-up: @flixos90
19 months ago

Replying to joemcgill:

Update WP_Scripts to make use of wp_get_inline_script_tag() and wp_get_script_tag() when constructing script tags

This seems like a natural enhancement to me, and given that @westonruter has already drafted a PR to implement this change, I see no reason not to move forward with this approach.

Agreed :)

Reconsider changing the registered loading strategy for async and defer scripts based on their dependents

[...]

For defer scripts, this approach probably makes sense. However, for async scripts, there is a valid argument to be made that anyone registering a script as async, or as a dependency of an async script should be responsible for anticipating the unknown execution order of an async script and handle that logic themselves, rather than WordPress trying to resolve conflicts on their behalf. For more, see this conversation on the PR. I'd appreciate additional opinions what WordPress' responsibility should be in this case.

After our conversation earlier on Slack, I feel more inclined towards leaving responsibility for scripts marked async to the developers doing so. However, I think that is a topic separate from "Reconsider changing the registered loading strategy for async and defer scripts based on their dependents", those are two things:

  1. One point is to potentially fall back to another strategy for an async script if e.g. a blocking script depends on it.
  2. Another point is to allow dependencies for async scripts at all.

For the first point, my take based on our conversation earlier is that we should still allow for an async script to fall back to blocking if needed, since that would ensure a load order that satisfies all scripts in the dependency chain: The async scripts basically don't care when they load, so it doesn't matter that the one async script becomes blocking in terms of load order, while for the blocking script it is imperative that the dependency is also blocking so that it is loaded before. Something to consider here though is that an async script should potentially only be eligible to fall back to blocking, but not to a defer strategy since in that case it would be possible for the script to be loaded at a later point than before.

For the second point, I now agree that for scripts added with async we can allow dependencies (as already hinted by my example to explain the first point). We still have to clearly explain the considerations for doing so, but from my current understanding any load order problems from declaring a script async will apply to that script itself, not other blocking or deferred scripts that depend on it. So it's reasonable to allow async dependencies as that won't break other people's code. That said, I'd like to re-emphasize that defer should be encouraged and async needs to be clearly explained on what requirements it has to the scripts that use it.

Address CSP concerns with managing inline scripts printed after dependencies

[...]

  1. WP should not support inline scripts printed after a script that is async or defer.
  2. WP should load non-blocking scripts with a blocking strategy if an inline script is registered with the after position.
  3. WP should allow inline scripts after non-blocking scripts, but should ensure the inline script is not executed until after the script it is attached to has loaded (i.e., the current approach) and address CSP concerns.

I think we should go with option 3, but only for defer scripts, as that will allow us to address the CSP concerns (by using a load event listener instead of onload attribute) while allowing scripts with inline scripts to be deferred still, which is a crucial piece to adoption of the new strategies. Based on our chat earlier today, this approach is technically not reliable for async scripts, and not supporting inline scripts for async scripts is a reasonable trade-off to me (due to their less common use case).

#97 in reply to: ↑ 95 ; follow-ups: @azaozz
19 months ago

Replying to joemcgill:

However, for async scripts, there is a valid argument to be made that anyone registering a script as async, or as a dependency of an async script should be responsible for anticipating the unknown execution order of an async script and handle that logic themselves

+1.

I still don't see why (and how) async scripts can be handled through script-loader. As I said earlier, they cannot be used as a dependency and cannot be dependent on other scripts because of the unknown execution order.

It would be much cleaner and easier if themes and plugins output these scripts themselves at the appropriate *print_scripts* action. There they can also handle any before/after scripts, etc.

In order to maintain execution order for inline scripts attached to defer and async scripts, we've attempted to control the loading order of those inline scripts by printing them with the type text/template so they're not initially executed, and then executing them after the script they're attached to has loaded. @westonruter added a great explanation about how this can lead to subtle CSP issues that we need to address.

Yea, printing inline scripts with text/template type seems pretty "hacky" imho. Lets avoid that even if it means that defer scripts will not support "after" scripts (async scripts shouldn't be in the script-loader at all imho, see above).

I'd appreciate feedback on these three options:

  1. WP should not support inline scripts printed after a script that is async or defer.
  2. WP should load non-blocking scripts with a blocking strategy if an inline script is registered with the after position.
  3. WP should allow inline scripts after non-blocking scripts, but should ensure the inline script is not executed until after the script it is attached to has loaded (i.e., the current approach) and address CSP concerns.

Thinking 1 makes most sense. Keep in mind that "after" scripts are relatively rarely used. So disallowing them for defer scripts would probably not be a problem for the great majority of plugins and themes.

Another possibility is to implement the "after" script in the defer or async script itself. I.e. when the script is being executed it can look for "external" object or function or data and use it when appropriate. That would be the "proper way" imho.

Last edited 19 months ago by azaozz (previous) (diff)

#98 in reply to: ↑ 96 @azaozz
19 months ago

  1. One point is to potentially fall back to another strategy for an async script if e.g. a blocking script depends on it.

Thinking the decision here is about what has the higher priority:

  • A script is intended to be loaded as async.
  • Some plugin or theme decides to use a third-party async script as a dependency.

Imho the first is higher priority than the second because generally async scripts are designed to be stand-alone and/or non-dependent on (old-style) loading order. Hence excluding them from script-loader makes most sense.

(Also keep in mind that it is possible to write JS that does not depend on script loading order. Such JS code can use any script as a dependency, even scripts that are (dynamically) loaded a lot later with AJAX or things like document.write()).

my take based on our conversation earlier is that we should still allow for an async script to fall back to blocking if needed...
The async scripts basically don't care when they load

Not sure if that's a good idea. As far as I see scripts that are loaded as async are "special" and may implement things that should not block the page loading.

For the second point, I now agree that for scripts added with async we can allow dependencies (as already hinted by my example to explain the first point). We still have to clearly explain the considerations for doing so

Again, ideally the script-loader will not support scripts with async. Doesn't seem to make sense to handle them there, even less if the async is automatically removed.

Last edited 19 months ago by azaozz (previous) (diff)

#99 in reply to: ↑ 97 @flixos90
19 months ago

Replying to azaozz:

I still don't see why (and how) async scripts can be handled through script-loader. As I said earlier, they cannot be used as a dependency and cannot be dependent on other scripts because of the unknown execution order.

I think it's a bit more nuanced than that. They cannot be used as a dependency when it comes to load order, however they can still be used as a dependency in that they need to be loaded at all (i.e. "make sure to load these dependencies" vs "make sure to load these dependencies before the main script"). They can always be used as a dependent though, i.e. depending on other scripts. An async script could certainly rely on a blocking script. So I think there's a benefit in supporting async, though we still have to identify how to handle its different implications and how much responsibility for "doing things right" we should leave to the developer using the strategy.

It would be much cleaner and easier if themes and plugins output these scripts themselves at the appropriate wp_print_scripts action. There they can also handle any before/after scripts, etc.

"cleaner and easier" is relative :) As I mention above, the one part that manual handling of async is missing out on is dependency management, which still is relevant even for async scripts. The original proposal we had was to not allow dependencies for async scripts (in which case I would have agreed with you) but based on the latest feedback shared by @westonruter that may not have been the right approach.

Yea, printing inline scripts with text/template type seems pretty "hacky" imho. Lets avoid that even if it means that defer scripts will not support "after" scripts (async scripts shouldn't be in the script-loader at all imho, see above).

I'm not sure how "hacky" it is. In the way outlined (including addressing the CSP concerns) it only involves using a simple inline script to ensure correct load order. Note that it would probably only be for defer scripts regardless of async support since it wouldn't be reliably possible for async scripts anyway without violating CSP.

Also note that not doing the text/template approach doesn't mean defer scripts couldn't support "after" inline scripts at all. They still could, though at that point it would be on the developer to ensure the "after" script is written in a way that works even though its main script is deferred. Not automatically ensuring an "after" script works is one thing, but not supporting it at all would be a notable loss for adoption, since there would not even be a migration path for scripts with "after" scripts; if we still support it but don't do the text/template handling, we can document how an "after" script's code has to be written if it is used on a deferred main script (e.g. wrap it in a load event listener for the main script).

Keep in mind that "after" scripts are relatively rarely used. So disallowing them for defer scripts would probably not be a problem for the great majority of plugins and themes.

That's a fair point, and I think it's worth quantifying that to make an informed decision. Right now, I believe that is simply a guess - a guess that I would share, but it's still a guess :)

#100 in reply to: ↑ 97 @westonruter
19 months ago

Replying to azaozz:

I still don't see why (and how) async scripts can be handled through script-loader. As I said earlier, they cannot be used as a dependency and cannot be dependent on other scripts because of the unknown execution order.

In this case, the scripts themselves handle the proper execution order. This is elaborated on in my PR comment and you can see an example for how an async library can handle the proper loading order in this glitch: https://async-library-script-loading-demo.glitch.me/

In short, for async scripts, the dependencies are not so much about the execution order but more about a bundling mechanism. If you have an async script C that depends on B, and async script B depends on async script A, then doing wp_enqueue_script('C') should automatically also print the scripts for A and B.

In order to maintain execution order for inline scripts attached to defer and async scripts, we've attempted to control the loading order of those inline scripts by printing them with the type text/template so they're not initially executed, and then executing them after the script they're attached to has loaded. @westonruter added a great explanation about how this can lead to subtle CSP issues that we need to address.

Yea, printing inline scripts with text/template type seems pretty "hacky" imho. Lets avoid that even if it means that defer scripts will not support "after" scripts (async scripts shouldn't be in the script-loader at all imho, see above).

I don't think we can avoid after scripts entirely, as then an author can't reliably execute some code when the script loads (without resorting to a hacky `MutationObserver` solution). I'm referring to defer scripts specifically, since the order of inline scripts for async should not be significant. To me it boils down to two options, to whether for defer scripts:

  1. We handle the deferred execution of after inline scripts using the current approach of wpLoadAfterScripts() with its text/template transformation.
  2. We let the inline scripts handle the deferred execution by attaching their own load event listener. For example:
<?php
wp_enqueue_script( 'foo', '/foo.js',array(), array( 'strategy' => 'defer' ) );
wp_add_inline_script( 
  'foo',
  'document.getElementById("foo-js").addEventListener("load", () => { /* ... */ })',
  'after' 
);

Which would output:

<script id="foo-js" src="/foo.js" defer></script>
<script>
document.getElementById("foo-js").addEventListener("load", () => { /* ... */ })
</script>

The nice thing about the first option is that the deferred execution is automatic. The bad thing is that it forces all after inline script logic to run once the defer script is loaded. Perhaps you want some logic to run after but other logic to run before? (Then again, they could just use a before inline script for that.)

Nevertheless, note that there is a somewhat brittle connection here between the after inline script and the defer script: whether there is a script element in the page with the expected id. If there is an optimization plugin that tries concatenating defer scripts together, then any such load event handler will fail because the original script will be gone. (As such, the use of an onload attribute is also brittle.)

In this case, it seems perhaps the safest thing to do is to not wait for the load event on the script[defer] at all, but rather to just attach a DOMContentLoaded event handler to the document which runs after all defer scripts have been evaluated. (The only gotcha here is the defer script may have failed to load.)

I'd appreciate feedback on these three options:

  1. WP should not support inline scripts printed after a script that is async or defer.
  2. WP should load non-blocking scripts with a blocking strategy if an inline script is registered with the after position.
  3. WP should allow inline scripts after non-blocking scripts, but should ensure the inline script is not executed until after the script it is attached to has loaded (i.e., the current approach) and address CSP concerns.

Thinking 1 makes most sense. Keep in mind that "after" scripts are relatively rarely used. So disallowing them for defer scripts would probably not be a problem for the great majority of plugins and themes.

Another possibility is to implement the "after" script in the defer or async script itself. I.e. when the script is being executed it can look for "external" object or function or data and use it when appropriate. That would be the "proper way" imho.

If we require defer scripts to rely on the DOMContentLoaded event, then options 1 & 2 are essentially the same, as authors should instead use before inline scripts. As I mentioned just above, I think option 3 may end up being unreliable due to other plugins munging script tags and losing the id. If the current approach is reworked to rely on DOMContentLoaded instead, then this would be mitigated for option 3.

Last edited 19 months ago by westonruter (previous) (diff)

#101 @azaozz
19 months ago

@flixos90 @westonruter Seems we are not exactly "on the same page" here, lets try to get there :)

The background:

  • Script loader is designed to load (legacy) scripts in a legacy way (that was considered "best practice" in late 2000s and early 2010s). Its main functionality is (in this order):
    1. Allow scripts to be enqueued and declare dependencies.
    2. Ensure the dependencies are added to the queue and moved up so they are outputted before the dependent scripts.
    3. Output scripts in the footer to increase page load performance.
    4. Concatenate scripts to increase page load performance.
  • The async and defer attributes are also "legacy" and can be used to increase page load performance. There are certain differences between them:
    1. Scripts without defer or async:
      • Are loaded synchronously with the rest of the HTML.
      • Are executed as soon as loaded.
      • Can be used as dependencies and have dependents as they maintain load order.
      • Block the parsing of the HTML while being loaded and executed.
      • Generally perform worse than scripts with defer and async (when the script is not in the browser cache).
    2. Scripts with defer:
      • Are loaded asynchronously while the HTML is being loaded.
      • Are executed after the HTML is parsed.
      • Can be used as dependencies and have dependents as they maintain load order.
      • Perform better than scripts with async and "blocking" scripts.
    3. Scripts with async:
      • Are loaded asynchronously while the HTML is being loaded.
      • Are executed as soon as loaded.
      • Cannot be used as a dependency or have dependents as the load order is undetermined.
      • Perform worse than scripts with defer as they block parsing while being executed (may happen in the middle of parsing the HTML).
      • Are executed immediately if cached by the browser. In that case their performance is identical to "blocking" scripts.

So in theory scripts with defer would be most desirable and scripts with async would generally be unusable in Script loader.

In practice that's not as simple. I looked around a bit to find websites that have scripts with async but couldn't find any. That's not very surprising as such scripts cannot be used in conjunction with the DOM. The only practical recommendation was to use async for truly "asynchronous/background" JS like Web Workers, etc. For comparison there are many sites that have scripts with defer.

Imho the next step regarding adding support for scripts with async to Script loader would be to determine what is the practical use of them. It would also help to try to find examples of WP sites that currently have scripts with async especially when the script has dependencies or is used as a dependency.

Last edited 19 months ago by azaozz (previous) (diff)

#102 @azaozz
19 months ago

While looking around for the above comment I also tried to find some comparison data for page load speed with blocking vs. defer vs. async scripts. Unfortunately I couldn't. Ended up doing some quick testing with a 100KB HTML with five scripts ~10KB each. The defer and async scripts were loaded in the head, the blocking scripts were concatenated and loaded in the footer.

The result was somewhat surprising: blocking vs. defer vs. async differences of page load (until DOM ready) were less than 3%. Perhaps I didn't set the test up properly or testing on local serer is not good enough.

Thinking it would be really good to have some performance comparison to justify this. Ideally testing would be on a remote server, with and without browser cache, with concatenated "blocking" scripts (as in production), and on HTTP/1.1 and HTTP/2.0 if possible.

#103 follow-up: @mor10
19 months ago

Hello.

async has significant use cases in situations where a script either needs to be front-loaded or has no impact on the DOM but benefits from being loaded early. Examples include:

  • tracking and analytics
  • independent processes like booting up webworkers
  • performance-critical scripts like cutting the mustard
  • event-driven scripts monitoring user interaction and input
  • offsite interactions like preloading API calls etc
  • loading fonts and other assets via JS (necessary for for-pay foundries etc)

I can think of a myriad of cases where themes and in particular plugins would use async to front-load JS.

Most importantly, loading JS in the header and using async/defer lets both the developer and the browser make better decisions about how to load the scripts.

I wasn't here.

#104 follow-up: @joemcgill
19 months ago

@azaozz Thanks for laying out the background so clearly here. I'm generally in agreement with how you've described the current purpose of the Script Loader, and (other than a few nitpicks) think you’ve summarized the characteristics of each loading strategy well enough. That said, another important consideration for this proposal is that the Script loader also serves as the common API (along with helper functions like wp_register_script and wp_enqueue_script) for developers to use to ensure scripts are included in the HTML that WP generates.

Given that async is a valid loading strategy in HTML spec, that there are many good use cases that developers have for using that strategy, and plugins are already using workarounds to make up for WP's current lack of direct support (see examples), I don't think the question should be if WordPress should support developers registering async scripts, but rather how we should support it.

The current approach taken in PR 4391 tries to resolve potential load order conflicts when async scripts have dependencies by forcing them to be loaded with a defer or blocking strategy (whichever is appropriate). However, as @westonruter has pointed out, it may not make sense for WP to try to control loading order for async scripts in this way, given that by definition the loading order of async scripts is unpredictable and should be accounted for by developers choosing to use this strategy. In other words, async scripts absolutely can have dependencies, they just need to account for the async loading nature of such scripts.

The question I'd like to get consensus on is whether we keep the current approach, which attempts to resolve loading order conflicts (more forgiving of developer mistakes) or if we should trust developers to take responsibility for using async scripts as designed and allow async scripts to have other dependencies that are async. Personally, I'm finding @westonruter's rationale for allowing developers to use async scripts as dependencies and trusting them to do so responsibly fairly convincing. If we can agree there, then we can address additional edge cases and nuances.

#105 in reply to: ↑ 103 ; follow-up: @azaozz
19 months ago

Replying to mor10:

Hello.

Hey, Welcome back! Long time no see :)

async has significant use cases in situations where a script either needs to be front-loaded or has no impact on the DOM but benefits from being loaded early.

Right. That's the theory. However I couldn't find any examples of scripts loaded with async that had dependencies or were dependent on other scripts. (Actually I couldn't find any at all, but didn't look for very long).

To justify adding support for scripts with async to Script loader, it will need at least one established use case where that is needed. If all cases for using async are only for stand-alone scripts there is no point to make Script loader more complex and slower for all other scripts.

I can think of a myriad of cases where themes and in particular plugins would use async to front-load JS.

I can too. The problem is that in almost all of them using defer would be better :)

#106 in reply to: ↑ 105 ; follow-up: @mor10
19 months ago

Replying to azaozz:

Hey, Welcome back! Long time no see :)

I'm not back. This is me from the past adding context to an old issue.

As per MDN: "For module scripts, if the async attribute is present then the scripts and all their dependencies will be executed in the defer queue, therefore they will get fetched in parallel to parsing and evaluated as soon as they are available."

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script

async and defer are not optional features; they are part of the web platform. Adding one but not the other is arbitrary and works counter to the intent of the platform itself. It also makes a decision on behalf of WordPress developers that runs counter to how the web works.

IMO this isn't a "decisions, not options" situation because the decision has already been made by the platform spec. WordPress needs to follow the standard.

#107 in reply to: ↑ 104 @azaozz
19 months ago

Replying to joemcgill:

another important consideration for this proposal is that the Script loader also serves as the common API (along with helper functions like wp_register_script and wp_enqueue_script) for developers to use to ensure scripts are included in the HTML that WP generates.

True. However it doesn't make sense to use that API just to output a <script> tag imho, i.e. waste a bit of resources "just because why not" :)

If this is added to Script loader I think there should be a nice, long explanation in the docs that such use is "not recommended" and a "bad practice" as it only uses a little bit of resources without providing any benefits. However I agree it would be better to support that, even only for completeness.

The current approach taken in PR 4391 tries to resolve potential load order conflicts when async scripts have dependencies by forcing them to be loaded with a defer or blocking strategy (whichever is appropriate).

Yea, that's a good question. According to this it seems async scripts used as a dependency should be loaded as "blocking" (that's the behavior when the script is cached in the browser).

This effectively "bans" use of scripts with async when they use the capabilities of Script loader.

However, as @westonruter has pointed out, it may not make sense for WP to try to control loading order for async scripts in this way, given that by definition the loading order of async scripts is unpredictable and should be accounted for by developers choosing to use this strategy. In other words, async scripts absolutely can have dependencies, they just need to account for the async loading nature of such scripts.

Yep, that makes sense too. In this case async scripts will always be treated as "stand alone" and excluded from calculating any dependencies.

The question I'd like to get consensus on is whether we keep the current approach, which attempts to resolve loading order conflicts (more forgiving of developer mistakes) or if we should trust developers to take responsibility for using async scripts as designed and allow async scripts to have other dependencies that are async.

Generally async scripts can be treated as "blocking", as that's their behaviour when cached. Also both async and defer scripts can have "blocking" dependencies (that will be loaded earlier).

However there haven't been any examples of websites that use async scripts as dependencies. My biggest problem with the current approach is exactly this: it tries to implement theoretical use cases without any evidence any of them would ever be useful.

Personally, I'm finding @westonruter's rationale for allowing developers to use async scripts as dependencies and trusting them to do so responsibly fairly convincing. If we can agree there, then we can address additional edge cases and nuances.

Yea, shifting the decisions to plugin/theme developers seem to make sense here (assuming good inline docs). After all the use cases for async are few and far between compared with the cases for defer. What I'm uncomfortable about are the possible edge cases and nuances. Seems addressing them would add quite a bit of code, and generally make Script loader slower and more complex for a very minimal, if any benefits. Again, imho this would depend on providing (and studying) examples of actual sites that use async scripts as dependencies or dependents.

#108 in reply to: ↑ 106 @azaozz
19 months ago

Replying to mor10:

I'm not back. This is me from the past adding context to an old issue.

Uh, I see :(

As per MDN: "For module scripts, if the async attribute is present then the scripts and all their dependencies will be executed in the defer queue

Right. This is another drawback here, imho. Script loader is the "legacy" way to load scripts. The modern way for modern JS (like Gutenberg/the block editor) is to load modules. Unfortunately Script loader is incompatible with that. I've looked a bit at possibilities to incorporate Script loader with JS modules. Seems possible (perhaps with few few caveats).

I'd really love to see a patch/refactoring of the way WP loads scripts that brings that functionality from 2008 to 2023 :)

async and defer are not optional features; they are part of the web platform. Adding one but not the other is arbitrary and works counter to the intent of the platform itself. It also makes a decision on behalf of WordPress developers that runs counter to how the web works.

Hmm, perhaps there is a bit on misunderstanding here. There are no attempts to "ban" scripts with async in WP. The problems discussed here are about the features of the Script loader API, and how they can or cannot be used with async scripts. I.e. are there any use cases to have async scripts as dependents or dependencies? Extending any API for "theoretical" use cases is not a good idea.

#109 @mihai2u
19 months ago

Using async for a script that is itself a dependent to another script, or has dependencies itself is risky and prone to errors due to not having any guarantee of the order the async script is ran in...
If someone adds async incorrectly, it's their responsibility to fix (plugin / theme developers), and the docs could contain a reference to what is known

"async cannot be used if the script has dependencies or is used as dependency for another script." - @azaozz

If standards change in the future, by supporting any attributes, things will just work, we don't have to hardcore standards and 'magic' fixes in core.. the more predictability you get out of your code, the better.
You get what you ask for should be a good enough mantra for this feature implementation.

#110 @lkraav
19 months ago

Personally, I'm finding @westonruter's rationale for allowing developers to use async scripts as dependencies and trusting them to do so responsibly fairly convincing. If we can agree there, then we can address additional edge cases and nuances.

Yea, shifting the decisions to plugin/theme developers seem to make sense here (assuming good inline docs).

I would trigger _doing_it_wrong(), but I'm not up to date on what core mandates for its usage policy. Other than that, I've found _d_i_w() to be a very well working education tool.

#111 follow-up: @joemcgill
19 months ago

@azaozz, it seems like the crux of your concern is not whether WP should support async scripts, but instead whether this should be part of the Script loader API or handled by some other means. Forgive me if I’m misunderstanding.

True. However, it doesn't make sense to use that API just to output a <script> tag imho, i.e. waste a bit of resources "just because why not" :)

I feel pretty strongly that wp_register_script and wp_enqueue_script (and the Script loader that these rely on) are the primary API used in the WP ecosystem for printing any kind of script, regardless of whether they have dependencies. In any case, there are several reasons to support async specifically via these APIs rather than requiring developers use other manual processes:

  • The Script loader reduces the amount of code developers need to use to print a script. If not using the Script loader, you’d have to manually print the <script> tag yourself at wp_head or wp_footer, which would mean there is no standard way to attach inline scripts.
  • Using the Scripts API allows other plugins to dequeue an async script in the standard way. Otherwise, the only way to prevent the script from being printed is to hunt around for the function that prints the script, and pass it to remove_action(), where hopefully a closure isn’t used and/or a method’s class instance variable is available.
  • There are already over 3,000 examples of plugins adding async scripts to the page. Many of which resort to manually printing the script tag specifically because they can’t use wp_enqueue_script(). See this inline comment example.

Additionally, async scripts can absolutely support dependencies. I’m adding several examples below. As an aside, registering async scripts with dependencies (even if the load order cannot be assumed) allows both first-party and third-party projects to ensure all dependencies are included in the markup whenever the single script relying on those dependencies is enqueued.

Async scripts are already being used with before/after inline scripts. Using the Scripts API allows inline scripts to be easily attached, including by other plugins. See the following:

Hopefully, these are helpful real world examples. Props to @westonruter for helping me pull these examples together. I also want to address a last point:

If all cases for using async are only for stand-alone scripts there is no point to make Script loader more complex and slower for all other scripts.

The way that we’ve implemented logic to resolve loading strategy conflicts in the dependency tree is already in place for defer and async. Removing async logic will not make any meaningful difference in performance or complexity.

Based on all of this, I propose the following next steps:

  1. We agree to support async scripts in the script loader with dependencies (this is already implemented, but we can address specifics in the PR).
  2. We iron out remaining issues for how inline scripts are handled for defer and async scripts.

#112 in reply to: ↑ 111 ; follow-up: @azaozz
19 months ago

Replying to joemcgill:

Think I understand where you're coming from, the desire to add support for all possible cases. That's not a bad way to look at it. However each way of loading scripts has its specifics which should be reflected in the Script Loader API imho. For example it looks like "after" scripts for async and defer scripts should be implemented in the scripts themselves, not in PHP.

@azaozz, it seems like the crux of your concern is not whether WP should support async scripts, but instead whether this should be part of the Script loader API or handled by some other means.

Yes, that's one of the concerns. It is already "handled by some other means" in "over 3,000 plugins". So the question becomes more like: How many of these 3,000 plugins will switch to use Script Loader in the first year after support for async is introduced? Would it be considered a failure if less than 10% of them switch?

The Script loader reduces the amount of code developers need to use to print a script. If not using the Script loader, you’d have to manually print the <script> tag yourself at wp_head or wp_footer, which would mean there is no standard way to attach inline scripts.

That's not exactly true :)

There are the several hooks that can be used to fine-tune when scripts without dependencies are outputted/printed. Can also use different hook priorities if you want to print your scripts before or after the default scripts, etc.

In addition there are more specific hooks for specific admin screens:

  • Using the Scripts API allows other plugins to dequeue an async script.

Yea, this is a good argument for supporting async scripts in Script Loader. Also (as I mentioned earlier) scripts with async can depend on "blocking" scripts without causing any problems.

Async scripts are already being used with before/after inline scripts.

Thanks for the examples. May be missing something but don't actually see any async scripts with "after" scripts there.

However imho a bigger concern is the proposed JS that handles "scripts after". Cloning a node and injecting it in the DOM as text/javascript is pretty much as bad as running that node's content with eval(). Here's what MDN has to say about it:

Warning: Executing JavaScript from a string is an enormous security risk. It is far too easy for a bad actor to run arbitrary code when you use eval(). See Never use eval()!, below.

Frankly I don't think that's acceptable. Are there any examples of big, popular sites like Google, Apple, FB, etc. that currently do something like this?

Based on all of this, I propose the following next steps:

  1. We agree to support async scripts in the script loader with dependencies (this is already implemented, but we can address specifics in the PR).
  2. We iron out remaining issues for how inline scripts are handled for defer and async scripts.

Sure, number 1 sounds okay, even if plugins and themes don't get onboard. Not so sure about number 2 though. In my opinion there should be no support for "after" scripts for scripts with async and defer. If plugins really want to run something after a script is evaluated, that can be handled from the JS much better.

Also there is one big unanswered question here: Does adding support for async and defer would make it harder or impossible to add support for module scripts in WordPress? I'd consider this questing a blocker. (Think there was a comment somewhere here talking about modules, can't find it now though.)

Last edited 19 months ago by azaozz (previous) (diff)

#113 follow-ups: @joemcgill
19 months ago

  • Keywords 2nd-opinion removed

Thanks for the detailed response, @azaozz. Sounds like we’re narrowing down on the last remaining issues to resolve.

However imho a bigger concern is the proposed JS that handles "scripts after". Cloning a node and injecting it in the DOM as text/javascript is pretty much as bad as running that node's content with eval().

I’m in total agreement with your concerns here. When testing this approach with Content-Security-Policy, @westonruter discovered that it had a security vulnerability related to the strict-dynamic source expression for which he made a test case. We’ve recently replaced the original approach with one that uses event listeners, suggested in this PR. The revised approach is now fully compliant with Strict CSP. See the updated test case.

Regarding examples of other platforms evaluating a script of type text/template, this is essentially what was done in AMP for the amp-script component. You can see an example of how an inline script (here of type text/plain) is evaluated in an AMP context.

In my opinion there should be no support for "after" scripts for scripts with async and defer. If plugins really want to run something after a script is evaluated, that can be handled from the JS much better.

This goes back to why inline scripts were introduced in the first place. There needs to be a way to pass dynamic data from PHP back to static JS scripts that can execute either before or after a registered script. Inline scripts are the way to do this in WP, and supporting them for async/defer would be very useful. With WordPress providing the framework to easily attach after inline scripts to async/defer scripts, plugin developers wouldn’t have to come up with their own solutions every time they want to pass data to such scripts. For example, without a deferred inline after script, passing data to a defer script would require setting a global variable. In contrast, with a deferred inline script you could cleanly do:

wp_register_script(
        'foo',
        plugin_dir_url( __FILE__ ) . 'foo.js',
        array(),
        '0.1',
        array( 'strategy' => 'defer' )
);
wp_add_inline_script(
        'foo',
        sprintf( 'Foo.init( %s )', wp_json_encode( getFooData() ) ),
        'after'
);

Additionally, we are attempting to create safe upgrade paths for scripts that are currently blocking to be converted to a defer or async loading strategy. We've designed the API changes to ensure that inline scripts added to a handle continue to be executed in the intended order when the loading strategy changes. This removes an important backward compatibility concern from plugin/theme developers. The only other reasonable alternative would be to always force any scripts that have an inline script added to it—and all its dependencies—to load with a blocking strategy regardless of how it was registered. This is less than ideal, which is why we’re advocating for the event listener strategy referenced above.

Does adding support for async and defer would make it harder or impossible to add support for module scripts in WordPress?

This is a good question, and one I’m continuing to review, but initially I don’t see there being a conflict here. According to the spec, any scripts added with type="module" are automatically loaded using a deferred strategy, but can also support async if present. What’s more is that the changes we’re proposing to the function signatures could make adding support for module easier in the future by adding an optional ‘type’ attribute to the array of options being used for loading strategy and position (not that we need to decide on this here). For example:

wp_register_script(
	'my-module',
	plugin_dir_url( __FILE__ ) . 'my-module.mjs',
	array(),
	'0.1',
	array(
		'type'      => 'module',
		'in_footer' => true,
	)
);

If you have specific concerns we need to be aware of, please let us know.

#114 in reply to: ↑ 112 ; follow-up: @westonruter
19 months ago

Replying to azaozz:

Thanks for the examples. May be missing something but don't actually see any async scripts with "after" scripts there.

You’re right, these examples were focused on cases where the script_loader_tag filter was used to inject the async attribute. There are other cases where async scripts occur in plugins which also involve after scripts, but they are manually printed. For example (links are to source code):

  • Jetpack: Prints an async script followed by an inline script defining the dataLayer configuration variable.
  • LightStart: Similarly, manually prints an async script for gtag. Includes an after script that defines the dataLayer global to configure gtag.
  • Google Listings & Ads: Also manually prints gtag script. The dataLayer is defined in an after inline script.
  • Orbit Fox by ThemeIsle: Yet again, manually prints googletagmanager script with after inline script continuing configuration
  • Ad Inserter – Ad Manager & AdSense Ads: Manually prints adsense async script tag, with after scripts inline scripts containing: (adsbygoogle = window.adsbygoogle || []).push({});
  • SEO Plugin by Squirrly SEO: Manually prints async scripts with after inline scripts for Google Analytics and Google Tag Manager.
  • GDPR Cookie Compliance: Manually prints gtag script with after inline script.

Note again that they are manually printed as opposed to using the script loader since the API doesn’t support async/defer (see prior plugin code comment). Some benefits:

  1. As we discussed above, by updating the script loader API to support async/defer, other plugins can easily dequeue these scripts without having to dig into the source code to figure out how the script was printed manually via an action (which sometimes is impossible or extremely difficult).
  2. Plugins can also easily attach additional inline after scripts.
  3. Additionally, by ensuring after inline scripts only evaluate once the async/defer script has loaded, some boilerplate async code can be eliminated (e.g. window.dataLayer = window.dataLayer || [])
  4. Lastly, there’s no longer a need to suppress WordPress.WP.EnqueuedResources.NonEnqueuedScript sniff.

I put together a proof of concept Jetpack PR which shows some of these advantages.

#115 in reply to: ↑ 113 ; follow-up: @azaozz
19 months ago

  • Keywords 2nd-opinion added

Replying to joemcgill:

However imho a bigger concern is the proposed JS that handles "scripts after". Cloning a node and injecting it in the DOM as text/javascript is pretty much as bad as running that node's content with eval().

I’m in total agreement with your concerns here. When testing this approach with Content-Security-Policy, @westonruter discovered that it had a security vulnerability related to...

Yea, this looks a bit better but is still "not-a-good-idea". The problem is with the content of a type="text/template" node being run with JS exec(). As mentioned many times in many places, this is generally a bad idea. So unless that script is outputted (printed) as type="text/javascript" (or the type attribute omitted) I don't think it would be a good addition to core.

However the above is not the only reason to remove that code. There are other reasons:

  1. This is not used in core. Adding untested and unconfirmed API methods to core is a big red flag. The "unconfirmed" part refers to usability, i.e. if that method is not used in core, would it at least be useful to at least 10% of the plugins that use Script Loader? As far as I see this is a "NO".

This goes back to why inline scripts were introduced in the first place. There needs to be a way to pass dynamic data from PHP back to static JS scripts that can execute either before or after a registered script.

  1. This is not exactly true :)

What you're saying is true for "before" scripts, but not for "after" scripts. There is actually very little need for "after" scripts, so this feature of Script Loader has always been on the border of being quite useless. I.e. nearly all cases where an "after" script is used can be handled by using a "before" script. There are only few rare exceptions (which frankly do not warrant the existence of the convenience method for adding of "after" scripts, but that's a different question of a "bad API decision" that was made some time ago) .

wp_register_script(
        'foo',
        plugin_dir_url( __FILE__ ) . 'foo.js',
        array(),
        '0.1',
        array( 'strategy' => 'defer' )
);
wp_add_inline_script(
        'foo',
        sprintf( 'Foo.init( %s )', wp_json_encode( getFooData() ) ),
        'after'
);

This example is "the incorrect way to output data for scripts in WordPress" :) The "correct" way (using a "before" script) can be seen in many places in Script Loader:

and many many more. (Note that $scripts->localize() adds a "before" script.)

Additionally, we are attempting to create safe upgrade paths for scripts that are currently blocking to be converted to a defer or async loading strategy. We've designed the API changes to ensure that inline scripts added to a handle continue to be executed in the intended order when the loading strategy changes. This removes an important backward compatibility concern...

Yea, this is a lofty goal but unfortunately there is no 100% backwards compatibility. Since the loading order of "async" scripts is undetermined, they are not backwards compatible when used as dependency.

I know this was discussed above and the decision was to "leave it to the plugins to decide what to do about it". Not having support for "script after" is inline with that decision.

So the TL;DR: for "script after" support for "async" and "defer" scripts:

  1. Doesn't use a good, safe methods in JS.
  2. Currently very rarely used (for "blocking" scripts). Most uses are not warranted.
  3. Not used in core (and seems it will never be used).

In these terms I'm starting to get curious why this strong push to add that to core? :)
If there is really such a big desire, a better option would be to make a canonical plugin that implements it, imho. That way all developers that really want to use such code can have access to a standard way of handling it (can even copy the plugin's code, etc.). It will be really interesting to see usage stats for such plugin before considering such additions to core.

Re-adding the "2nd opinion" label as I think there are many unanswered questions here.

Last edited 19 months ago by azaozz (previous) (diff)

#116 in reply to: ↑ 113 @azaozz
19 months ago

Replying to joemcgill:

Does adding support for async and defer would make it harder or impossible to add support for module scripts in WordPress?

This is a good question, and one I’m continuing to review, but initially I don’t see there being a conflict here. According to the spec, any scripts added with type="module" are automatically loaded using a deferred strategy, but can also support async if present.

Right. However I'm looking more at the way module scripts are loaded:

A recent addition to JavaScript modules functionality is dynamic module loading. This allows you to dynamically load modules only when they are needed, rather than having to load everything up front. This has some obvious performance advantages

See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#dynamic_module_loading. This seems somewhat incompatible with Script Loader (no idea what's going to be loaded and when), and currently I'm unsure if adding "async" and "defer" might make that incompatibility larger. Imho exploring this now would be a really good thing.

#117 in reply to: ↑ 114 @azaozz
19 months ago

Replying to westonruter:

There are other cases where async scripts occur in plugins which also involve after scripts, but they are manually printed. For example (links are to source code):

The consideration here, imho, is that:

  1. Adding "async" attribute to script tags is not a new feature. It has existed for many years, and many WP plugins (3000+) are already using it.
  2. Estimate for the "conversion ratio" if/when support for the "async" attribute is added to the Script Loader: My guess is that only a small number of plugins that currently add "async" will switch to using Script Loader. Generally only plugins that want to let third-party code (other plugins) to remove their scripts would have a reason to switch. All the rest will not have a reason. In these terms my estimate is that at most about 5% of the plugins will switch to Script Loader which would be unsatisfactory/poor conversion.
  3. It seems reasonable to expect the plugins to handle cases of the rarely used "script after" the same way plugins are expected to handle cases for the "undefined" load order for "async" scripts. I.e. plugins that absolutely must use a script that runs immediately after an "async" script finishes running, and cannot do that from JS, should hande that themselves, same as currently.

Note that there are better alternatives to run an inline script after a script was run:

  1. Can be implemented in the main script. This is by far the best option.
  2. The after-script can be run on the DOMContentLoaded event. This event fires after all scripts, blocking, defer, and async have been loaded and executed.

In these terms I'd consider the Script Loader support for "after" scripts for scripts with async and defer as not-a-good idea. The alternatives are better.

This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.


19 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


18 months ago

joemcgill commented on PR #4391:


18 months ago
#120

@westonruter I spent some time today investigating why the PHPUnit tests are a now failing after merging #67, and turns out that the workflow to run PHPUnit as part of the CI pipeline doesn't build all of the assets before running the tests (see: Trac #57844) which means that the unit tests can't get the contents of the delayed inline script loader file. I've started playing with some options that would allow the unit tests to load the src version of the file if the tests are being run in a src context, but I think we'd need to update the main logic as well.

joemcgill commented on PR #4391:


18 months ago
#121

I just merged #72, which is a workaround for the PHPUnit failures I mentioned yesterday, by ensuring the delayed inline script loader file exists when the test suite is run from the src directory without building assets.

The remaining unit tests failures are now related to an upstream change to the WP_HTML_Tag_Processor that broke the normalize_markup() method introduced in 644769e. Those upstream changes were merged into this branch via 76602e5, which is why they just showed up today.

This ticket was mentioned in Slack in #core by joemcgill. View the logs.


18 months ago

This ticket was mentioned in Slack in #core-js by joemcgill. View the logs.


18 months ago

#124 in reply to: ↑ 115 ; follow-up: @joemcgill
18 months ago

Replying to azaozz:

Since we’re nearing the end of the merge window for 6.3, I want to revisit some of the things raised in this conversation and get final clarification on a path forward. Let me summarize my current understanding of the main concerns you have raised:

  1. Your opinion is that supporting inline after scripts is unnecessary, in general
  2. The implementation method in the current PR for delaying inline scripts is unsafe
  3. Supporting async and defer could conflict with future dynamic module imports

First, I’ll restate the reasons that we’ve chosen to keep support for inline scripts when added to a script that has an async or defer attribute. There are over 5000 cases in plugins and 808 cases in themes on the wp.org repos where wp_add_inline_script() is used, with the vast majority (>90%) of them using the after position. Even in core, there are many places where data from the server is being supplied to an inline script in an after position. An example is in site-editor.php, where settings from the server are dynamically supplied to the front end to be used after a core script is loaded. Without a way to delay those inline scripts until after the handle to which they are attached has loaded, those handles will always need to be loaded as a blocking script. After inline scripts are very useful because they do not require polluting the global namespace with a variable defined in the before inline script, and the script itself doesn’t have to be updated to read from that global variable: with an inline script, existing libraries can be used as-is. Our implementation allows script handles with inline scripts attached to be deferred or loaded asynchronously without the need to rewrite the inline logic, allowing more scripts to adopt async or defer strategies, including scripts in core that are currently blocking but could be delayed in the future (outside the scope of this PR, but something that I'd like to see us explore).

Regarding your concerns about the safety of the implementation, there are very important distinctions between scripts that are executed using eval() and scripts that are dynamically injected into the DOM during script execution (i.e., the approach we’re using). Most important is that sites implementing a content security policy (CSP) will not execute eval unless they include the unsafe-eval keyword. Dynamic scripts are handled completely separately, including external scripts and inline scripts. By using the strict-dynamic source expression, only trusted scripts are allowed to dynamically add scripts to the DOM. This trust is achieved by outputting a nonce-{random} attribute on the script tag. With strict CSP, browsers block executing scripts at parse time if they lack a valid nonce, and our delayed inline script loader does the exact same thing by making sure that inline scripts are only dynamically added if the original script[type=text/plain] has a valid nonce matching the CSP. In summary, dynamically adding scripts to a DOM is quite different and safer than running eval() on a text string. If you have more specific concerns about the security of this approach, please specify so they can be addressed.

Finally, I’ve looked into the dynamic module loading you raised in your previous comment and am not seeing any incompatibility with adding support for adding async and defer attributes to scripts. From what I can tell, any scripts that are registered as modules in the future would be able to be written in a way that includes dynamic imports without ever needing to interact with the Script Loader, since all of the logic to dynamically import external modules would be handled within the script itself.

Having said all that, what needs to be determined is whether the support for inline scripts needs to be completely removed from the implementation for an initial commit, or if there are further modifications that need to be made in order to leave that support in place. Additionally, if you have any other final changes that you would like to see addressed before commit, please list them so we can ensure they are addressed.

#125 in reply to: ↑ 124 ; follow-up: @azaozz
18 months ago

Replying to joemcgill:

  1. Your opinion is that supporting inline after scripts is unnecessary, in general

Yes. Seems it may be unnecessary for the following reasons:

Currently the inline "after scripts" may run (be placed in the HTML source) long after the "main" script. Example: when using script concatenation (with load-scripts.php which is always used in production) they are outputted after the concatenated script that contains all "main" scripts.

This behaviour cannot be achieved with the current patch/PR. Seems that to match this behaviour the "after" scripts for delayed main scripts would have to be executed on the DOMContentLoaded event, or another appropriate event, that ensures that all delayed scripts have finished executing. This could be done from PHP by wrapping each "after" script in a lambda JS function that would execute at the appropriate time.

Looking further, using this method would guarantee that the "after" scripts will be executed after the delayed main scripts in all cases, regardless of when and where the inline scripts appear in the HTML. However as the Script Loader API is a JavaScript API (not PHP), adding that lambda function may be considered "too much hand-holding", as this is a very well known, very basic pattern in JS.

So in practice there will be no difference (in PHP) between the "before" and "after" scripts as long as the JavaScript in the "after" scripts is written accordingly. Even more, these inline scripts can be outputted directly by the plugins on different PHP "actions" or with different hook priority. There is actually no need for plugins to add any "before" or "after" inline scripts in Script Loader at all as long as the JS is written properly. (I wouldn't call this "best practice", but it is possible.)

  1. The implementation method in the current PR for delaying inline scripts is unsafe

Wouldn't call it "unsafe", just "unneeded" and perhaps a bit "weird" :)

The question here is: do the browsers handle <script> tags exactly in the same way and with the same security restrictions like <script type="something-else"> tags? Think that wasn't the case some time ago, not sure now. You're right, the use of nonces seems to remove many security concerns.

However the bigger problem here is that this method does not match (doesn't have parity with) the way "after" scripts currently work (mostly) in production, see above.

  1. Supporting async and defer could conflict with future dynamic module imports

Yep, I really hope they won't, just don't have the time to fully investigate that now.

I’ve looked into the dynamic module loading you raised in your previous comment and am not seeing any incompatibility with adding support for adding async and defer attributes to scripts. From what I can tell, any scripts that are registered as modules in the future would be able to be written in a way that includes dynamic imports without ever needing to interact with the Script Loader, since all of the logic to dynamically import external modules would be handled within the script itself.

Thanks for looking at this! Yes, seems that dynamic module loading will be a browser controlled "pure JS" thing, completely "outside" of Script Loader.

...where data from the server is being supplied to an inline script in an after position

Yea, this is generally "doing_it_wrong" as long as Script Loader is concerned. See examples from all previous inline scripts that supply data (like the old translated strings). While it is possible to do I'd consider it "bad practice" for any JS that is handled by Script Loader. Can make the core scripts "non-portable" for no good reason.

scripts in core that are currently blocking but could be delayed in the future

Don't think that would ever be possible because of the edge cases. It would open a huuuge can of worms :)

I'm actually thinking the current PR may need to include a way to prevent (and add "doing_it_wrong") cases where a plugin or theme attempts to re-register any core script with async or defer. Unfortunately seems this functionality could be used only for newly added core scripts as there's no way to ensure 100% back-compat for the loading and execution order.

Having said all that, what needs to be determined is whether the support for inline scripts needs to be completely removed from the implementation for an initial commit, or if there are further modifications that need to be made in order to leave that support in place.

  • Support for "before" scripts seems to be working fine.
  • As mentioned above, support for "after" scripts can either be revised to achieve parity with current concatenated scripts, or completely removed and left for themes/plugins to handle it in JS. If removed there should be enough documentation explaining why, and how to add inline scripts that would execute after the main script has finished executing. Seems the most appropriate way would be to run any "after" scripts on the DOMContentLoaded event.

Additionally, if you have any other final changes that you would like to see addressed before commit, please list them so we can ensure they are addressed.

Do you think a method to detect when a plugin or a theme "tweaks" a core script to add defer or async is worth adding? Can probably see if that becomes common and then add it?

#126 in reply to: ↑ 125 ; follow-up: @westonruter
18 months ago

Thanks for the quick reply, @azaozz.

Replying to azaozz:

Currently the inline "after scripts" may run (be placed in the HTML source) long after the "main" script. Example: when using script concatenation (with load-scripts.php which is always used in production) they are outputted after the concatenated script that contains all "main" scripts. [...] However the bigger problem here is that this method does not match (doesn't have parity with) the way "after" scripts currently work (mostly) in production, see above.

I’m confused because previously you said that "support for concatenation would not be a priority and probably won't be necessary. This would simplify it a bit.” You’ve also proposed in #57548 that we eliminate script concatenation entirely since it is obsolete. Nevertheless, load-scripts.php isn’t used in production on the frontend; it’s only in the admin.

In any case, Script Loader always skips concatenation for any handle to which an inline script is attached. This can be confirmed in the existing unit tests.

Seems that to match this behaviour the "after" scripts for delayed main scripts would have to be executed on the DOMContentLoaded event, or another appropriate event, that ensures that all delayed scripts have finished executing. This could be done from PHP by wrapping each "after" script in a lambda JS function that would execute at the appropriate time. ¶ Looking further, using this method would guarantee that the "after" scripts will be executed after the delayed main scripts in all cases, regardless of when and where the inline scripts appear in the HTML. However as the Script Loader API is a JavaScript API (not PHP), adding that lambda function may be considered "too much hand-holding", as this is a very well known, very basic pattern in JS. ¶ As mentioned above, support for "after" scripts can either be revised to achieve parity with current concatenated scripts, or completely removed and left for themes/plugins to handle it in JS. If removed there should be enough documentation explaining why, and how to add inline scripts that would execute after the main script has finished executing. Seems the most appropriate way would be to run any "after" scripts on the DOMContentLoaded event.

We actually considered relying on the DOMContentLoaded event to run the after inline scripts, but the problem with this is that existing inline after scripts may be attempting to define something in the global scope without explicitly setting it on the window. For example:

wp_add_inline_script( 'foo', 'var fooHasLoaded = true;' );

Such an after inline script would break if it were automatically wrapped in an event handler callback. So instead our approach of dynamically inserting the script[type="text/plain"] after scripts is fully backwards compatible since anything defined in the function will go into the global scope.

Even more, these inline scripts can be outputted directly by the plugins on different PHP "actions" or with different hook priority. There is actually no need for plugins to add any "before" or "after" inline scripts in Script Loader at all as long as the JS is written properly. (I wouldn't call this "best practice", but it is possible.)

Right, this would not be a best practice as it disconnects the inline scripts from the scripts they relate to, making dequeuing them difficult or impossible.

  1. The implementation method in the current PR for delaying inline scripts is unsafe

Wouldn't call it "unsafe", just "unneeded" and perhaps a bit "weird" :)

It doesn’t seem particularly weird when compared with how external scripts often get dynamically inserted into the page (i.e. lazy-loading JS). For example, the emoji-loader does this:

function addScript( src ) {
        var script = document.createElement( 'script' );

        script.src = src;
        script.defer = script.type = 'text/javascript';
        document.getElementsByTagName( 'head' )[0].appendChild( script );
}

This can also be seen in MediaElement.js and in Gutenberg. The only difference is that we’re doing this for inline scripts instead of external ones.

The question here is: do the browsers handle <script> tags exactly in the same way and with the same security restrictions like <script type="something-else"> tags? Think that wasn't the case some time ago, not sure now. You're right, the use of nonces seems to remove many security concerns.

Good to hear!

  1. Supporting async and defer could conflict with future dynamic module imports

Thanks for looking at this! Yes, seems that dynamic module loading will be a browser controlled "pure JS" thing, completely "outside" of Script Loader.

Agreed. Nevertheless, I think Script Loader is still useful to enqueue module scripts, even though the dependencies array may be empty. Even so, the dependencies could end up being useful still as they could perhaps be added to a script[type=importmap].

...where data from the server is being supplied to an inline script in an after position

Yea, this is generally "doing_it_wrong" as long as Script Loader is concerned. See examples from all previous inline scripts that supply data (like the old translated strings). While it is possible to do I'd consider it "bad practice" for any JS that is handled by Script Loader.

The problem with the examples in Script Loader that are using before inline scripts is that they require the use of a global variable. There are a couple dozen examples of after inline scripts being used in core which allow data to be passed directly to the script without involving a global variable. This is beneficial for a few reasons, including: (1) reduces memory usage, (2) prevents pollution of global scope, (3) allows existing libraries to be re-used without having to work in a shim to make them look for global variables for configuration. So to me it seems like quite a good practice. Some examples of passing data to a script via an after inline script include block-editor.php, site-editor.php, and general-template.php.

Can make the core scripts "non-portable" for no good reason.

How do you mean?

Don't think that would ever be possible because of the edge cases. It would open a huuuge can of worms :) ¶ I'm actually thinking the current PR may need to include a way to prevent (and add "doing_it_wrong") cases where a plugin or theme attempts to re-register any core script with async or defer. Unfortunately seems this functionality could be used only for newly added core scripts as there's no way to ensure 100% back-compat for the loading and execution order. ¶ Do you think a method to detect when a plugin or a theme "tweaks" a core script to add defer or async is worth adding? Can probably see if that becomes common and then add it?

Actually, being able to transition applicable core scripts to use a delayed strategy is specifically what we want to explore once this gets merged (e.g. comment-reply). We’ve specifically developed our implementation in a way that retains compatibility between core delayed scripts and theme/plugin blocking scripts. In particular, if a core script adopts a delayed strategy but a theme/plugin is enqueueing a blocking script that depends on the core-delayed script, then our implementation forces the core script to revert to being blocking. So we have implemented a back-compat method to maintain the loading and execution order, including for before/after inline scripts.

#127 in reply to: ↑ 126 @azaozz
18 months ago

Replying to westonruter:

I’m confused because previously you said that "support for concatenation would not be a priority and probably won't be necessary.

Yea, sorry, I didn't explain that well at all. The reasons I think support for concatenation is not needed in the first iteration (this patch) is because only default core scripts can be concatenated and only in wp-admin. As there are no default scripts that are (can be) loaded with async or defer, adding concatenation support now seems an overkill. If eventually there are enough (new) core scripts that are loaded with async or defer by default, it seems concatenation can be added as that dramatically speeds up page load times on HTTP/1.2.

Actually, being able to transition applicable core scripts to use a delayed strategy is specifically what we want to explore once this gets merged (e.g. comment-reply).

Yep, seems the only core script that may be able to be loaded with a delayed strategy is comment-reply (front-end) as it is self-contained and afaik not used as a dependency. However pretty much all of the rest of the core scripts are not self-contained and often used as dependencies by various plugins. In some cases some core scripts are also used in inline scripts, even scripts that are pasted in post_content. That makes it virtually impossible to "convert" them to a delayed loading without bringing regressions and edge cases.

We’ve specifically developed our implementation in a way that retains compatibility between core delayed scripts and theme/plugin blocking scripts. In particular, if a core script adopts a delayed strategy but a theme/plugin is enqueueing a blocking script that depends on the core-delayed script, then our implementation forces the core script to revert to being blocking. So we have implemented a back-compat method to maintain the loading and execution order, including for before/after inline scripts.

That would work as long as a script file is enqueued through script-loader, but will usually fail for stand-alone inline scripts. There are even scripts that are pasted by users with unfiltered_html capability in posts, etc. Seen this type of regressions before, even on big/modern sites...

Last edited 18 months ago by azaozz (previous) (diff)

#128 follow-ups: @azaozz
18 months ago

@joemcgill @westonruter Frankly I'm starting to wonder why are you so eager to add support for "script after". Is that some kind of a personal goal or is there another reason?

Generally speaking the "script after" functionality promotes writing of mediocre JS (scripts that require additional JS code snippets to be outputted from PHP in order to work). This was somewhat useful ~10 years ago as many WP devs had good knowledge of PHP but weren't particularly well versed in JS. However in 2023 I'd like to think that most WP devs have a deep understanding of JS, and can implement anything they may need or want with "pure" JS, with no need of PHP "crutches" :)

My opinion still is that re-adding support for "script after" for scripts with async and defer is a step in the wrong direction. Seems it can be seen as "locking" the API to use outdated 2010 methods for this new functionality instead of attempting to modernize it and try to bring it closer to the 2023 best practices.

#129 follow-up: @spacedmonkey
18 months ago

  • Keywords commit added; 2nd-opinion removed

@westonruter @joemcgill @10upsimon

Trunk - TT3 PR - TT3 Trunk - TT1 PR - TT1
Response Time (median) 100.72 99.87 61.53 61.77
wp-load-alloptions-query (median) 0.64 0.63 0.62 0.63
wp-before-template (median) 51.4 51.08 22.4 22.4
wp-before-template-db-queries (median) 5.4 5.35 3.36 3.39
wp-template (median) 42.96 42.67 32.34 32.42
wp-total (median) 94.66 93.89 54.93 55.04
wp-template-db-queries (median) 2.64 2.61 3.75 3.76

The PR looks good to me. I think this change is ready to commit.

#130 in reply to: ↑ 128 @spacedmonkey
18 months ago

Replying to azaozz:

Generally speaking the "script after" functionality promotes writing of mediocre JS (scripts that require additional JS code snippets to be outputted from PHP in order to work). This was somewhat useful ~10 years ago as many WP devs had good knowledge of PHP but weren't particularly well versed in JS. However in 2023 I'd like to think that most WP devs have a deep understanding of JS, and can implement anything they may need or want with "pure" JS, with no need of PHP "crutches" :)

There are many examples of where after is used in core script with great success. A couple of scripts that are important include.

wp-i18n
heartbeat
wp-api-fetch
wp-blocks
wp-date

After can be missed used. But in many cases it is correctly used by developer to pass data from PHP to javascript. Take for example moment, that uses it to setup translated dates.

<script id='moment-js-after'>
moment.updateLocale( 'en_US', {"months":["January","February","March","April","May","June","July","August","September","October","November","December"],"monthsShort":["Jan","Feb","Mar","Apr","May","Jun","Jul","Aug","Sep","Oct","Nov","Dec"],"weekdays":["Sunday","Monday","Tuesday","Wednesday","Thursday","Friday","Saturday"],"weekdaysShort":["Sun","Mon","Tue","Wed","Thu","Fri","Sat"],"week":{"dow":1},"longDateFormat":{"LT":"g:i a","LTS":null,"L":null,"LL":"F j, Y","LLL":"F j, Y g:i a","LLLL":null}} );
</script>

Supporting after makes sense to me, as it is widely and correctly used by developers.

#131 in reply to: ↑ 128 @joemcgill
18 months ago

Replying to azaozz:

@joemcgill @westonruter Frankly I'm starting to wonder why are you so eager to add support for "script after". Is that some kind of a personal goal or is there another reason?

There is no personal agenda here other than ensuring that support for async and defer is added in a way that allows developers to adopt these strategies in their projects without breaking the existing functionality of the Script Loader API that they are relying on.

I agree with you that there are many other ways to write JS that makes use of data from the server without using inline scripts, including fetching data from an API endpoint rather than printing the data to an inline script. However, as we've investigated and shown in this thread, there are still a lot of places where people are using inline scripts — including newer code that has been added in Gutenberg. We've been able to add support for async and defer while also handling backward compatibility concerns in a thoughtful way. At this point, I'm starting to wonder why you are against supporting a feature of the API that has existed for years and is still widely used?

We can continue to evolve the way people are using JS in WordPress and support more modern best practices, but we should not leave behind folks who are still using our traditional APIs in the process.

@azaozz commented on PR #4391:


18 months ago
#132

Marking as ready to commit

Yep, seems ready to commit except the code that re-adds support for "script after". I agree the discussion about adding that code can continue during beta, and if the decision is to continue to promote outdated, mediocre JS architecture it can be committed then.

However I'm against adding that in the initial commit. No matter how I look at it, it doesn't make sense to keep promoting this outdated method instead of asking the developers to transition to more modern JavaScript. This seems like a pretty bad "step backwards" from a code architecture point of view.

(As the code that adds support for "script after" is still in this PR, I'm not removing the "changes requested" label.)

#133 in reply to: ↑ 129 @azaozz
18 months ago

Replying to spacedmonkey:

@westonruter @joemcgill @10upsimon

Trunk - TT3 PR - TT3 Trunk - TT1 PR - TT1
Response Time (median) 100.72 99.87 61.53 61.77
wp-load-alloptions-query (median) 0.64 0.63 0.62 0.63
wp-before-template (median) 51.4 51.08 22.4 22.4
wp-before-template-db-queries (median) 5.4 5.35 3.36 3.39
wp-template (median) 42.96 42.67 32.34 32.42
wp-total (median) 94.66 93.89 54.93 55.04
wp-template-db-queries (median) 2.64 2.61 3.75 3.76

Could you please describe what is shown in this table and how it is relevant to loading scripts and the speed the users see something in their browsers when accessing a web site?

#134 @azaozz
18 months ago

We actually considered relying on the DOMContentLoaded event to run the after inline scripts, but the problem with this is that existing inline after scripts may be attempting to define something in the global scope without explicitly setting it on the window. For example:

wp_add_inline_script( 'foo', 'var fooHasLoaded = true;' );

Such an after inline script would break if it were automatically wrapped in an event handler callback.

@westonruter And how hard would it be to fix/improve that script when transition it to use async or defer? :)

Also, once this event handler is added, the inline script won't need to be outputted "after" the main script. Can be outputted at any time making it a lot more flexible, less dependent on PHP, and generally of a bit better quality.

Ideally the developers will wrap their inline code in an event callback or lambda function or something similar, not just "dump it" in global scope. So the transition to using async or defer could actually be used to improve their JS too.

in many cases it is correctly used by developer to pass data from PHP to javascript

@spacedmonkey The "after" snippets were never intended for passing data from PHP to JS. The "before" inline scripts are for that. All cases where data is passed from PHP to JS in an "after" script should be considered "doing-it-wrong".

There are several reasons for this architectural decision. Perhaps if you wish we can discuss them at length somewhere else.

At this point, I'm starting to wonder why you are against supporting a feature of the API that has existed for years and is still widely used?

@joemcgill I'm not against supporting this feature. I'm against supporting it in an outdated way that promotes mediocre JS code/architecture.

Last edited 18 months ago by azaozz (previous) (diff)

#135 @azaozz
18 months ago

  • Keywords 2nd-opinion added

Re-adding the second-opinion keyword as I think the discussion about adding the current "script after" implementation should continue. Imho the more people contribute their opinions and the more use cases are examined the better.

#136 @azaozz
18 months ago

Related: Thinking more about the "script after" discussion here, it seems support for these inline script snippets should be deprecated for "blocking" scripts too.

The legitimate use cases can be re-implemented in JS. Would be a nice "JS code modernization". It doesn't seem to make sense for the Script Loader API to support this from PHP in 2023.

This ticket was mentioned in Slack in #core by francina. View the logs.


18 months ago

joemcgill commented on PR #4391:


18 months ago
#138

@azaozz, I've removed the support for inline "after" scripts and would appreciate another look. What this means is that now any script handle that is registered with an async or defer loading strategy will be printed without either (i.e. will be loaded as a blocking script) if an inline "after" script is attached to it or any of the scripts depending on it.

#139 @spacedmonkey
18 months ago

Benchmarks done - TT theme, theme data, PHP 8.2.4, 1000 runs.

Trunk - 55990 PR - 365f232
Response Time (median) 123.1 123.21
wp-load-alloptions-query (median) 0.56 1.03
wp-before-template (median) 20.84 21.11
wp-before-template-db-queries (median) 3.15 3.14
wp-template (median) 92.63 92.43
wp-total (median) 113.56 113.71
wp-template-db-queries (median) 10.14 10.05

Blackfire profiles show around 1.5% decrease in server time performance.
https://blackfire.io/profiles/compare/f0725970-0a76-432e-802c-ba8f14682cf9/graph

#140 @shawfactor
18 months ago

What several commenters are missing is that wp_register_script is a WordPress standard whilst the defer and async attributes are web standards so it is important that WordPress follows the web standard.

Those that saying that there are easier ways to includes script if you don't need dependencies are missing a major point in that wp_register_script allows other plugins to understand the script exists. This can ensure scripts are not added twice but has other applications. For instance I also use it in a service worker to ensure that all registered scripts is cached.

This will become increasingly important as vanilla javascript has already rendered libraries like jquery redundant.

Last edited 18 months ago by shawfactor (previous) (diff)

#141 @joemcgill
18 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 56033:

Script Loader: Add support for HTML 5 "async" and "defer" attributes.

This allows developers to register scripts with an intended loading strategy by changing the $in_footer parameter of wp_register_script and wp_enqueue_script to an array that accepts both an in_footer and strategy argument. If present, the loading strategy attribute will be added to the script tag when that script is printed to the page as long as it is not a dependency of any blocking scripts, including any inline scripts attached to the script or any of its dependents.

Props 10upsimon, thekt12, westonruter, costdev, flixos90, spacedmonkey, adamsilverstein, azaozz, mukeshpanchal27, mor10, scep, wpnook, vanaf1979, Otto42.
Fixes #12009.

#143 @joemcgill
18 months ago

  • Keywords needs-dev-note added; has-patch has-unit-tests commit 2nd-opinion removed

@azaozz commented on PR #4391:


18 months ago
#144

I've removed the support for inline "after" scripts and would appreciate another look.

Sorry for the delay, was afk dealing with some family stuff. The commit LGTM, thanks!

#145 @TobiasBg
18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Were the changes to .jshintrc and phpcs.xml.dist in [56033]? These seem unrelated and should probably be reverted?

#146 @joemcgill
18 months ago

  • Owner changed from 10upsimon to joemcgill
  • Status changed from reopened to assigned

@TobiasBg the phpcs.xml.dist changes are intentional and related to the parse_markup_fragment method that was added to the Unit Test suite for in tests/phpunit/tests/dependencies/scripts.php. The .jshintrc change were required in an earlier iteration of this PR, but are no longer technically necessary. That said, I chose to include the .jshintrc changes as well, since they were discovered in the process of developing this PR. Are you seeing any side effects from these changes or have any other specific concerns?

#147 follow-up: @TobiasBg
18 months ago

Thanks for the clarification, @joemcgill!

No, I'm not seeing side effects, but was confused when I saw the changes, as they and their purpose are not mentioned in the commit message. As there have been accidental commits of changes in dotfiles in the past, I just wanted to be sure.

Now, if the .jshintrc changes are not strictly necessary for the purpose of this ticket, they should maybe go in their own documented commit (after reverting that part of [56033] first). The scenario that I'm thinking about here is a developer stumbling upon e.g. "console": false in the future but not finding explanations when blameing the changes. After all, globally allow-listing console might cover/cloak the accidental addition of debug code in a commit and therefore doesn't come fully risk-free.

This ticket was mentioned in PR #4708 on WordPress/wordpress-develop by joemcgill.


18 months ago
#148

  • Keywords has-patch added

This reverts changes to the .jshintrc file that were included in r56033.

Props TobiasBg.
See #12009.

Trac ticket: https://core.trac.wordpress.org/ticket/12009

#149 in reply to: ↑ 147 @joemcgill
18 months ago

  • Keywords commit added

Replying to TobiasBg:

Always good to double check these things, so thank you for raising the concern about these two files potentially being committed accidentally.

Now, if the .jshintrc changes are not strictly necessary for the purpose of this ticket, they should maybe go in their own documented commit (after reverting that part of [56033] first). The scenario that I'm thinking about here is a developer stumbling upon e.g. "console": false in the future but not finding explanations when blameing the changes. After all, globally allow-listing console might cover/cloak the accidental addition of debug code in a commit and therefore doesn't come fully risk-free.

This is a fair argument for reverting this part of the change and letting someone add them back if/when the situation arises again. I've add a new PR that reverts those changes and will commit once ensuring all tests pass.

joemcgill commented on PR #4708:


18 months ago
#150

E2E test and Performance test failures are unrelated to this change, as the same errors can be observed in previous runs of these workflows prior to this PR.

#151 @joemcgill
18 months ago

In 56049:

Revert unnecessary changes to .jshintrc files.

This reverts changes to the .jshintrc file that were included in r56033.

Props TobiasBg.
See #12009.

#153 @joemcgill
18 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

@austyfrosty commented on PR #4391:


18 months ago
#154

For those whom might be using PHP >= 8.0's named arguments, this will break in WordPress 6.3.0.
!
named argument example method

Removing the in_footer prop will result in a "Cannot use a positional argument after a named argument" error.

I would suggest implementing a stub in your code until you've had time to resolve potential issues.

Just calling it out for those who might come looking.

This ticket was mentioned in Slack in #hosting by javier. View the logs.


18 months ago

This ticket was mentioned in Slack in #hosting by amykamala. View the logs.


18 months ago

#157 @westonruter
18 months ago

Note that the landed feature does not include support for delaying inline scripts. This means that if a delayed script (either async or defer) has an inline after script, the delayed script will be forced to be blocking to preserve the execution order. For revisiting the implementation of delayed inline scripts to avoid this, see #58632.

#159 @westonruter
17 months ago

In 56323:

Script Loader: Delay loading comment-reply script with async loading strategy.

Props westonruter, flixos90, joemcgill, sergiomdgomes.
See #12009.
Fixes #58870.

This ticket was mentioned in Slack in #core-docs by 10upsimon. View the logs.


17 months ago

#161 @lavinya
16 months ago

I hope the guy who started the thread is still alive. It's been 14 years.

#162 in reply to: ↑ 89 ; follow-up: @vasartam
16 months ago

Replying to 10upsimon:

The approach we’ve taken will enhance the Scripts API to allow developers to declare intended loading strategies for scripts (and inline scripts) during registration by replacing the current $in_footer param with a new $args param of type array that supports declaring both in_footer and strategy keys in wp_register_script and wp_enqueue_script functions. This change is fully backward compatible with the previous function signatures.

I claim that these changes are not backward compatible.

I had my production site broken after automatic update to WordPress 6.3.0 due to the changes to the wp_register_script function API.

I use PHP 8.0 and I call this function using named parameters:

<?php
wp_register_script(
        'rk-scripts-vendors',
        THEME_URL . '/dist/vendors.min.js',
        deps: array(),
        ver: filemtime( THEME_DIR . '/dist/vendors.min.js' ),
        in_footer: true,
);

After automatic update to WordPress 6.3.0 the site started responding with the following error:

Unknown named parameter $in_footer in /var/www/rk.loc/wp-content/themes/rk/src/features/setup/assets.php:58
Stack trace:
#0 /var/www/rk.loc/wp-includes/class-wp-hook.php(310): rk_theme_load_assets()
#1 /var/www/rk.loc/wp-includes/class-wp-hook.php(334): WP_Hook->apply_filters()
#2 /var/www/rk.loc/wp-includes/plugin.php(517): WP_Hook->do_action()
#3 /var/www/rk.loc/wp-includes/script-loader.php(2225): do_action()
#4 /var/www/rk.loc/wp-includes/class-wp-hook.php(310): wp_enqueue_scripts()
#5 /var/www/rk.loc/wp-includes/class-wp-hook.php(334): WP_Hook->apply_filters()
#6 /var/www/rk.loc/wp-includes/plugin.php(517): WP_Hook->do_action()
#7 /var/www/rk.loc/wp-includes/general-template.php(3053): do_action()
#8 /var/www/rk.loc/wp-content/themes/rk/header.php(14): wp_head()
#9 /var/www/rk.loc/wp-includes/template.php(785): require_once('...')
#10 /var/www/rk.loc/wp-includes/template.php(720): load_template()
#11 /var/www/rk.loc/wp-includes/general-template.php(48): locate_template()
#12 /var/www/rk.loc/wp-content/themes/rk/front-page.php(2): get_header()
#13 /var/www/rk.loc/wp-includes/template-loader.php(106): include('...')
#14 /var/www/rk.loc/wp-blog-header.php(19): require_once('...')
#15 /var/www/rk.loc/index.php(17): require('...')
#16 {main} thrown in /var/www/rk.loc/wp-content/themes/rk/src/features/setup/assets.php on line 58

For changes to be backwards compatible in this case you should have added a new argument $args before the $in_footer argument like so:

<?php
wp_register_script( $handle, $src, $deps = array(), $ver = false, $args = array(), $in_footer = false )

That way the existing code that relies on PHP 8.0 named parameters would not be broken.

Similar API changes have been applied to wp_enqueue_script function, and it also needs attention. I am not aware of any other significant API changes like this one, but maybe you are.

Hope this encourages to reconsider the choices and release a fix for backward compatibility.

#163 in reply to: ↑ 162 @vasartam
16 months ago

Replying to vasartam:

I claim that these changes are not backward compatible.

Created a ticket #59214 for this issue. As @Clorith pointed out in the comments there, named parameters are explicitly stated to be not supported in the WordPress docs: https://make.wordpress.org/core/handbook/references/php-compatibility-and-wordpress-versions/

Turns out this is not considered an issue.

#164 @westonruter
14 months ago

In 56933:

Script Loader: Move delayed head script to footer when there is a blocking footer dependent.

This prevents a performance regression when a blocking script is enqueued in the footer which depends on a delayed script in the head (with async or defer). In order to preserve the execution order, a delayed dependency must fall back to blocking when there is a blocking dependent. But since it was originally delayed (and thus executes similarly to a footer script), it does not need to be in the head and can be moved to the footer. This prevents blocking the critical rendering path.

Props adamsilverstein, westonruter, flixos90.
Fixes #59599.
See #12009.

#165 @johnbillion
14 months ago

In 57046:

Docs: Correct some docblock formatting errors.

Fixes #59784

See #12009, #52710

This ticket was mentioned in Slack in #core-performance by joemcgill. View the logs.


13 months ago

Note: See TracTickets for help on using tickets.