Only send libraries with aggregate URLs that have the aggregate type included

Pushed a commit, this is only a small change to AssetResolver, most of it is indentation changes because we can remove a condition in a later hunk.

Will see whether this requires any test changes.

Also realised we can do this in the opposite direction for CSS aggregates - remove the non-CSS libraries when building those URLs too,

Apart from the potential improvements to browser and edge caches, this also makes the URLs significantly shorter.

Before:
https://drupal-dev.ddev.site/sites/default/files/js/js_Xc0CqWop9HPDPJJr9o1ZfdcVQmp-Q9tMliiLOllT6-4.js?scope=footer&delta=0&language=en&theme=umami&include=eJx1j0sOwjAMRC9E2jM5jikW_lR2CiqnpyrqgkU2Xvg9jWZyz046V0i6bQrKMwpk7tP5QQ-azUNB-HMJi3gF-TF0Xd3Iek4nLKVxHgmsFCOjcxcawbt7pyhVHJ-3F9M75_NO6m2Tq0IQ8koFXYSws1uO8oKgFT3YSECI9r_cvA3tCmbjZTXAGtsy4o-jDMUXI82NiA

After:

https://drupal-dev.ddev.site/sites/default/files/js/js_Xc0CqWop9HPDPJJr9o1ZfdcVQmp-Q9tMliiLOllT6-4.js?scope=footer&delta=0&language=en&theme=umami&include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ

It should also reduce the potential for differently ordered libraries to change the asset order.

We have implicit test coverage of this code in that the code path is tested, I think we can rely on 📌 Record total css and js file size in performance tests Fixed to cover the performance/caching optimization, the newly added test in that issue is how I found this in the first place.

Ran against a page on a site with ~230 CSS-only component libs and ~5 JS-only libs, and ~10 CSS+JS libs.

Single page

Without MR Applied:

    DOM: 65kB
    CSS: 25.4kB
    JS: 96.9kB

With MR Applied:

    DOM: 64.4kB
    CSS: 25.4kB transferred
    JS: 97.1kB transferred

Delta:

    DOM -.6kB
    CSS 0kB
    JS +.2kB

Net: -.4kB

No clue why / how an extra 200 bytes of JS was delivered. That shouldn't have been possible. In theory, less variance means:

1. Quicker loads overall
2. Fewer Drupal bootstraps (important for managed hosting customers who pay per view).

This improvement sounds good to me all around.

Note - I didn't review the code; just performed a basic sanity test against a highly customized 10.1.x site.

Fewer Drupal bootstraps (important for managed hosting customers who pay per view).

It won't actually mean fewer Drupal bootstraps, because this doesn't change the number of files on disk, I'm working on that in 📌 Use placeholdering for more elements to optimize asset serving Needs work , which is how I indirectly found this. What it should (hopefully) do is instead of a many-to-one relationship of URLs to files, make it 1-1. Because of that, it will reduce the number of requests that fall through from the browser cache, CDN, or varnish, to the webserver itself.

https://git.drupalcode.org/project/drupal/-/merge_requests/7313 now has test coverage for this, showing that it saves one very large file download when browsing two different Umami pages as an authenticated user.

Before:

  $this->assertSame(4, $performance_data->getStylesheetCount());
   $this->assertSame(94355, $performance_data->getStylesheetBytes());
   $this->assertSame(2, $performance_data->getScriptCount());
   $this->assertSame(264076, $performance_data->getScriptBytes());

After:

    $this->assertSame(4, $performance_data->getStylesheetCount());
    $this->assertSame(94355, $performance_data->getStylesheetBytes());
    $this->assertSame(1, $performance_data->getScriptCount());
    $this->assertSame(132038, $performance_data->getScriptBytes());

Rebased now that 📌 Record total css and js file size in performance tests Fixed is in.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased again.

Following the validation steps, I can confirm the query parameter on the JavaScript file remains the same after the merge request had been applied.

## With MR

### Home

<script src="/sites/default/files/js/js_AmsTgkZtlnQSPjPq3dYt_0F5jMg4cWrd8Dzn1C0fPEs.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=umami&amp;include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script>

### Articles

<script src="/sites/default/files/js/js_AmsTgkZtlnQSPjPq3dYt_0F5jMg4cWrd8Dzn1C0fPEs.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=umami&amp;include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script>


## Without MR/11.x Branch

### Home

<script src="/sites/default/files/js/js_AmsTgkZtlnQSPjPq3dYt_0F5jMg4cWrd8Dzn1C0fPEs.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=umami&amp;include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script>

### Articles

<script src="/sites/default/files/js/js_kYim7ymiBmzlTrrCnusXensmAsaPewPRBnRIVZr8DE8.js?scope=footer&amp;delta=0&amp;language=en&amp;theme=umami&amp;include=eJx1j0EOwzAIBD8Ux2_CmKaoYCJwWqWvb5Qqhxx84cCMFjb26KS5QNC0KShnFIjY53OD5pSbuYLw9xIWsQLyZ2i6WqPWYz5hSpXjSGAlHxmdu9AIPsw6eSpi-JreTJ_I55zV6ibXC07IKyU0EcLO1mKU5wQ16cFGAoLXe_NmdWgXh1a5LSP-PM6R_wAzGIIn"></script>

Marking as RTBC for the validation steps. If it needs a code review, please switch it back to Needs review. Thanks!

Committed 281e69a and pushed to 11.x. Thanks!

We need a 10.3.x MR...

Opened a 10.3 MR. We can backport this to 10.2.x too I think.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased. Green apart from a known failure in 10.3.x

The backport only needed to update performance tests, so re-RTBCing.

Committed 4dd78b9 and pushed to 10.3.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.