Incorporate script loading strategies in bundled themes

[westonruter]

As of #12009 the script loader now has the ability for scripts to be printed with async or defer. This is being used on the frontend now for block view scripts (#59115), the comment-reply script (#58870), and the wp-embed script (#58931). Nevertheless, these new script loading strategies are not currently being used in the bundled themes. There are opportunities to improve performance by doing so.

It turns out that Twenty Twenty actually has a custom implementation of script loading strategy injection which can now be deprecated in favor of what is in core.

In general, any script that involves anything to do with the header (initial viewport) should get moved to the head (if it was registered as being $in_footer) and applied the defer loading strategy. This will ensure that the script will start loading earlier so that as soon as the DOM has loaded the logic can run and minimize the amount of time that the initial viewport is in an uninitialized state. Note that defer is preferable to async for scripts in the head because if the script is cached an async script can actually block rendering whereas a defer script never will.

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

[westonruter]

In 56556:

Bundled Themes: Use defer loading strategy for theme scripts.

• Add defer loading strategy for all frontend end-user theme scripts (excluding Customizer preview).
• Move scripts to the head which relate to the initial page viewport to ensure they start loading earlier and execute sooner while still not blocking rendering.
• Update Twenty Twenty's script loader (TwentyTwenty_Script_Loader) to support core's built-in script loading strategies (#12009), while also retaining backwards-compatibility for child themes that may set async and defer script data.
• Update the main script loading strategy in Twenty Twenty from async to defer for better performance on repeat page views, since when an async script is cached it will block rendering.

Props westonruter, flixos90, sabernhardt.
Fixes #59316.

Committed in r56556.

[peterwilsoncc]

Generally WP tries to keep bundled themes compatible with the WordPress version in which they were introduced. As an example of this, 2011 checks the ​block pattern functions exist prior to calling them.

I think these changes and those in #59302 will need to be modified to account for the older methods.

In this change I can see that 2011 running on older versions of WP will have the showcase script loaded in the footer rather than the header. At the time of writing I've only checked the first file.

To account for the script loading strategies in newer versions of WP, I think adding the data via the $wp_scripts global will keep the themes compatible with older versions of WP. Again using 2011's showcase as an example:

<?php
// Enqueue showcase script for the slider.
wp_enqueue_script(
        'twentyeleven-showcase',
        get_template_directory_uri() . '/js/showcase.js',
        array( 'jquery' ),
        '20211130',
);
// Defer loading of the script in WP 6.3+.
$wp_scripts->add_data( 'twentyeleven-showcase', 'strategy', 'defer' );

I've reopened this to work on a PR.

Follow up to:

• https://core.trac.wordpress.org/ticket/59316
• https://core.trac.wordpress.org/ticket/59302

[flixos90]

While I disagree with the direction of the new PR, there are indeed two specific instances of backward compatibility breaks in [56556], which need to be addressed. Please see ​https://github.com/WordPress/wordpress-develop/pull/5196#pullrequestreview-1625013049 for details.

cc @peterwilsoncc @westonruter

[westonruter]

Replying to peterwilsoncc:

Generally WP tries to keep bundled themes compatible with the WordPress version in which they were introduced. As an example of this, 2011 checks the ​block pattern functions exist prior to calling them.

I think these changes and those in #59302 will need to be modified to account for the older function signature.

The function signature change from bool to array|bool was actually anticipating backwards-compatibility. For example, if someone does:

<?php
wp_enqueue_script(
    'foo',
    'https://example.org/foo.js',
    array(),
    false,
    array(
        'strategy' => 'defer', 
        'in_footer' => false,
    ) 
);

The array being passed in as $in_footer will result in WP<6.3 as the value being truthy and thus effectively being treated the same as true in both ​wp_enqueue_script() and ​wp_register_script(). The defer loading strategy is essentially the same behavior as $in_footer=true because execution happens at the end of the document loading in both cases. The enhancement for using defer here is that the script can start loading earlier, but if it loads later once encountered in the footer there isn't any functional difference.

The only back-compat case where this doesn't work as expected is if the script has to be executed in the head, such as if they passed array( 'in_footer' => false ) without any strategy. However, this wasn't the case for any of the core themes: they were all already being printed in the footer (or in the case of Twenty Twenty, using a loading strategy; also see ​exception in Twenty Eleven).

So I don't see this as being a case where there is a backwards-compatibility break. Do you? I know @flixos90 has some additional thoughts.

• In https://core.trac.wordpress.org/changeset/56556 however, there are indeed a few places that are not backward compatible because in pre-WP 6.3 their script location would now have changed from what it was before. Those are specifically:

• https://core.trac.wordpress.org/changeset/56556/trunk/src/wp-content/themes/twentyeleven/showcase.php

I just noticed this as well, but I explained how this is not a backwards-compatibility break in ​https://github.com/WordPress/wordpress-develop/pull/5170/files#r1324822884

For the showcase script, this actually was originally added to the head, so in WP<6.3 this will actually get printed in the footer since the $args array is truthy for $in_footer. Nevertheless, the function's contents are wrapped in jQuery.ready() so it makes no difference when it loads.

https://core.trac.wordpress.org/changeset/56556/trunk/src/wp-content/themes/twentytwenty/functions.php

In the same way, the index.js script for Twenty Twenty is designed to be loaded asynchronously, so it doesn't matter where the script loads, whether it is in the head or footer.

@westonruter I still consider those two changes backward compatibility breaks because the code does not work the same way as it did before, and I don't think there's a good reason to move these two scripts to the footer? They were in the head originally, and whether that decision was originally made with caution or not, I think the scripts affect content in the head, so we shouldn't just move them to the footer in older versions, specifically for Twenty Eleven where there's no shim for async/defer. Even for Twenty Twenty though, you mention async, but the loading strategy was actually changed in the new PR to defer. I know that one was intentional, but for older versions I think it still results in an unintentional change of head async to footer defer.

@westonruter I still consider those two changes backward compatibility breaks because the code does not work the same way as it did before, and I don't think there's a good reason to move these two scripts to the footer? They were in the head originally, and whether that decision was originally made with caution or not, I think the scripts affect content in the head, so we shouldn't just move them to the footer in older versions, specifically for Twenty Eleven where there's no shim for async/defer.

@felixarntz While the script location does move, there is no functional backwards-compatibility break because the logic is wrapped in jQuery.ready(). In fact, there is actually a good reason to move it to the footer because it prevents a render-blocking script form being needlessly in the head. In WP 6.3, the script is kept in the head because it can get defer. But in WP<6.3 the side effect of passing an array for $in_footer is that it gets moved to the footer, which is also good for performance. The lack of the $in_footer parameter in Twenty Eleven may have been an oversight or due to the $in_footer parameter being "relatively" new to WordPress core.

Even for Twenty Twenty though, you mention async, but the loading strategy was actually changed in the new PR to defer. I know that one was intentional, but for older versions I think it still results in an unintentional change of head async to footer defer.

Actually, I meant it was designed to load _asynchronously_ not necessarily async. In other words, it is designed to load before or after DOMContentLoaded. But yes, in this case the script doesn't have a benefit of being moved to the footer and that should be revised with the wp_script_add_data() workaround.

Thanks for clarifying @westonruter. Both of those explanations make sense to me. Though one follow up question:

The lack of the $in_footer parameter in Twenty Eleven may have been an oversight or due to the $in_footer parameter being "relatively" new to WordPress core.

This is speaking from a performance perspective, but what about the functionality that the script covers? Could it be considered essential enough to have the script in the head? Speaking hypothetically, if async and defer did not exist, there would probably still be some scripts that should be in the head if they cover critical content, even if it's worse for load time performance, that's where my question is coming from.

This is speaking from a performance perspective, but what about the functionality that the script covers? Could it be considered essential enough to have the script in the head? Speaking hypothetically, if async and defer did not exist, there would probably still be some scripts that should be in the head if they cover critical content, even if it's worse for load time performance, that's where my question is coming from.

@felixarntz You mean the showcase.js script? The ​logic is wrapped in jQuery.ready() (aka $()) so it is deferred to execute until DOMContentLoaded anyway. So there is no functional difference. This DOMContentLoaded event is necessary for any such scripts loading in the head because without it none of the body would be loaded; so here the showcase script's $('.feature-slider a') query would return no results. The only way to work around the use of a DOMContentLoaded event handler is to rely on event delegation or MutationEvents, neither of which Twenty Eleven is using. So yeah, functionally the showcase script can be loaded either at wp_head or wp_footer without any difference in functionality.

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

[westonruter]

In 56569:

Bundled Themes: Ensure Twenty Twenty's main script loads in head for WP<6.3.

Amends [56556].
Props westonruter, flixos90, peterwilsoncc.
Fixes #59316.

Committed in r56569

@westonruter @felixarntz Thanks for hunting this out after I forgot to request reviews once the tests passed. I'll follow it up later today.

• When I asked before on the ticket, in https://core.trac.wordpress.org/ticket/59302#comment:5 you mention "choosing to pass around the incorrect type doesn't seem like a great idea", yet this PR does exactly the same, just using the old signature instead of the new one, which is not better or worse.

There's a significant difference here: in 6.3+ there is a type check for the $args parameter that doesn't exist for the $in_footer argument in earlier versions of WP.

A part of the motivation with these changes is to encourage future additions to these themes to use the legacy signature when supporting new features in WP (most likely the block editor). [ in_footer => false ] will load scripts in the header in older versions of WP.

@peterwilsoncc

There's a significant difference here: in 6.3+ there is a type check for the $args parameter that doesn't exist for the $in_footer argument in earlier versions of WP.

That check is there because handling an array has more requirements. For example we can't access $args['something'] on a boolean, so the check is needed there. That is not the case for the reverse - pretty much anything in PHP can be evaluated to a boolean.

A part of the motivation with these changes is to encourage future additions to these themes to use the legacy signature when supporting new features in WP (most likely the block editor). [ in_footer => false ] will load scripts in the header in older versions of WP.

Do you mean footer? [ in_footer => false ] will load scripts in the footer in older versions, and that's why it should only be used if we want a script to be in the head in 6.3+ and in the footer in <6.3. Otherwise (i.e. if we want the script to be in the head across all versions), passing [ in_footer => false ] is a breaking change. For exactly that reason, there is no such usage here (except for the one in https://core.trac.wordpress.org/changeset/56556/trunk/src/wp-content/themes/twentyeleven/showcase.php, which per the above is intentional)., and there is no breaking change.

Again, I'm not sure which problem you're trying to solve here. It doesn't make sense to me to encourage the legacy signature of the function when it's just that, the legacy signature, particularly as using the new signature doesn't cause backward compatibility breaks.

@felixarntz The legacy signature is the only signature that is officially supported in the versions of WP that these themes need to support. For themes that only support WP 6.3 or later, then you are right, it's best not to encourage use of the legacy signature.

However, for themes that are required to support older versions of WordPress then using the updated signature can and has led to mistakes so these themes should be encouraged to use the old signature.

[ 'in_footer' => false ], [ 'strategy' => 'whatever' ] and various other combinations can cause unintentional behaviour in earlier versions of WP and we need to account for that.

What I am trying to support in this PR is simple: making sure developers don't unintentionally introduce errors to older themes while using older methods to set data on scripts to take advantage of the new features. It doesn't revert the changes of the previous commits, it makes sure they're used in a fashion that won't introduce mistakes.

@peterwilsoncc Yes, using the new signature incorrectly can lead to errors. But that applies to any code, we can't prevent developers from making mistakes.

The way the code currently is there is no backward compatibility breakage, and I think that's a reasonable goal. I don't think it's a good idea to encourage using the legacy signature when it's the legacy signature. We should encourage the new signature, otherwise what is that even good for?

If we want to provide a way to more intuitively avoid mistakes from using the new signature in older WordPress versions, we should introduce a wrapper function that takes care of the one quirk, as it is ​outlined in the dev note here.

I personally think that is overkill since the usage in the themes is not problematic as it stands, and the wrapper function would need to be duplicated across every default theme. That said, if you feel strongly about avoiding mistakes with the new signature on older WP versions, then that is an approach I would be on board with. It encourages the new signature while taking care of the one quirk that people would otherwise have to keep in mind.

@felixarntz I don't think it's overkill.

The reason we are having this discussion is that the signature was changed in such a way that it's possible a developers intent is reversed when using the legacy signature.

There were multiple comments on the initial ticket requesting the use of add_data, as is the case for conditional comments, but that's not how the change was implemented. The logic was to avoid the bugs that have already being introduced & since fixed.

But that applies to any code, we can't prevent developers from making mistakes.

This is true but we can make it easier for developers. to catch mistakes.

I really don't see why it is controversial for themes that are required to support older versions of WordPress to use function signatures those versions of WordPress support. I am trying to prevent the introduction of bugs while improving performance, I don't see the problem.

Thanks @peterwilsoncc. As mentioned, I'm not opposed to introducing a wrapper function in those themes so that this kind of mistake can be avoided.

I really don't see why it is controversial for themes that are required to support older versions of WordPress to use function signatures those versions of WordPress support.

What to me is controversial is the idea that the default themes would encourage using the old signature of the function. We can avoid that by using a wrapper function that allows using the new signature without the risk of making the mistake of having array( 'in_footer' => false ) being interpreted as true in older versions.

I really don't see why it is controversial for themes that are required to support older versions of WordPress to use function signatures those versions of WordPress support. I am trying to prevent the introduction of bugs while improving performance, I don't see the problem.

@peterwilsoncc Since these themes are maintained with WordPress Core, the risk seems to be that core committers would be unaware of the new argument to wp_enqueue_script()/wp_register_script(), right? In any case, old core themes already are lacking maintenance and even more so it seems highly unlikely that any new scripts would be introduced in old core themes which would cause the problem that you've identified. So it seems to me that the risk of there being a problem is very low.

What to me is controversial is the idea that the default themes would encourage using the old signature of the function.

I think this misrepresents what this pull request does. It encourages the use of the supported signature in the versions of WP a theme is required to support. It does this because the type casting can result in different results when using the old and new signature.

For the new features this pull request encourages their use in a forward compatible manner in themes required to support older versions of WP.

Honestly, I don't think I care enough about this to continue the discussion. I've tried to pre-empt the bugs I think it will introduce in themes required to support older versions of WP (one if which has already needed to be fixed) but given the discussion they're not my bugs to fix.