-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Introduce fetchpriority
for Scripts and Script Modules
#8815
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
fetchpriority=low
by default for script modules and add ability to overridefetchpriority
for Scripts and Script Modules
@@ -175,11 +175,18 @@ function register_block_script_module_id( $metadata, $field_name, $index = 0 ) { | |||
$block_version = isset( $metadata['version'] ) ? $metadata['version'] : false; | |||
$module_version = isset( $module_asset['version'] ) ? $module_asset['version'] : $block_version; | |||
|
|||
$args = array(); | |||
if ( isset( $metadata['supports']['interactivity'] ) && $metadata['supports']['interactivity'] ) { | |||
// TODO: Add ability for the fetchpriority to be specified in block.json for the viewScriptModule. In wp_default_script_modules() the fetchpriority defaults to low since server-side rendering is employed for core blocks, but there are no guarantees that this is the case for non-core blocks. That said, viewScriptModule entails Interactivity API, following the pattern from core it _should_ be SSR'ed and that is why this is the default for when the block supports interactivity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that property in block.json
should become an object instead of a string. In the future we might need other properties in there too.
cc @gziolo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely! See below 😄 #8815 (comment)
@@ -243,6 +250,7 @@ function register_block_script_handle( $metadata, $field_name, $index = 0 ) { | |||
$script_args = array(); | |||
if ( 'viewScript' === $field_name && $script_uri ) { | |||
$script_args['strategy'] = 'defer'; | |||
// TODO: There needs to be a way to specify that a script defined in a module is safe to use fetchpriority=low. Perhaps the viewScript should not only allow a handle string or a `file:./foo.js` string, but allow an an array of params like { "href": "./foo.js", "fetchpriority": "low" }. This would allow for more metadata to be supplied, like the script loading strategy. Related: <https://core.trac.wordpress.org/ticket/56408> and <https://core.trac.wordpress.org/ticket/54018>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes, exactly what I meant above 😅
Co-authored-by: Pascal Birchler <[email protected]>
Co-authored-by: Luis Herranz <[email protected]>
* @phpstan-type ScriptModule array{ | ||
* src: string, | ||
* version: string|false|null, | ||
* enqueue: bool, | ||
* dependencies: array<array{ id: string, import: 'dynamic'|'static' }>, | ||
* fetchpriority: 'auto'|'low'|'high', | ||
* } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added these @phpstan
annotations for myself as a safeguard while developing this patch. If others don't want PHPStan annotations in core, I'll remove prior to commit.
fetchpriority
argument to go alongside the loadingstrategy
(async
&defer
) when registering scripts viawp_register_script()
/wp_enqueue_script()
. This is a follow-up to the script loading strategies introduced in Core-12009.$args
parameter towp_register_script_module()
/wp_enqueue_script_module()
(and their corresponding methods) to mirror the same 5th$args
parameter used inwp_register_script()
/wp_enqeue_script()
. This$args
parameter samefetchpriority
argument introduced for non-script modules. (Note that module scripts have adefer
loading strategy by default, so adding support for astrategy
would only be useful forasync
. This is not in the scope of this PR.) This will apply thefetchpriority
attribute both to theSCRIPT[type="module"]
tags as well as theLINK[rel="modulepreload"]
tags for static import dependencies.fetchpriority
value for both scripts and script modules isauto
.fetchpriority
attribute is emitted onSCRIPT
andLINK
tags, it is omitted if the value isauto
since this is the default value.comment-reply
script is given an explicitfetchpriority
oflow
. While the script is already registered with theasync
loading strategy which Chrome causes the resource to be downloaded with alow
priority by default, this is not the case for Safari or Firefox which use a default medium/normal priority. So the explicitlow
priority reduces the chance that the loading of thecomment-reply
script will interfere with the loading of resources needed in the critical rendering path (e.g. an LCP image element).fetchpriority
oflow
. The very first requirement/goal defined for the Interactivity API was for server-side rendering, therefore blocks should not depend on the view module script for initial rendering.Note
If Gutenberg is active, you won't see any changes without WordPress/gutenberg#70173 also checked out.
Trac ticket: https://core.trac.wordpress.org/ticket/61734
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.