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

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 do this for both CSS and Javascript, so re-titling.

🐛 Aggregation creates two extra aggregates when it encounters {media: screen} in a library declaration Fixed 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()=>{}

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 🐛 Turbo: JS aggregation causes the RefreshLess JavaScript to evaluate more and more times on navigation Active 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.

On top of that, 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.