- Issue created by @catch
- π¬π§United Kingdom catch
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.
- Status changed to Needs review
7 months ago 11:21am 3 April 2024 - π¬π§United Kingdom catch
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.
- πΊπΈUnited States luke.leber Pennsylvania
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:
- Quicker loads overall
- 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.
- π¬π§United Kingdom catch
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.
- π¬π§United Kingdom catch
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());
- π¬π§United Kingdom catch
Rebased now that π Record total css and js file size in performance tests Fixed is in.
- Status changed to Needs work
7 months ago 11:40am 9 April 2024 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.
- Status changed to Needs review
7 months ago 1:28pm 9 April 2024 - Status changed to Needs work
7 months ago 12:02pm 10 April 2024 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.
- Status changed to Needs review
7 months ago 3:15pm 10 April 2024 - Status changed to RTBC
7 months ago 12:32pm 22 April 2024 - πΊπΈUnited States thejimbirch Cape Cod, Massachusetts
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&delta=0&language=en&theme=umami&include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script> ### Articles <script src="/sites/default/files/js/js_AmsTgkZtlnQSPjPq3dYt_0F5jMg4cWrd8Dzn1C0fPEs.js?scope=footer&delta=0&language=en&theme=umami&include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script> ## Without MR/11.x Branch ### Home <script src="/sites/default/files/js/js_AmsTgkZtlnQSPjPq3dYt_0F5jMg4cWrd8Dzn1C0fPEs.js?scope=footer&delta=0&language=en&theme=umami&include=eJxLzi9K1U_Ozy3Iz0vNKynWK81NzM3U1c1ITUxJLQIAqggLKQ"></script> ### Articles <script src="/sites/default/files/js/js_kYim7ymiBmzlTrrCnusXensmAsaPewPRBnRIVZr8DE8.js?scope=footer&delta=0&language=en&theme=umami&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!
- Status changed to Downport
7 months ago 10:07am 23 April 2024 -
alexpott β
committed 22707d5c on 11.x
Issue #3437839 by catch, Luke.Leber, thejimbirch: Only send libraries...
-
alexpott β
committed 22707d5c on 11.x
- Merge request !7671Only send libraries with the same asset type to asset URLs. β (Open) created by catch
- Status changed to Needs review
7 months ago 1:54pm 23 April 2024 - π¬π§United Kingdom catch
Opened a 10.3 MR. We can backport this to 10.2.x too I think.
- Status changed to Needs work
7 months ago 1:17pm 24 April 2024 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.
- Status changed to Needs review
7 months ago 3:06pm 24 April 2024 - π¬π§United Kingdom catch
Rebased. Green apart from a known failure in 10.3.x
- Status changed to RTBC
7 months ago 10:49am 25 April 2024 - π¬π§United Kingdom catch
The backport only needed to update performance tests, so re-RTBCing.
-
alexpott β
committed 4dd78b90 on 10.3.x
Issue #3437839 by catch, thejimbirch, Luke.Leber: Only send libraries...
-
alexpott β
committed 4dd78b90 on 10.3.x
- Status changed to Fixed
7 months ago 10:53am 25 April 2024 Automatically closed - issue fixed for 2 weeks with no activity.