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

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 📌 By default, don't aggregate jquery.js Active . 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 📌 Aggregate 'only ancestor' libraries in their own groups Active 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 📌 Remove on-demand JavaScript translation parsing and do everything on rebuild Needs work 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 🐛 CKEditor 5 loads all plugin translations on AJAX operations Active .

OK 🐛 Locale JavaScript translation doesn't take into account AJAX Active and 🐛 CKEditor 5 loads all plugin translations on AJAX operations Active 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.