[PP-1] Allow setting aggregation groups for js files in library definitions

I'm spending some time with Drupal\Core\Asset so I can provide an informed opinion on this. Even if it's just a "looks good!", it'll be an informed "looks good!". Although I'm still familiarizing myself with everything Asset, a few things come to mind:

1. There's a default value that can be overridden by a config setting. This should have test coverage, perhaps something analogous to \Drupal\Tests\Core\Asset\CssCollectionGrouperUnitTest::testGrouper? Clearly there's some missing coverage as the the MR changes functionality but all tests continue to pass
2. The aggregation itself should be tested too. Looks like that may be a reasonably simple addition to \Drupal\KernelTests\Core\Asset\AttachedAssetsTest::testAggregation? Aside from basic coverage, this seems additionally important as there are other chunks of code dealing with JS Assets that may be assuming JS does not have configurable groups. (\Drupal\Core\Asset\AssetResolver::getJsAssets seems like it may do this, but tests would prove/disprove it). Such a test would essentially confirm it will work for CKEditor 5 so we don't have to open additional followups while working on #3224261: [PP-1] Front-end performance: concatenate CKEditor 5 JS assets for every unique configuration
3. I don't have a strong opinion on whether or not this should be considered, but it merits mention: Before group was un-API'd, it was understood that its value informed the loading weight. This re-introduction of group is only for aggregation purposes, so perhaps the config property could be named something different likeaggregation_group? Even though the value ultimately gets assigned to group in the stored config, the outcome is different from what group previously did.

+1 to the remarks in #4.

I think this basically means that \Drupal\Core\Asset\AssetCollectionGrouperInterface::group()'s implementation freedom becomes more restricted — but the existing interface and API does not specify very strictly (or very clearly…) the existing responsibilities and freedoms either. (Mea culpa!)

An even better approach? Perhaps too abstract and too much magic?

More generally, I am not sure this change is necessary. We could also choose to instead make the existing grouping logic smarter. For example, if a particular asset library (dependency) subtree is tightly related, then we could choose to put that in a group of its own. See https://en.wikipedia.org/wiki/Component_(graph_theory). That is tricky though: where does one draw the boundaries?
So … I think I agree that this change is probably the simpler (and "less magical") choice.

But I still wonder if there's a better approach here. In the end, this declaring of some group X on a bunch of asset libraries is essentially the same as what I just described in the previous paragraph: marking the assets in a particular asset library subtree — i.e. those with strong connections to each other and weak (or usually no) connections to other asset libraries.

I wonder if we should instead declare the starting point (i.e. the root) of a particular asset subtree as deserving its own group/aggregate? 🤔 But that too leaves the "where does one draw the boundary?" question 😬 And again would involve some level of magic.

Stepping stones towards a better approach?

To get more concrete instead of describing these abstract ideal states: I wonder what the output would be for a script that analyzes the entire list of Drupal core's asset libraries and lists all asset libraries that: A) are leaves (have no dependencies), B) have >1 dependents. For this list, sort them by most dependents to least, and show the dependents count.

For ckeditor5/ckeditor5 (or when it's in core: core/ckeditor5), it would be listed as one of the topmost of these, and would have dozens of dependents.

I think this information might lead us to see a way forward to a better approach?

And the relative order of the groups instead of going with weights, we let the developer declare the order they want the aggregates in, like the css layers?

The order should really be determined by library dependencies.
Which would mean we need #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering first.

There would then still be a possibility of someone declaring groups that are incompatible with dependencies:

library 1 - group 1

library 2 - depends on library 2, group 2

library 3 - depends on library 2, group 1

For this case, there's a couple of options:
1. Validate it (during discovery?) and throw an exception/log an error. Ignore the group. I think this would be good if we can do it.

2. Create three aggregates (messy):
Aggregate A - group 1
Aggregate B - group 2
Aggregate C - group 1

1. Validate it (during discovery?) and throw an exception/log an error. Ignore the group. I think this would be good if we can do it.

+1

If I'm wrong change it back, but I think this should be postponed on #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering.

Ohhhhh, long time no see, #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering, but that literally came up earlier today in a meeting with the CKEditor 5 team, as a more elegant solution to a tricky plugin loading problem that @lauriii had been battling! :D

Adding the issue this is postponed on to the issue title because I couldn't figure it out until I read my own comment...

Discussed this a bit with @nod_ in slack.

I think we can do this without the dependency issue having landed. We need to make a new aggregate every time we encounter a new group, but it shouldn't affect the actual order of assets, just whether they're in a new aggregate or not.

Also I'm wondering if we should add a new key to library definitions like 'unique aggregate' instead of using group directly, this would work the same as 'preprocess: false' but at the library level. We could use it for jQuery and ckeditor, which are already setting preprocess: false on their individual files (after #734080: Set preprocess: false for jquery.min.js to reduce duplication between asset aggregates. But we could also set it for:

- other libraries with large individual files
- other libraries with lots of files that would add up to a decent sized aggregate, internal.jquery_ui would be a good example.

There is a risk that contrib overuses it, but it can't do any more damage, or actually less, than setting preprocess: false.

The problem with groupings bigger than one library is that as soon as libraries vary between pages, it's almost impossible to prevent duplication again

e.g. if library A and library B depend on library C, but library A, B and C can all be independently added to a page, then a page can have:

A
AB
ABC
AC

I tried to come up with an approach for that problem in #3497475: Aggregate 'only ancestor' libraries in their own groups and while it worked in theory, it completely failed in practice. e.g. if we only aggregate 'A'-type libraries together, we can still have multiple libraries like A that can result in duplicates, and the same for B and C.

However if we selectively aggregate a handful of libraries independently, then they're never combined with anything else and can't create duplicates. The libraries that are still aggregated together can still create duplicates, but the duplicates will be smaller because they don't include the ones we're aggregating by themselves.

Thought about this again, and still think 'unique aggregate' will work. Per #18 don't think this is postponed on anything.

Made some progress here.

There are two main parts to this:

1. Added support for 'unique_aggregate: true' to libraries.yml - when this is in place, the js grouper will try to generate a 1-1 relationship between library and group. It can handle header vs. footer scope, just not files from other libraries in the middle of a library due to weight etc.

2. When generating URLs and in the asset controller etc. if a single library is sent as an argument, it generates the group with only that library, instead of using include/exclude.

The reason for #2 is that currently we send all the libraries on the page in the include query string, and when an aggregate is otherwise identical (the same filename) but generated via pages with different sets of libraries, we end up with different query strings - this is not good for CDN/varnish/browser caching.

I think we might be able to go further than #2.

The original reason we used include + exclude was to avoid sending the full list of libraries in the query string to avoid too-long URLs, and so that we can handle libraries being split across aggregates due to weight.

However, when all libraries within an aggregate are 'fully encapsulated' (e.g. not split), we might be able to just send the full list of libraries used in that aggregate compressed instead. This won't necessarily be longer than include + exclude because it's only for a single aggregate.

In JsCollectionGrouper there is some new logic using $seen_index etc. to check whether a library is seen twice in two different aggregates, we would need to make this handle multiple libraries instead of one, and also run even when unique_aggregate isn't set. But that shouldn't be too bad.

This would give us better browser/edge cache hit ratios, because now both the filename and query arguments would be identical whenever possible.

An example where this would definitely kick in is the touchevents test library - because it's in the header, it's usually unique to the header aggregate anyway, so we could remove unique_aggregate from there and it should still use the new mechanism. There might be other examples of aggregates within the umami test coverage but probably need to implement it to find out.

Performance tests catch this problem - especially the new node/add test method added in this issue, because they compare the full URL including query string when checking file size, not only the filename, so we should see improvements vs. not. Currently it's well over 1mb of js between three pages, so definitely a lot of potential savings there.

OK more progress but also found a showstopper.

Overall - got this working* with asset groups containing multiple libraries. If I manually browse around Umami, I see a lot of 304 status code which is what we want.

However, both locale_js_alter() and ckeditor5_js_alter() require the presence of a 'placeholder' library/file to determine whether and how they add js translation files. This requires having the full list of files available on the page when generating an individual aggregate, which we obviously don't have if we're trying to generate an aggregate from a specific list of libraries. The result is that the aggregates end up missing all the translation files which in turn leads to a 301 redirect because there's a mismatch in the assets between the main page and the aggregate request.

This means that both the single and multiple versions of this MR won't work as-is without breaking locale and ckeditor5.

There is #2607376: Remove on-demand JavaScript translation parsing and do everything on rebuild open for locale js translation parsing, but don't think that covers the specific problem here of the way the placeholder files/libraries are used - we'd need to figure out translation another way.

One possible workaround - locale js translation only works for Drupal libraries that contain translatable strings, we could allow libraries to declare themselves as translatable: false and then only use the libraries approach when none of the libraries in an aggregate are translatable - this should be a pretty quick proof of concept to do.

The other way would be to completely redo how JavaScript translation works so it doesn't require the placeholder files.

A further issue is that having seen looked at the translation mechanism, I have a feeling locale's JavaScript translation mechanism is already incompatible with Big Pipe and AJAX - because once the locale placeholder library is loaded on the page, it's not loaded again on AJAX requests so any new js assets loaded by those pages won't be translated. Turns out there's already a ckeditor5 issue for this at #3516264: CKEditor 5 loads all plugin translations on AJAX operations.

OK #3525743: Locale JavaScript translation doesn't take into account AJAX and #3516264: CKEditor 5 loads all plugin translations on AJAX operations are pre-existing bugs in locale and ckeditor5 that also won't work on AJAX requests, this issue just exposes the same problem in more situations.

If we fix those two bugs, I think the approach here should work, marking postponed on those two issues.

I combined the unique_js_aggregate approach with the css version over in #3565258: Support library-specific aggregates, closing the MR here.

That leaves this PP-1 on the weights/ordering issue again, but I also think we might want to see how far unique_css/js_aggregate gets us.