JavaScript aggregation should account for "async" and "defer" attributes

+++ b/core/includes/common.incundefined
@@ -4353,31 +4351,39 @@ function drupal_group_js($javascript) {
+          $index = md5(serialize($group_keys));

Why change this to md5? Lets keep this simple and just add the new item to the group_keys.

Fixing history related to defer attribute.

Marking this major now that #1140356: Add async, onload property to script tags is in.

@iamEAP drupal_group_js is an addition in D8 but there is currently a D7 backport that is RTBC which is holding up a couple of other drupal js related patch. The issue is #865536: drupal_add_js() is missing the 'browsers' option. Once that gets in all these changes become a lot easier to backport :)

I tried the patch in #8 and it seems to group the async scripts in its own group (I have not made a thorough review though).

But what the patch did not do was to actually put the async attribute on the script tag of the aggregated script.

Attached patch adds the attributes (also for the defer attribute) on the aggregated script tags, but is otherwise the same patch as the one in #8.

Applied cleanly

Checking patch core/includes/common.inc...
Hunk #1 succeeded at 4180 (offset -45 lines).
Hunk #2 succeeded at 4256 (offset -45 lines).
Hunk #3 succeeded at 4273 (offset -45 lines).
Applied patch core/includes/common.inc cleanly.

Applied cleanly

Checking patch core/includes/common.inc...
Hunk #1 succeeded at 4180 (offset -45 lines).
Hunk #2 succeeded at 4256 (offset -45 lines).
Hunk #3 succeeded at 4273 (offset -45 lines).
Applied patch core/includes/common.inc cleanly.

Do we have test that test the different aggregation with the new properties?

Updated issue summary.

I read the issue summary several times, and I don't see how this is a bug. If it is, can the summary clarify what is broken?

Okay, that sounds reasonable. Can you copy that into the issue summary?

As @ericduran points out, this should have automated tests.

Updating summary to describe the bug.

Fixing tags per http://drupal.org/node/1517250

Further clarification of the problem/motivation/bug.

From what I see it's fixed, anyone can confirm?

Most likely fixed by #1664602-35: Allow attributes to be passed to drupal_add_[css|js] (SRI)

Playing around with this in 8.6. The behavior I'm seeing is that if I set the defer attribute, the JS file will never be aggregated. It will retain the defer attribute.

Even if I have multiple JS files in one library with the same weight and defer attribute, the files will not be aggregated together.

At this time JS is pluggable, It is solved in this issue #352951 JS & CSS Preprocessing Pluggable so the method drupal_pre_render_scripts not exist and the patch is outside of context.

I found int this file /core/lib/Drupal/Core/Asset/JsCollectionGrouper.php:21

public function group(array $js_assets) {
    $groups = [];
    // ...
    $current_group_keys = NULL;
    $index = -1;
    foreach ($js_assets as $item) {
      // ...
      ksort($item['browsers']);

      switch ($item['type']) {
        case 'file':
          // ...
          // Set here: 'type', 'group', 'every_page', 'browsers', 'async', 'defer'
          $group_keys = $item['preprocess'] ? [$item['type'], $item['group'], $item['browsers']] : FALSE;
          break;

        case 'external':
          // Do not group external items.
          $group_keys = FALSE;
          break;
      }
     ...
    return $groups;
  }

I am not sure about where set 'async' and 'defer' attributes.

Playing around with this in 8.6. The behavior I'm seeing is that if I set the defer attribute, the JS file will never be aggregated. It will retain the defer attribute.

Even if I have multiple JS files in one library with the same weight and defer attribute, the files will not be aggregated together.

This seems to apply if you are not using a module like advagg enabled. The module has a feature Enable preprocess on all JS, which will do the following: Force all JavaScript to have the preprocess attribute be set to TRUE. All JavaScript files will be aggregated if enabled.

I've done a basically 1-for-1 re-roll of the changes onto the new files.

fix added for test case failure. Patch Applied in drupal 9.x branch.

Marking this issue as needs work since defer and async attributes are not present on the aggregated JS file.

Below is a fresh approach to resolve this issue.

First I did some research to understand how the aggregation process starts and i found that there is a method named Drupal\Core\Asset\AssetResolver::getJsAssets() which is responsible for triggering the aggregation, and there itself a condition has been added to make sure that Js libraries with assets will not be aggregated. This was added as part of https://www.drupal.org/project/drupal/issues/1664602#comment-6683702.

To resolve this issue:

• I modified Drupal\Core\Asset\AssetResolver::getJsAssets() so that it can allow aggregation for JS libraries having attributes
• I modified Drupal\Core\Asset\JsCollectionGrouper::group() function so that it can create aggregation groups for defer and async attributes as well.

This is an initial patch which is working good for normal page loads and will need more testing with Ajax behaviours. Also this patch tries to maintain the order of weight in which javascript files have been added as per the previous core implementation.

Please review.

Patch to include all JS attributes to be considered while aggregating, grouping for D8.

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -260,7 +260,7 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize) {
+            $options['preprocess'] = $options['cache'] ? $options['preprocess'] : FALSE;

This line is broken the tests

Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Exception Other      phpunit-22.xml       0 Drupal\KernelTests\Core\Asset\Attac
    PHPUnit Test failed to complete; Error: PHPUnit 8.5.8 by Sebastian Bergmann
    and contributors.
    
    Testing Drupal\KernelTests\Core\Asset\AttachedAssetsTest
    ......E.............                                              20 / 20
    (100%)
    
    Time: 26.31 seconds, Memory: 8.00 MB
    
    There was 1 error:
    
    1)
    Drupal\KernelTests\Core\Asset\AttachedAssetsTest::testAggregatedAttributes
    file_get_contents(core/modules/system/tests/modules/common_test/deferred-internal.js):
    failed to open stream: No such file or directory
    
    /app/core/lib/Drupal/Core/Asset/JsOptimizer.php:25
    /app/core/lib/Drupal/Core/Asset/JsCollectionOptimizer.php:126
    /app/core/lib/Drupal/Core/Asset/AssetResolver.php:298
    /app/core/tests/Drupal/KernelTests/Core/Asset/AttachedAssetsTest.php:157
    /app/vendor/phpunit/phpunit/src/Framework/TestResult.php:691
    
    ERRORS!
    Tests: 20, Assertions: 57, Errors: 1.
Fail      run-tests. Unknown              0 Unknown                            
    FATAL Drupal\KernelTests\Core\Asset\AttachedAssetsTest: test runner
    returned a non-zero error code (2).

Reverted the line causing test failure.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Adding patch against 10.1.x-dev

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This was previously tagged for tests and issue summary updates which still need to happen it seems.

Here's a quick reroll of #55 against 10.1.x-dev that seems to work for me. #61 did not work for me on 10.1.x, so it might be out of date or missing the `AssetResolver` part of this.

Whoops, looks like the patch doesn't quite work. Hiding for now until I can figure out why.

Setting to 10.1.x-dev to create the issue fork against that.

Re-ordered the attributes in the issue title to alphabetical because it was bothering me.

Just fyi though the issue would have to go into 11.x first before 10.x

@smustgrave My bad. Switching back.

No worries! If anyone here didn’t know but 11.x is the “main” branch. Where Drupal10 tags will be made off of. So no longer need to worry about 10.2.x, 10.3.x etc.

Here's a patch from the current draft merge request if anyone wants a snapshot. Feel free to build on this in the merge request - a test to verify that async and defer attributes are being aggregated (while other attributes aren't) seems like the next step.

@smustgrave I noticed that message on issues but didn't realize that using 11.x for 10.x.x versions was was the plan. Thanks for the heads up.

I think we should support defer for both CSS and Javascript, so re-titling.

#2695871: Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration added some unit test coverage which I think could be used as the basis for coverage here.

Also it would be good to actually use the API if we can - Umami has some footer-specific CSS files which look like good candidates. Could we apply it to all dialog CSS (like Claro's) or is it actually possible for dialogs to be part of initial paint? Any others?

No wait, deferring CSS isn't part of the spec, you can only use it on script tags :(. There are techniques for it (like https://web.dev/defer-non-critical-css/) but that's not helpful here. Back as it was then, but still the test coverage can probably be cribbed from.

@catch you were quicker.. just wanted to write something similar :)

Ha, yeah, I was about to comment that I didn't know you could defer CSS with defer attribute. 😂

Faced same problem after upgrade on Drupal 10.1.

This

async foo() {}

Become this:

foo(){}

but

foo: async () => {}

works as expected

foo:async()=>{}

P.s. oops, looks like you are here fixing attributes problem, mine with async inside code. For those who find this issue but the problem in 'async' inside code, I've created a separate issue #3374660: Update mck89/peast composer dependency to 1.15.2

It is also possible to use a fetchpriority attribute on CSS and JS. IMHO, assets with the same priority should be grouped together.

Looks like my changes in the issue fork have some grouping problems with Drupal 10.1+'s aggregation system in that it seems to result in aggregates sometimes being generated without a file included on one page and then again with all the same contents plus the additional file. This has been biting us in #3414538: Turbo: Implement additive JavaScript aggregation to prevent multiple evaluation where we're trying to get Hotwire Turbo working with JS aggregation on but it's downloading and evaluating (nearly) the same JavaScript, and after a bunch of debugging, I finally realized it was due to us using the defer attribute plus this issue fork, because removing the attribute and removing the patch instantly fixed the JS aggregates. Edit: actually, this doesn't seem to be the case, but that I'm dumb and misunderstood a fundamental part of how aggregates are built and from what libraries; see #3411010: Document that aggregation of JavaScript files can cause redeclaration.

Given that there are also other attributes that we may want to group by (e.g. fetchpriority as mentioned by #81) and that new attributes may come along later, I propose reworking this as a more generic approach: instead of hard-coding the attributes (i.e. async, defer, etc.), we add a settings.php setting which is an array of attribute names that can be grouped, with the default being ['async', 'defer'] - this will make it easy for sites to opt into aggregating with other attributes based on their needs and be more future proof.

I'm uncertain if this is an active issue still for some. In my case for D10.3, it is. So I've updated for the patch from #73 to match with the latest changes I'm seeing in core.

I added a test.
But it seems I can't remove the draft status from the MR.

Left a few small comments on the MR

But will need a CR for the settings change. and probably should include in API section too.

I rebased the MR and added a change record: https://www.drupal.org/node/3526457

Sites can do that in a site-specific services.yml so it shouldn't prevent that.

Ran test-only feature https://git.drupalcode.org/issue/drupal-1587536/-/jobs/5897064 which shows the coverage.

Believe the rest of the feedback here has been addressed too.

There's still an open discussion here about moving to a service parameter instead of using $settings.

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.

@catch in #90 you wrote

There's still an open discussion here about moving to a service parameter instead of using $settings.

and set this back to NW from RTBC. Has this discussion come to a conclusion?

Just ran into this with defer, I'll try finishing this with @grevil then!

@anybody no I mean there was an open discussion in this issue, which should have been resolved before the issue was RTBC, since I marked it needs work, that discussion hasn't moved on unfortunately. Maybe 'needs review' would be better though.

Also I'm not really sure the settings/container parameter is even necessary looking at this again.

What we could do instead is allow preprocess with those two attributes (hard coded), but add the attributes to the $group_keys in JsCollectionGrouper. Then if multiple async or defer files appear next to each other, they'd end up in the same aggregate, if they don't, they won't - but they'd still get minification etc.

Moving back to needs work for that.