Lower weight of classList library to optimize aggregation

In total, I had 10 JS script files. With this patch and explicitly setting the weight of all my external libraries to -100, I was able to change it to:

This beautifully illustrates the problem of doing this in such a simple way: sometimes, the loading order matters. Script A assumes script B has already loaded, which assumes script X has already loaded.

For remote scripts, this is less likely to be a problem (many don't have dependencies on other scripts), but not guaranteed to not be a problem. So your trick definitely works for your case, but not in all cases, which means we can't do it in Drupal core :(

As we've said before, weights are a bad/broken system. Which is why Drupal 8 made big steps towards removing it.

That brings me to dependencies. Drupal 8 removed weights in favor of dependencies. Using dependencies, we can reason about which script load orders are valid.

Today, Drupal 8 still generates weights internally. That's why you end up with remote files interspersed with aggregates. But if we'd use dependency information, we could reason based on dependencies, which would allow us to do exactly what you did manually by setting external libraries to weight -100: we could load external libraries first, and then have far fewer aggregates.

(I realize this is not exactly a great explanation, but I hope it made sense. I can revisit this and explain it better later if necessary. Currently short on time.)

core.libraries.yml has lots of weights :)

It doesn't. It only has it for the "old school ones", the libraries that also had explicit low weights in Drupal 7. Plus the libraries that are new that those libraries with low weights in Drupal 7 are now depending on in Drupal 8.

1. backbone
2. classList
3. domready
4. drupal
5. drupalSettings
6. html5shiv
7. jquery
8. jquery.once
9. jquery.ui
10. modernizr
11. normalize

(those all have ~-20 weights)

Plus some weird cases:

1. drupal.autocomplete
2. drupal.tabledrag
3. drupal.vertical-tabs

#1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering was committed and removed all remaining weights. It was then reverted because it broke a few things. And then it was never worked on again :(

I don't think dependencies just magically solve all those problems. We already have dependencies and that's exactly why we end up with an order like my example above.

That's not true. We still convert dependencies to weights, and that's what I was getting at in #7. See \Drupal\Core\Asset\AssetResolver::getJsAssets():

            // Always add a tiny value to the weight, to conserve the insertion
            // order.
            $options['weight'] += count($javascript) / 1000;

We conserve insertion order, but don't need to.

And we only generate a weight for each individual asset in the first place because we still have few weights lingering around. If we didn't have those (the ones quoted), then we could remove this altogether.

That results in 3 possible orders, all are *correct* according to the dependencies and work […] However, only the first option results in good aggregation but there is no way that we can know that based on the dependencies.

You're absolutely right!

To optimize for that, we need to add some assumptions/rules on top of the dependencies.

+1

Weights is currently what we have, but we could also build logic to e.g. load external JS first as long as dependencies allow it.

Yes, this please! We shouldn't introduce more uses of weights.

The main point of this issue is simply to optimize an already existing weight

That's also true.

I hadn't noticed in #7 that classList already has a weight :(

+++ b/core/core.libraries.yml
@@ -20,7 +20,7 @@ classList:
-    assets/vendor/classList/classList.min.js: { weight: -21, browsers: { IE: 'lte IE 9', '!IE': false }, minified: true }
+    assets/vendor/classList/classList.min.js: { weight: -25, browsers: { IE: 'lte IE 9', '!IE': false }, minified: true }

So then this is all that remains: add a comment explaining why this weight of 25 was chosen.

Slightly off-topic:

Accurately 'lte IE9' is wrong. classList.js also support other methods that IE10/IE11 doesn't. Back to the time when we release D8.0.0, FF and Safari and Android also doesn't support classList well I think (But yeah, we only used add/remove in active-link.js, most of the users will not run into the bug)

I was suggested somewhere to use a simple polyfill with className inside the active-link.js directly. If anyone interested, raise an issue again :)

works for me

Just fixing a language nit.

I think we should be more explicit into the comment as to why we're picking -25 instead of -99 or -26 or -24. After all, the difference between -25 and -21 is not that big. I originally assumed this had something to do with the IE 30 asset thing, but from the comments that doesn't seem to be the case at all.

I think it is clear enough as-is and don't know how to better describe that. So I feel this is RTBC, but not changing status to wait for @xjm's feedback

> the currently lowest is -22 and -25 is just the number I picked that is lower
I bet this is what @xjm asked to add to comment

I filed alternative issue to get rid of the problem for core #2955842: Deprecate classList library
But this one still makes sense for contrib which still use classList to work with SVG that is not supported in IE11