Make WordPress Core

Opened 16 months ago

Closed 6 weeks ago

Last modified 6 days ago

#61734 closed enhancement (fixed)

Add the ability to handle "fetchpriority" to ES Modules and Import Maps

Reported by: dennysdionigi's profile dennysdionigi Owned by: westonruter's profile westonruter
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 @westonruter
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

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.js

The 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 fetchpriority for a script module, either this could be added to the script tag itself or it could be added as a link[rel=modulepreload] tag that the same src.

For traditional scripts and module scripts alike, you can use the wp_script_attributes filter to inject the fetchpriority attribute onto the script tag. For example, to mark all module scripts to load with fetchpriority=low:

<?php
add_filter(
        'wp_script_attributes',
        static function ( $attributes ) {
                if (
                        isset( $attributes['type'], $attributes['id'] ) &&
                        'module' === $attributes['type'] &&
                        str_starts_with( $attributes['id'], '@wordpress/block-library/' )
                ) {
                        $attributes['fetchpriority'] = 'low';
                }
                return $attributes;
        }
);

However, this does not help when a script module is not directly enqueued in which case a link[rel=modulepreload] is added via WP_Script_Modules::print_script_module_preloads() which normally occurs for the @wordpress/interactivity script. And there is no way to insert fetchpriority=low into the generated link tag without some hacking:

<?php
add_action(
        'init',
        static function () {
                $position = wp_is_block_theme() ? 'wp_head' : 'wp_footer';
                $priority = has_action( $position, array( wp_script_modules(), 'print_script_module_preloads' ) );
                if ( false === $priority ) {
                        return;
                }
                remove_action( $position, array( wp_script_modules(), 'print_script_module_preloads' ), $priority );
                add_action(
                        $position,
                        static function () {
                                ob_start();
                                wp_script_modules()->print_script_module_preloads();
                                $buffer = ob_get_clean();
                                if ( '' === $buffer ) {
                                        return;
                                }
                                $processor = new WP_HTML_Tag_Processor( $buffer );
                                while ( $processor->next_tag( array( 'tag_name' => 'LINK' ) ) ) {
                                        $processor->set_attribute( 'fetchpriority', 'low' );
                                }
                                echo $processor->get_updated_html();
                        },
                        $priority
                );
        }
);

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=low to the scripts, and over 100 requests the median LCP-TTFB was 44.6 ms.

So adding fetchpriority=low makes 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=low to all of the module scripts it registers.

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


16 months ago

#4 follow-up: @westonruter
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.

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

#5 in reply to: ↑ 4 @dennysdionigi
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=low to 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 @westonruter
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 @mukesh27
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

#11 @westonruter
14 months ago

  • Owner westonruter deleted
  • Status changed from accepted to assigned

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


14 months ago

#13 @mukesh27
14 months ago

  • Milestone changed from 6.7 to Future Release

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


14 months ago

#15 @westonruter
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

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


6 months ago

#18 @westonruter
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 @westonruter
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 @westonruter
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.

#29 @westonruter
3 months ago

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

In 60704:

Script Loader: Introduce fetchpriority for Scripts and Script Modules.

  • Allow scripts and script modules to be registered with a fetchpriority of auto (default), high, low:
    • When registering a script, add a fetchpriority arg to go alongside the strategy arg which was added for loading scripts with the defer and async loading strategies. See #12009.
    • For script modules, introduce an $args array parameter with a fetchpriority key to the wp_register_script_module(), and wp_enqueue_script_module() functions (and their respective underlying WP_Script_Modules::register() and WP_Script_Modules::enqueue() methods). This $args parameter corresponds with the same parameter used when registering non-module scripts.
    • Also for script modules, introduce WP_Script_Modules::set_fetchpriority() to override the fetchpriority for what was previously registered.
    • Emit a _doing_it_wrong() warning when an invalid fetchpriority value is used, and when fetchpriority is added to a script alias.
    • Include fetchpriority as an attribute on printed SCRIPT tags as well as on preload LINK tags for static script module dependencies.
  • Use a fetchpriority of low by default for:
    • Script modules used with the Interactivity API. For overriding this default in blocks, see Gutenberg#71366.
    • The comment-reply script.
  • Improve type checks and type hints.

Developed in GitHub PR, with companion for Gutenberg.

Props westonruter, jonsurrell, swissspidy, luisherranz, kraftbj, audrasjb, dennysdionigi.
Fixes #61734.

#30 @westonruter
3 months ago

  • Keywords needs-dev-note added

#31 @westonruter
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 fetchpriority attribute 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 the WP_HTML_Tag_Processor and 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 high fetch priority script that depends on a module registered with low? 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/a11y module 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 a11y be low as 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 fetchpriority depends on another script with a lower fetchpriority, the dependency's fetchpriority is 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_Modules and WP_Scripts:
    • Both classes now have a new private method, get_highest_fetchpriority_with_dependents(), which recursively traverses the dependency tree to find the highest fetchpriority.
    • WP_Script_Modules has a new get_dependents() method (and a corresponding $dependents_map property 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_preloads in WP_Script_Modules, and do_item in WP_Scripts) have been updated to use the new get_highest_fetchpriority_with_dependents() method.
    • If a script's fetchpriority is "bumped", the original priority is stored in a data-wp-fetchpriority attribute for debugging purposes.
  • wp_default_script_modules():
    • The @wordpress/a11y script module is now assigned a low fetchpriority by default.
  • Tests:
    • New PHPUnit tests have been added to verify the fetchpriority bumping 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 fetchpriority is sound and correctly handles recursive dependencies.
  • Performance: The use of a $dependents_map to cache the dependency tree is a good performance optimization.
  • Clarity: The addition of the data-wp-fetchpriority attribute 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 @westonruter
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 @westonruter
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 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.

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: @welcher
6 weeks ago

@westonruter as owner do you think this will be ready for 6.9 Beta 1 in a week?

Last edited 6 weeks ago by welcher (previous) (diff)

#48 in reply to: ↑ 47 @westonruter
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.

#49 @westonruter
6 weeks ago

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

In 60931:

Script Loader: Propagate fetchpriority from dependents to dependencies.

This introduces a "fetchpriority bumping" mechanism for both classic scripts (WP_Scripts) and script modules (WP_Script_Modules). When a script with a higher fetchpriority is enqueued, any of its dependencies will have their fetchpriority elevated to match that of the highest-priority dependent. This ensures that all assets in a critical dependency chain are loaded with the appropriate priority, preventing a high-priority script from being blocked by a low-priority dependency. This is similar to logic used in script loading strategies to ensure that a blocking dependent causes delayed (async/defer) dependencies to also become blocking. See #12009.

When a script's fetchpriority is escalated, its original, registered priority is added to the tag via a data-wp-fetchpriority attribute. This matches the addition of the data-wp-strategy parameter added when the resulting loading strategy does not match the original.

Developed in https://github.com/WordPress/wordpress-develop/pull/9770.

Follow-up to [60704].

Props westonruter, jonsurrell.
Fixes #61734.

#50 @westonruter
5 weeks ago

In 60999:

Script Loader: Add support for printing script modules at wp_footer.

This brings API parity with WP_Scripts by implementing opt-in support for printing in the footer via an in_footer argument. This argument can be supplied via the $args array passed to wp_enqueue_script_module() or wp_register_script_module(), alongside the existing fetchpriority key introduced in #61734. It can also be set for previously-registered script modules via WP_Script_Modules::set_in_footer(). This is not applicable to classic themes since modules are enqueued while blocks are rendered after wp_head has completed, so all script modules are printed in the footer anyway; the importmap script must be printed after all script modules have been enqueued.

Script modules used for interactive blocks (with the Interactivity API) are automatically printed in the footer. Such script modules should be deprioritized because they are not in the critical rendering path due to interactive blocks using server-side rendering. Script modules remain printed at wp_head by default, although this default should be revisited since they have deferred execution (and they are printed in the footer for classic themes already, as previously noted). Moving a script module to the footer ensures that its loading does not contend with the loading of critical resources, such as the LCP element's image resource, and LCP is improved as a result.

This also improves specificity of some PHP types, it ensures that script modules can't be registered with an empty ID, and it prevents printing script modules with empty src URLs.

Developed in https://github.com/WordPress/wordpress-develop/pull/9867

Follow-up to [60704].

Props b1ink0, westonruter, jonsurrell, peterwilsoncc, vipulpatil, mindctrl.
See #61734.
Fixes #63486.

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


5 weeks ago

#52 follow-up: @pbiron
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 @westonruter
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-&gt;process()
#2 /var/www/src/wp-content/plugins/query-monitor/classes/Dispatcher.php(123): QM_Collectors-&gt;process()
#3 /var/www/src/wp-content/plugins/query-monitor/dispatchers/Html.php(357): QM_Dispatcher-&gt;get_outputters('html')
#4 /var/www/src/wp-content/plugins/query-monitor/dispatchers/Html.php(321): QM_Dispatcher_Html-&gt;before_output()
#5 /var/www/src/wp-includes/class-wp-hook.php(332): QM_Dispatcher_Html-&gt;dispatch('')
#6 /var/www/src/wp-includes/class-wp-hook.php(356): WP_Hook-&gt;apply_filters(NULL, Array)
#7 /var/www/src/wp-includes/plugin.php(517): WP_Hook-&gt;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 @johnbillion
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 @westonruter
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 { 
    398398                                ! $this->registered[ $id ]['in_footer']
    399399                        ) {
    400400                                // 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 ) ) );
    402402                                foreach ( $dependencies as $dependency_id ) {
    403403                                        if (
    404404                                                in_array( $dependency_id, $this->queue, true ) &&
    class WP_Script_Modules { 
    528528         */
    529529        private function get_import_map(): array {
    530530                $imports = array();
    531                 foreach ( $this->get_dependencies( $this->queue ) as $id ) {
     531                foreach ( array_keys( $this->get_dependencies( $this->queue ) ) as $id ) {
    532532                        $src = $this->get_src( $id );
    533533                        if ( '' !== $src ) {
    534534                                $imports[ $id ] = $src;
    class WP_Script_Modules { 
    566566         * @param string[] $ids          The identifiers of the script modules for which to gather dependencies.
    567567         * @param string[] $import_types Optional. Import types of dependencies to retrieve: 'static', 'dynamic', or both.
    568568         *                                         Default is both.
    569          * @return string[] List of IDs for script module dependencies.
     569         * @return array<string, array> Script modules keyed by ID.
    570570         */
    571571        private function get_dependencies( array $ids, array $import_types = array( 'static', 'dynamic' ) ): array {
    572572                $all_dependencies = array();
    class WP_Script_Modules { 
    584584                                        in_array( $dependency['import'], $import_types, true ) &&
    585585                                        isset( $this->registered[ $dependency['id'] ] )
    586586                                ) {
    587                                         $all_dependencies[ $dependency['id'] ] = true;
     587                                        $all_dependencies[ $dependency['id'] ] = $this->registered[ $dependency['id'] ];
    588588
    589589                                        // Add this dependency to the list to get dependencies for.
    590590                                        $id_queue[] = $dependency['id'];
    class WP_Script_Modules { 
    592592                        }
    593593                }
    594594
    595                 return array_keys( $all_dependencies );
     595                return $all_dependencies;
    596596        }
    597597
    598598        /**

This seems preferable for you, right?

#56 @johnbillion
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:

https://github.com/WordPress/wordpress-develop/blob/58edb2f700ae2adb3c3c295e0e8be525009d3dbf/src/wp-includes/class-wp-script-modules.php#L306

The aforementioned commit for 6.9 changed it to:

https://github.com/WordPress/wordpress-develop/blob/7a840d3672ee11d6e3486e426b500cfaac7cb72c/src/wp-includes/class-wp-script-modules.php#L569

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 @westonruter
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().

#61 @westonruter
4 weeks ago

@pbiron Fixed in r61073. I mentioned #63486 in the commit instead of this ticket because the change was specifically introduced for that ticket via [60999].

@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

#64 @westonruter
2 weeks ago

In 61176:

Script Loader: Guard against exponential recursion during calculation of loading strategy and fetchpriority.

This addresses a performance issue in the recursive WP_Scripts::get_highest_fetchpriority_with_dependents() and WP_Scripts::filter_eligible_strategies() methods for redundant processing of shared dependencies in complex dependency graphs. To fix this, a $stored_results param is introduced which is passed by reference; this variable contains a cache of the calculated results for all scripts handles, so that subsequent calls for the same handle can return the cached value instead of re-computing it.

Developed in https://github.com/WordPress/wordpress-develop/pull/10459

Follow-up to [60704], [60931], [56033].

Props ciobanucatalin, b1ink0, westonruter, mukesh27.
See #61734, #12009.
Fixes #64194.

Note: See TracTickets for help on using tickets.