#61734 closed enhancement (fixed)
Add the ability to handle "fetchpriority" to ES Modules and Import Maps
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 6.9 | Priority: | normal |
| Severity: | minor | Version: | 6.5 |
| Component: | Script Loader | Keywords: | has-patch has-unit-tests has-dev-note |
| Focuses: | javascript, performance | Cc: |
Description
This is a follow-up to #56313.
Following this ticket, seems that adding fetchpriority to module scripts gives no luck.
So any imported script has high priority, even when they should have low fetching prioritize.
I guess that should be a plus, to allow that choise / filter also for modules and importmaps, while actually loader_tag , script_add_data, not even replace or HTML API seem having effect.
I guess that giving a low priority hint, sparingly and with proper controls, can bring an improvement in overall performance.
Reference documentation: https://developer.mozilla.org/en-US/docs/Web/API/HTMLScriptElement/fetchPriority
Change History (65)
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
16 months ago
#2
@
16 months ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 6.7
- Owner set to westonruter
- Status changed from new to accepted
- Version changed from 6.6.1 to 6.5
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
16 months ago
#4
follow-up:
↓ 5
@
16 months ago
I'm having a hard time reproducing the results I saw above. Now I'm seeing the addition of fetchpriority=low to be hurting LCP when the LCP element is a large Image block. This doesn't make sense to me.
#5
in reply to:
↑ 4
@
16 months ago
Replying to westonruter:
I'm having a hard time reproducing the results I saw above. Now I'm seeing the addition of
fetchpriority=lowto be hurting LCP when the LCP element is a large Image block. This doesn't make sense to me.
First LCP image should have always fetchpriority high, the perf hint combo:
- Heavy non essential resources: imgs, videos, links, scripts (example heavy animation scripts) low priority
- imgs, iframe and videos can/should be further optimized by lazy-loading, decoding async - something WP already someway does...
- Heavy essential resources (like lcp image), hypothetically also fonts, and primary static content, can get high priority hint, with additionally extra links prefetch/preconnect/preload - or newest speculation api for scripts.
- fonts than can have an additionally optimisation trick: using a font-display optional (with no swap timing, with no-blocking timing), and a link rel preload to that font, gives a really huge performance boost, fixing some loading issues; but this is more like a "pro-hack"; font-display fallback, usually is enough. 😅
Otherwise adding wrong priority, or adding it to everything can lead the opposite results, as you said for the lcp image...
#6
@
16 months ago
@dennysdionigi yes, I'm well familiar, which is why I'm confused why I'm not seeing that loading the modules with low priority is not improving LCP on a page that has an image with fetchpriority=high. I'm sure I saw an improvement when I tested before. We need to put together a test page that shows consistent performance improvements.
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
15 months ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
15 months ago
#9
@
15 months ago
- Keywords 2nd-opinion added
This ticket was discussed in today's performance bug scrub. Someone needs to verify Weston's findings https://core.trac.wordpress.org/ticket/61734#comment:2 before any implementation.
Props to @joemcgill.
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
15 months ago
This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.
14 months ago
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
14 months ago
#15
@
7 months ago
- Keywords 2nd-opinion removed
- Milestone changed from Future Release to 6.8.2
- Owner set to westonruter
- Status changed from assigned to accepted
I've been re-testing this and after some tooling improvements to benchmark-web-vitals, I'm now seeing the expected performance gains from reducing the priority of the module scripts. I'm working on publishing my findings and I'll work on a patch.
This ticket was mentioned in PR #8815 on WordPress/wordpress-develop by @westonruter.
6 months ago
#16
- Keywords has-patch has-unit-tests added; needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/61734
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
6 months ago
#18
@
6 months ago
I published an in-depth post (Improve LCP by Deprioritizing Script Modules from the Interactivity API) with benchmarking results, showing that adding fetchpriority=low to the script modules (on both the script[type=module] and link[rel=modulepreload] tags) can improve LCP by ~7%.
Pull request with core patch is ready for review. See also corresponding Gutenberg PR.
When combined with moving the script modules to the footer (#63486), the LCP improvement can be >9%.
#19
@
6 months ago
Note that fetchpriority attribute on a script tag is Baseline 2024 Newly available. It's supported by all modern browsers (Can I Use?) with >90% of all global users having a supported browser. If a browser doesn't support the attribute then it is just ignored, making it a great progressive enhancement.
This ticket was mentioned in Slack in #core-performance by adamsilverstein. View the logs.
6 months ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
5 months ago
@audrasjb commented on PR #8815:
5 months ago
#22
@westonruter if this is still on track for 6.8.2, note that the PR is conflicting against trunk ;)
@westonruter commented on PR #8815:
5 months ago
#23
if this is still on track for 6.8.2, note that the PR is conflicting against trunk ;)
@audrasjb Merge conflicts resolved in 4197eb2584fd51b839b20de965fc2595aceb3679
This ticket was mentioned in Slack in #core-committers by westonruter. View the logs.
5 months ago
#25
@
5 months ago
- Milestone changed from 6.8.2 to 6.9
Since 6.9 is now coming later this year and this is an engagement not a defect, I'm punting to the next major. This also needs more review, but we're coming down to the wire for 6.8.2.
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
5 months ago
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
3 months ago
@westonruter commented on PR #8815:
3 months ago
#28
@sirreal Thank you for the review! I've added checks for valid fetchpriority along with tests to ensure the expected behavior.
#31
@
3 months ago
Note: In order to see fetchpriority on script modules, make sure you either have Gutenberg deactivated or you have PR Gutenberg#70173 checked out (unless it has already been merged).
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
3 months ago
@westonruter commented on PR #8815:
3 months ago
#33
I just started trying using Gemini to code review patches. I hadn't done it for this PR, so I'm doing so retroactively:
git show c987f34948491544fcc1e5a49af193532bb4ec54 | gemini -m "gemini-2.5-pro" -p "Review these changes to WordPress core according to WordPress coding standards. Anything amiss?"
Output:
This is an excellent patch. The introduction of the
fetchpriorityattribute is well-implemented, the code is clean, and the accompanying tests are thorough and robust. The refactoring of the script modules test suite to use theWP_HTML_Tag_Processorand data providers is a significant improvement.
The changes adhere to the WordPress coding standards regarding documentation, naming conventions, and code style.
I found one very minor point that could be improved for consistency:
In
src/wp-includes/class-wp-script-modules.php, a check was updated to validate the type of a dependency ID:
//... if ( is_array( $dependency ) ) { if ( ! isset( $dependency['id'] ) || ! is_string( $dependency['id'] ) ) { _doing_it_wrong( __METHOD__, __( 'Missing required id key in entry among dependencies array.' ), '6.5.0' ); continue; } //...
The
_doing_it_wrong()message,Missing required id key..., was not updated to reflect the new! is_string()check. It now covers cases where the key is present but not a string (i.e., invalid), not just missing. A more accurate message might be:Missing or invalid required "id" key in dependency array entry.
This is a minor detail in an otherwise outstanding commit. The implementation is solid.
This ticket was mentioned in PR #9770 on WordPress/wordpress-develop by @westonruter.
3 months ago
#34
This is based on feedback from @sirreal in https://github.com/WordPress/gutenberg/pull/70173#pullrequestreview-3184253484:
Script modules are _registered_ with fetch priority. What if I enqueue a
highfetch priority script that depends on a module registered withlow? As the browser explores the dependency graph, will it determine that it should increase the priority of the dependency? Is that sort of priority dependence something that should be built into the core script modules system?
This PR ensures that if you have a dependent script module with fetchpriority=high, that any recursive dependencies will also get printed with fetchpriority=high regardless of the originally-registered fetchpriority, in which case the originally fetchpriority is emitted as a data-wp-fetchpriority attribute on the SCRIPT[type=module] or LINK[rel=modulepreload] for the sake of debugging. This is highly analogous to how non-module scripts check their dependents to see if any have async or defer, where if a blocking script depends on a defer script, the defer script will become blocking and a data-wp-strategy attribute is added to the SCRIPT tag.
The same is also implemented here for non-module scripts.
Also, this PR addresses this feedback in https://github.com/WordPress/gutenberg/pull/70173#discussion_r2321430500:
There's
@wordpress/a11ymodule that's not strictly an iAPI concept.
https://make.wordpress.org/core/2024/10/14/updates-to-script-modules-in-6-7/
I don't _think_ other modules have been introduced, but I could have missed something. Should
a11ybelowas well?
The script @wordpress/a11y is set to have a fetchpriority=low.
Prompt for Gemini CLI:
Summarize the changes in this branch compared to origin/trunk. List out any API changes. Do a code review review of the changes. Anything amiss?
Output:
### Summary of Changes
The core of this change is the introduction of a "fetchpriority bumping" mechanism for both traditional scripts and script modules. If a script with a higher
fetchprioritydepends on another script with a lowerfetchpriority, the dependency'sfetchpriorityis elevated to match the dependent's. This ensures that all scripts in a critical dependency chain are loaded with the appropriate priority.
The key changes are:
WP_Script_ModulesandWP_Scripts:
- Both classes now have a new private method,
get_highest_fetchpriority_with_dependents(), which recursively traverses the dependency tree to find the highestfetchpriority.WP_Script_Moduleshas a newget_dependents()method (and a corresponding$dependents_mapproperty for caching) to find scripts that depend on a given script.- The methods responsible for printing
<script>and<link rel="modulepreload">tags (print_enqueued_script_modules,print_script_module_preloadsinWP_Script_Modules, anddo_iteminWP_Scripts) have been updated to use the newget_highest_fetchpriority_with_dependents()method.- If a script's
fetchpriorityis "bumped", the original priority is stored in adata-wp-fetchpriorityattribute for debugging purposes.wp_default_script_modules():
- The
@wordpress/a11yscript module is now assigned alowfetchpriorityby default.- Tests:
- New PHPUnit tests have been added to verify the
fetchprioritybumping logic for both scripts and script modules.### API Changes
There are no public API changes. The new methods are private, and the existing functions for registering and enqueuing scripts remain unchanged. The only difference is in the rendered HTML.
### Code Review
The changes are well-implemented and adhere to WordPress coding standards.
- Logic: The logic for determining the highest
fetchpriorityis sound and correctly handles recursive dependencies.- Performance: The use of a
$dependents_mapto cache the dependency tree is a good performance optimization.- Clarity: The addition of the
data-wp-fetchpriorityattribute is a thoughtful touch that will make it easier to debug script loading issues.- Testing: The new tests are comprehensive and cover the new functionality well.
I don't see anything amiss. The changes are a valuable improvement to the script loading functionality in WordPress.
#35
@
3 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Based on feedback from @jonsurrell, I have a follow-up PR to make sure that if a dependent script module has a higher fetchpriority than a dependency script module, that the fetchpriority of the dependency is bumped to be as high as the dependent: https://github.com/WordPress/wordpress-develop/pull/9770
#36
@
3 months ago
This PR also now implements the same for non-module scripts: "escalated fetchpriority harmony".
This ticket was mentioned in Slack in #core-performance by westonruter. View the logs.
3 months ago
@jonsurrell commented on PR #9770:
2 months ago
#38
I've been reviewing and considering this. What this does is find the highest fetch priority in a dependency graph and apply that fetch priority to the entire graph. Am I interpreting the changes correctly?
Let's take an example with two enqueues (entry points), A and X:
---
config:
theme: base
---
flowchart LR
classDef HI stroke-width:1px, stroke:#ff0000, fill:#ff000022
classDef LO stroke-width:1px, stroke:#0000ff, fill:#0000ff22
classDef ENQUEUE stroke-width:4px
A["A::low"]
A:::LO
B["B"]
C["C"]
E["E"]
D["D::high"]
D:::HI
X["X::high"]
Y["Y"]
Z["Z"]
X:::HI
A --> B --> C --> E & D
A:::ENQUEUE
X --> D & Y --> Z
X:::ENQUEUE
With this PR, the F module's high priority would "infect" the entire graph and make everything high priority. That's not desirable, is it? We'd wind up with this:
---
config:
theme: base
---
flowchart LR
classDef HI stroke-width:1px, stroke:#ff0000, fill:#ff000022
classDef LO stroke-width:1px, stroke:#0000ff, fill:#0000ff22
classDef ENQUEUE stroke-width:4px
A:::HI
B:::HI
C:::HI
E:::HI
Y:::HI
Z:::HI
A["A::low"]
B["B"]
C["C"]
E["E"]
D["D::high"]
D:::HI
X["X::high"]
Y["Y"]
Z["Z"]
X:::HI
A --> B --> C --> E & D
A:::ENQUEUE
X --> D & Y --> Z
X:::ENQUEUE
It seems like the fetch priority should be directed from dependent to dependency. In fact, I wonder whether modules that aren't enqueued should have a fetchpriority at all or if fetchpriority should be associated with enqueue, and cascade to its dependents. In other words, dependency modules would inherit fetch priority from their dependents.
I think our ideal result graph would look like this:
---
config:
theme: base
---
flowchart LR
classDef HI stroke-width:1px, stroke:#ff0000, fill:#ff000022
classDef LO stroke-width:1px, stroke:#0000ff, fill:#0000ff22
classDef ENQUEUE stroke-width:4px
A:::LO
B:::LO
C:::LO
E:::LO
Y:::HI
Z:::HI
A["A::low"]
B["B"]
C["C"]
E["E"]
D["D::high"]
D:::HI
X["X::high"]
Y["Y"]
Z["Z"]
X:::HI
A --> B --> C --> E & D
A:::ENQUEUE
X --> D & Y --> Z
X:::ENQUEUE
@westonruter commented on PR #9770:
2 months ago
#39
@sirreal I think what you're describing is almost exactly what was implemented. It is indeed being directed from dependent to dependency. This follows the same approach as was implemented by the script loading strategies, where the dependent dictates what loading strategy a dependency is eligible to have. For example, a dependent with a blocking loading strategy cannot have a dependency with a defer loading strategy, or else they get loaded out of order. In this case, the dependency's defer gets downgraded to blocking.
I've added two test cases that show what happens with both classic scripts and script modules in 3dd0e0f8f0, using the dependency graph you came up with. The result is it comes up with the ideal result graph, with the exception that it is also taking into account the fetchpriority of auto:
The initial graph you depicted actually gets resolved as follows:
---
config:
theme: base
---
flowchart LR
classDef HI stroke-width:1px, stroke:#ff0000, fill:#ff000022
classDef LO stroke-width:1px, stroke:#0000ff, fill:#0000ff22
classDef ENQUEUE stroke-width:4px
A:::LO
Y:::HI
Z:::HI
A["A::low"]
B["B"]
C["C"]
E["E"]
D["D::high"]
D:::HI
X["X::high"]
Y["Y"]
Z["Z"]
X:::HI
A --> B --> C --> E & D
A:::ENQUEUE
X --> D & Y --> Z
X:::ENQUEUE
Classic scripts:
<script type="text/javascript" src="/z.js" id="z-js" fetchpriority="high" data-wp-fetchpriority="auto"></script> <script type="text/javascript" src="/d.js" id="d-js" fetchpriority="high"></script> <script type="text/javascript" src="/e.js" id="e-js"></script> <script type="text/javascript" src="/c.js" id="c-js"></script> <script type="text/javascript" src="/b.js" id="b-js"></script> <script type="text/javascript" src="/a.js" id="a-js" fetchpriority="low"></script> <script type="text/javascript" src="/y.js" id="y-js" fetchpriority="high" data-wp-fetchpriority="auto"></script> <script type="text/javascript" src="/x.js" id="x-js" fetchpriority="high"></script>
Script modules:
<link rel="modulepreload" href="/b.js" id="b-js-modulepreload"> <link rel="modulepreload" href="/c.js" id="c-js-modulepreload"> <link rel="modulepreload" href="/d.js" id="d-js-modulepreload" fetchpriority="high"> <link rel="modulepreload" href="/e.js" id="e-js-modulepreload"> <link rel="modulepreload" href="/z.js" id="z-js-modulepreload" fetchpriority="high" data-wp-fetchpriority="auto"> <link rel="modulepreload" href="/y.js" id="y-js-modulepreload" fetchpriority="high" data-wp-fetchpriority="auto"> <script type="module" src="/a.js" id="a-js-module" fetchpriority="low"></script> <script type="module" src="/x.js" id="x-js-module" fetchpriority="high"></script>
@jonsurrell commented on PR #9770:
2 months ago
#40
I think what you're describing is almost exactly what was implemented.
That's good, I misinterpreted the changes. Thanks for the additional test and clarification, they're helpful 👍
I'll do my best to give this thorough review early next week.
Do you have thoughts on this point?
I wonder whether modules that aren't enqueued should have a fetchpriority at all or if fetchpriority should be associated with enqueue, and cascade to dependents.
Would it make sense to always derive a module's fetch priority from whatever enqueues it, so that it inherits the highest fetch priority of an enqueued module?
We'd start with a graph like this
---
config:
theme: base
---
flowchart LR
classDef HI stroke-width:1px, stroke:#ff0000, fill:#ff000022
classDef LO stroke-width:1px, stroke:#0000ff, fill:#0000ff22
classDef ENQUEUE stroke-width:4px
X["X::low"]
X:::LO
X:::ENQUEUE
Y["Y::auto"]
Z["Z::high"]
Z:::HI
X:::ENQUEUE
Y:::ENQUEUE
Z:::ENQUEUE
X ---> A
Y ---> B
Z ---> C
A --> D & E
B --> E
C --> E & F
And the result would be this
---
config:
theme: base
---
flowchart LR
classDef HI stroke-width:1px, stroke:#ff0000, fill:#ff000022
classDef LO stroke-width:1px, stroke:#0000ff, fill:#0000ff22
classDef ENQUEUE stroke-width:4px
X["X::low"]
X:::LO
X:::ENQUEUE
Y["Y::auto"]
Z["Z::high"]
Z:::HI
X:::ENQUEUE
Y:::ENQUEUE
Z:::ENQUEUE
X ---> A
Y ---> B
Z ---> C
A --> D & E
B --> E
C --> E & F
A["A::low"]:::LO
C["C::high"]:::HI
D["D::low"]:::LO
E["E::high"]:::HI
F["F::high"]:::HI
This ticket was mentioned in Slack in #core by welcher. View the logs.
2 months ago
@westonruter commented on PR #9770:
2 months ago
#42
@sirreal That's a good question, and I'm not entirely sure which way is better, or if “enqueued-script fetchpriority determinance” should apply to both script modules and classic scripts. I suppose so? I took a stab at implementing what you suggested in c59f440, specifically for script modules. If that looks right I can implement the same for classic scripts.
Note this changes the priorities from https://github.com/WordPress/wordpress-develop/pull/9770#issuecomment-3282824522 which were originally:
---
config:
theme: base
---
flowchart LR
classDef HI stroke-width:1px, stroke:#ff0000, fill:#ff000022
classDef LO stroke-width:1px, stroke:#0000ff, fill:#0000ff22
classDef ENQUEUE stroke-width:4px
A:::LO
Y:::HI
Z:::HI
A["A::low"]
B["B"]
C["C"]
E["E"]
D["D::high"]
D:::HI
X["X::high"]
Y["Y"]
Z["Z"]
X:::HI
A --> B --> C --> E & D
A:::ENQUEUE
X --> D & Y --> Z
X:::ENQUEUE
But now they are:
---
config:
theme: base
---
flowchart LR
classDef HI stroke-width:1px, stroke:#ff0000, fill:#ff000022
classDef LO stroke-width:1px, stroke:#0000ff, fill:#0000ff22
classDef ENQUEUE stroke-width:4px
A:::LO
B:::LO
C:::LO
E:::LO
Y:::HI
Z:::HI
A["A::low"]
B["B"]
C["C"]
E["E"]
D["D::high"]
D:::HI
X["X::high"]
Y["Y"]
Z["Z"]
X:::HI
A --> B --> C --> E & D
A:::ENQUEUE
X --> D & Y --> Z
X:::ENQUEUE
@westonruter commented on PR #9770:
2 months ago
#43
Relatedly: If an enqueued script module has a low priority, would it make sense to just omit the modulepreload links rather than add them with fetchpriority=low as well? If something is truly low priority, then there shouldn't be a need to preload dependencies. This would reduce additional network contention for the LCP element resource.
@jonsurrell commented on PR #9770:
2 months ago
#44
If an enqueued script module has a
lowpriority, would it make sense to just omit themodulepreloadlinks rather than add them withfetchpriority=lowas well? If something is truly low priority, then there shouldn't be a need to preload dependencies. This would reduce additional network contention for the LCP element resource.
I'm not sure of the specifics on modulepreload in this case, but it's a good question. I understand that they're only browser hints, they give additional information to the browser and it decides exactly what to do with them and how to prioritize them. I'd hope that browsers are capable of prioritizing those preloads well so that there low priority module preloads are not harmful to things like LCP.
It's worth doing some testing, but let's leave that for another PR if we want to make any changes.
@jonsurrell commented on PR #9770:
2 months ago
#45
What this does is find the highest fetch priority in a dependency graph and apply that fetch priority to the entire graph. Am I interpreting the changes correctly?
Replying to myself: No, I was not interpreting the changes correctly. I had mixed up _dependents_ with _dependencies_ in the graph exploration so had backwards expectations. The implementation does the right thing.
This ticket was mentioned in Slack in #core by welcher. View the logs.
6 weeks ago
#47
follow-up:
↓ 48
@
6 weeks ago
@westonruter as owner do you think this will be ready for 6.9 Beta 1 in a week?
#48
in reply to:
↑ 47
@
6 weeks ago
Replying to welcher:
@westonruter as owner do you think this will be ready for 6.9 Beta 1 in a week?
Yes. I'm finalizing the PR.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
5 weeks ago
#52
follow-up:
↓ 53
@
5 weeks ago
I think there is a problem with the refactoring of WP_Script_Modules::get_dependencies() in [60999].
I updated a local/dev version of client site yesterday and something broke. This particular site has a custom block-based theme, that uses the navigation block (which enqueues @wordpress/interactivity).
In 6.8.3, WP_Script_Modules::get_dependencies() returns
array (
'@wordpress/interactivity' =>
array (
'src' => 'https://cppclaims.test/wp-includes/js/dist/script-modules/interactivity/debug.js',
'version' => 'beb31ebdbe898d3dd230',
'enqueue' => false,
'dependencies' =>
array (
),
),
)
but in 6.9-beta1 it returns:
array ( 0 => '@wordpress/interactivity', )
I'm pretty sure the above is not enough information to fully debug the problem, but I'm extremely busy w/ my day-job at the moment and don't have time right now to look into this further before posting a report...but wanted to get this report in so that those involved in the refactoring could look into it quickly.
If you have any questions, I'll do my best to respond.
#53
in reply to:
↑ 52
@
5 weeks ago
Replying to pbiron:
I think there is a problem with the refactoring of WP_Script_Modules::get_dependencies() in [60999].
@pbiron Thanks for that. I can indeed reproduce the issue with Query Monitor active when viewing a post that has lightbox enabled on an Image block. I get this error:
Uncaught TypeError: Cannot access offset of type string on string in /var/www/src/wp-content/plugins/query-monitor/classes/Collector_Assets.php:351
Stack trace:
#0 /var/www/src/wp-content/plugins/query-monitor/classes/Collector_Assets.php(89): QM_Collector_Assets::get_script_modules()
#1 /var/www/src/wp-content/plugins/query-monitor/classes/Collectors.php(84): QM_Collector_Assets->process()
#2 /var/www/src/wp-content/plugins/query-monitor/classes/Dispatcher.php(123): QM_Collectors->process()
#3 /var/www/src/wp-content/plugins/query-monitor/dispatchers/Html.php(357): QM_Dispatcher->get_outputters('html')
#4 /var/www/src/wp-content/plugins/query-monitor/dispatchers/Html.php(321): QM_Dispatcher_Html->before_output()
#5 /var/www/src/wp-includes/class-wp-hook.php(332): QM_Dispatcher_Html->dispatch('')
#6 /var/www/src/wp-includes/class-wp-hook.php(356): WP_Hook->apply_filters(NULL, Array)
#7 /var/www/src/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
#8 /var/www/src/wp-includes/load.php(1308): do_action('shutdown')
#9 [internal function]: shutdown_action_hook()
#10 {main}
thrown in <b>/var/www/src/wp-content/plugins/query-monitor/classes/Collector_Assets.php</b> on line <b>351</b><br />
I was partially aware of the issue with Query Monitor because @peterwilsoncc found that it uses the private WP_Script_Modules::get_marked_for_enqueue() method. This private method wasn't being used anymore so I had removed it, but this caused QM to fail, so I restored it in 04ec8ac as part of PR #9867, but clearly I didn't look far enough.
We should prioritize adding proper accessor methods to WP_Script_Modules to avoid the need to access the private internals. We did add one such method here in 6.9 and that is WP_Script_Modules::get_queue(). But there isn't a way to access the underlying registered data for a given script module without resorting to the Reflection API.
cc @johnbillion
#54
@
5 weeks ago
I'm happy to fix QM because it is knowingly using a private method so I don't expect its API to be stable. It's annoying but there is a reason we have method visibility. I've already fixed this in the develop branch of QM and I'll get a release out shortly.
We've got #60597 for a public API. Would be great to implement that so there's a stable API for extenders to use.
#55
@
5 weeks ago
There are four methods we potentially need to ensure remain compatible:
get_marked_for_enqueue()get_dependencies()get_src()
@johnbillion The first one now has a public API in the get_queue() method.
For get_dependencies(), it will be simply to restore the previous interface. I've got this drafted:
-
src/wp-includes/class-wp-script-modules.php
index 838c0eaf9c..cd9a81b99c 100644
a b class WP_Script_Modules { 398 398 ! $this->registered[ $id ]['in_footer'] 399 399 ) { 400 400 // If any dependency is set to be printed in footer, skip printing this module in head. 401 $dependencies = $this->get_dependencies( array( $id) );401 $dependencies = array_keys( $this->get_dependencies( array( $id ) ) ); 402 402 foreach ( $dependencies as $dependency_id ) { 403 403 if ( 404 404 in_array( $dependency_id, $this->queue, true ) && … … class WP_Script_Modules { 528 528 */ 529 529 private function get_import_map(): array { 530 530 $imports = array(); 531 foreach ( $this->get_dependencies( $this->queue) as $id ) {531 foreach ( array_keys( $this->get_dependencies( $this->queue ) ) as $id ) { 532 532 $src = $this->get_src( $id ); 533 533 if ( '' !== $src ) { 534 534 $imports[ $id ] = $src; … … class WP_Script_Modules { 566 566 * @param string[] $ids The identifiers of the script modules for which to gather dependencies. 567 567 * @param string[] $import_types Optional. Import types of dependencies to retrieve: 'static', 'dynamic', or both. 568 568 * Default is both. 569 * @return string[] List of IDs for script module dependencies.569 * @return array<string, array> Script modules keyed by ID. 570 570 */ 571 571 private function get_dependencies( array $ids, array $import_types = array( 'static', 'dynamic' ) ): array { 572 572 $all_dependencies = array(); … … class WP_Script_Modules { 584 584 in_array( $dependency['import'], $import_types, true ) && 585 585 isset( $this->registered[ $dependency['id'] ] ) 586 586 ) { 587 $all_dependencies[ $dependency['id'] ] = true;587 $all_dependencies[ $dependency['id'] ] = $this->registered[ $dependency['id'] ]; 588 588 589 589 // Add this dependency to the list to get dependencies for. 590 590 $id_queue[] = $dependency['id']; … … class WP_Script_Modules { 592 592 } 593 593 } 594 594 595 return array_keys( $all_dependencies );595 return $all_dependencies; 596 596 } 597 597 598 598 /**
This seems preferable for you, right?
#56
@
5 weeks ago
I'm not sure it makes much difference for QM. The main thing QM does is recursively builds the full list of all modules required on the page using the dependencies element in the array that was previously returned by get_dependencies(). It now fetches the array of module data from the WP_Script_Modules::$registered property (which is also not public).
If get_dependencies() was restored to its previous shape and made public that would be nice but all it would really do it prevent manual calls to the registered property.
Here are the non-public methods and properties that QM has to query to build a list of the enqueued modules and build the full list of dependent modules: https://github.com/johnbillion/query-monitor/blob/f7ec89b87d88bd66f68bca52d2e4f2c679b63c04/classes/Collector_Assets.php#L295-L305
This ticket was mentioned in PR #10403 on WordPress/wordpress-develop by @westonruter.
5 weeks ago
#57
See https://core.trac.wordpress.org/ticket/61734#comment:55
This reverts the WP_Script_Modules::get_dependencies() change in https://github.com/WordPress/wordpress-develop/commit/25420f05cdb013a5a6fb78c910009af7bc842fa5 which was developed in https://github.com/WordPress/wordpress-develop/pull/9867 to fix Core-63486.
As of WP 6.8, WP_Script_Modules::get_dependencies() had this return signature:
The aforementioned commit for 6.9 changed it to:
Since the method was private, I didn't think there would be a problem to make this change. However, it turns out that Query Monitor is using this private method via the Reflection API. This is because core is not yet exposing the necessary accessor methods from WP_Script_Modules, and so QM is forced to use a private method. This is something which should be no longer be necessary as of Core-60597.
In the meantime, we can restore the original return value for the method since it isn't particularly difficult to do so for 6.9-beta2.
Trac ticket: https://core.trac.wordpress.org/ticket/61734
#58
@
5 weeks ago
@pbiron Here's a PR for you to test if you are able: https://github.com/WordPress/wordpress-develop/pull/10403
@pbiron commented on PR #10403:
4 weeks ago
#59
An initial test of this PR applied to 6.9-beta1 seems to resolve the problem I reported in https://core.trac.wordpress.org/ticket/61734#comment:52. I'll test some more tomorrow.
@pbiron commented on PR #10403:
4 weeks ago
#60
After further review, this PR looks good. Doesn't result in a fatal for QM and still provides the the extra in_footer and fetchpriority items in the return value of get_dependencies().
@westonruter commented on PR #10403:
4 weeks ago
#62
Committed in r61073.
ciobanu0151 commented on PR #9770:
3 weeks ago
#63
There is an issue here: https://github.com/WordPress/wordpress-develop/blame/81b3c68c692b79b9bbaed0f8e16dfa51147b356b/src/wp-includes/class-wp-scripts.php#L1073. The checked param of the function needs a reference the way it works now you only save the checked one way down the recursive tree not in parallel. This can cause huge performance problems
I do indeed see that on a page with the Interactivity API present for the Image block and the Navigation block, that these scripts are all loaded with high priority in Chrome:
/wp-includes/blocks/navigation/view.js/wp-includes/blocks/image/view.js/wp-includes/js/dist/interactivity.jsThe HTML in question:
<script type="importmap" id="wp-importmap"> {"imports":{"@wordpress\/interactivity":"http:\/\/localhost:8888\/wp-includes\/js\/dist\/interactivity.js?ver=6.7-alpha-58835"}} </script> <script type="module" src="http://localhost:8888/wp-includes/blocks/navigation/view.js?ver=6.7-alpha-58835" id="@wordpress/block-library/navigation-js-module"></script> <script type="module" src="http://localhost:8888/wp-includes/blocks/image/view.js?ver=6.7-alpha-58835" id="@wordpress/block-library/image-js-module"></script> <link rel="modulepreload" href="http://localhost:8888/wp-includes/js/dist/interactivity.js?ver=6.7-alpha-58835" id="@wordpress/interactivity-js-modulepreload">This doesn't seem ideal, considering that server-side rendering should be employed by interactive blocks, meaning that the scripting should not be in the critical path for rendering the page. By loading these scripts with high priority, they may be hurting LCP by adding network contention for fetching the LCP image.
For adding
fetchpriorityfor a script module, either this could be added to thescripttag itself or it could be added as alink[rel=modulepreload]tag that the samesrc.For traditional scripts and module scripts alike, you can use the
wp_script_attributesfilter to inject thefetchpriorityattribute onto thescripttag. For example, to mark all module scripts to load withfetchpriority=low:However, this does not help when a script module is not directly enqueued in which case a
link[rel=modulepreload]is added viaWP_Script_Modules::print_script_module_preloads()which normally occurs for the@wordpress/interactivityscript. And there is no way to insertfetchpriority=lowinto the generatedlinktag without some hacking:I put these snippets in a plugin you can use for testing as well: https://gist.github.com/westonruter/471111a891f43e0f48bc7e0ca478623d
Given a test post using the TT4 theme that contains one lightbox-enabled Image block which is the LCP element, where the responsive Navigation block is also present:
I tried benchmarking page loads with the existing behavior where all scripts are loaded with high priority, and over 100 requests the median LCP-TTFB was 48.15 ms.
Then I tried adding
fetchpriority=lowto the scripts, and over 100 requests the median LCP-TTFB was 44.6 ms.So adding
fetchpriority=lowmakes the LCP metric here 7.37% faster.(The TTFB-LCP metric here is the LCP discounting variations in TTFB.)
I think the Interactivity API should add
fetchpriority=lowto all of the module scripts it registers.