Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering

Yup.... Dependencies should determine asset order, not "weight".

This broke at least two tricky edge cases:

1. contextual.toolbar.js wants to hide its pencil icon in the toolbar when the overlay is open. Including when the URL fragment indicates that the overlay should open, meaning that the drupalOverlayOpen event is triggered and Drupal.behaviors.contextualToolbar.attach() must already have been executed. The removal of the weights broke that. Note that we may not introduce a dependency here because contextual.toolbar.js does not depend on overlay-parent.js, but instead it listens to it in case it exists.
2. tour.js adjusts its tour icon in the toolbar (and the corresponding tour tips) when the overlay is open. Analogous additional info here.

I think the only way to solve this is by setting up the listener outside of the Drupal behaviors, so that it runs earlier. I hope I'm wrong :)

I had no idea what this issue was about — it's witty, but this is hopefully a bit more clear :)

This is blocked on a mechanism that uses a DAG (Directed Acyclic Graph) to determine the order of the assets. We can then work around the problem in #3 by having the ability to mark a library to be loaded before another one, and having the DAG take that into account.

(As per a call with sdboyer and a discussion with nod_.)

Related: #1014086-114: Stampedes and cold cache performance issues with css/js aggregation.

Let's revisit this — I think we were able to remove some of the weights in the libraries.yml patch already, but there are certainly lots of remaining instances.

changing to use the more common tag, so the less common one can be deleted, so it does not show up in the auto complete and confuse people.

Is #1805552: Remove custom weights from library definitions a duplicate of this?

Tested, clicked around Views. Works well.

<script src="http://localhost:8000/core/assets/vendor/jquery/jquery.min.js?v=2.1.3"></script>
<script src="http://localhost:8000/core/assets/vendor/domready/ready.min.js?v=1.0.7"></script>
<script src="http://localhost:8000/core/misc/drupal.js?v=8.0.0-dev"></script>
<script src="http://localhost:8000/core/assets/vendor/underscore/underscore-min.js?v=1.7.0"></script>
<script src="http://localhost:8000/core/assets/vendor/backbone/backbone-min.js?v=1.1.2"></script>
<script src="http://localhost:8000/core/assets/vendor/jquery-once/jquery.once.js?v=1.2.3"></script>
<script src="http://localhost:8000/core/modules/contextual/js/contextual.js?v=8.0.0-dev"></script>
<script src="http://localhost:8000/core/modules/contextual/js/models/StateModel.js?v=8.0.0-dev"></script>

Oh, nice! Less code, same functionality.

Committed and pushed to 8.0.x. Thanks!

Could someone cross-link the issue that actually removes weight support for js? I can't find it atm.

Please revert this commit; it broke at least /admin/config/development/testing. Hence, this wasn't sufficiently manually tested. Is it possible that #12 only tested the JS in the Views module?

Sorry about that! Sometimes my enthusiasm for a small RTBC queue gets the better of me. :) Reverted.

yes, you can't filter and all checkboxes are gone ...

Removed simpletest_js_alter(). Simpletest form works again.

AttachedAssetsTest::testAlter() has a @see for simpletest_js_alter(). We should do something about it if we remove it.

btw, after this change the only hook_alter_js() implementation in core is locale :_)

simpletest depends on drupal.tableselect, so I guess we should just remove the test.

Done that.

Rather than removing the test, it might be better to add a mock that tests the hook_js_alter() functionality. It's in core, and it should be tested.

Needs work then.

Something like this? https://github.com/RobLoach/drupal/pull/4

Interdiff

• Added mock test for hook_js_alter()
• Fixed mock test for attached js alter
  

Wrong file, sorry. Here's the correct patch.

@RobLoach: Thanks for the test! I changed it so the module is only enabled for testAlter. I could have added a check for jquery in the test module, but this looks cleaner to me.

I have tested the issue on views and test module. Patch #32 is working fine.

Please find attached screen shot.

Adding Issue tag as #DCM2015. This patch was tested in Code Sprint.

+++ b/core/modules/system/src/Tests/Common/AttachedAssetsTest.php
@@ -389,21 +389,23 @@ function testRenderDifferentWeight() {
+    $this->enableModules(['attached_assets_test']);

Elegant solution :-) . What other places should we manually test before this goes RTBC?

There isn't any other hook_js_alter() but locale, and that one already fails in HEAD and is being worked on #2419897: Javascript translations are loaded in the wrong order due to missing dependencies.
So I guess we are good.
Aside of that, I don't know the *official* procedure for manually testing JS patches. IMHO there is no problem with RTBCing this one.

This patch makes several comment lies. We either need to remove those comments by verifying that the ordering requirements that used to exist no longer apply, or we can't remove all weights just yet…

1. +++ b/core/modules/contextual/contextual.libraries.yml
   @@ -3,14 +3,14 @@ drupal.contextual-links:
        # Ensure to run before contextual/drupal.context-toolbar.
        # Core.
   -    js/contextual.js: { weight: -2 }
   +    js/contextual.js: {}
   

   This comment is now a lie. And I'm afraid this means we'll have to keep this one for now.

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -49,18 +49,6 @@ function simpletest_theme() {
   -function simpletest_js_alter(&$javascript, AttachedAssetsInterface $assets) {
   -  // Since SimpleTest is a special use case for the table select, stick the
   -  // SimpleTest JavaScript above the table select.
   -  $simpletest = drupal_get_path('module', 'simpletest') . '/simpletest.js';
   -  if (array_key_exists($simpletest, $javascript) && array_key_exists('core/misc/tableselect.js', $javascript)) {
   -    $javascript[$simpletest]['weight'] = $javascript['core/misc/tableselect.js']['weight'] - 1;
   -  }
   

   Why can this simply be removed?

   This comment says simpletest.js must run *before* tableselect.js. But the library definition says the opposite:

   drupal.simpletest:
     version: VERSION
     js:
       simpletest.js: {}
     css:
       component:
         css/simpletest.module.css: {}
     dependencies:
       - core/jquery
       - core/drupal
       - core/drupalSettings
       - core/jquery.once
       - core/drupal.tableselect
       - core/drupal.debounce
   

   Which is it?

3. +++ b/core/modules/views/views.libraries.yml
   @@ -21,7 +21,7 @@ views.contextual-links:
        # Ensure to run before contextual/drupal.contextual-links.
   -    js/views-contextual.js: { weight: -10 }
   +    js/views-contextual.js: {}
   

   This comment is now also a lie.

Thanks for the review @Wim!

I'm not sure how to proceed.

1. and 3.) I tested diverse cases of contextual links with different contextual items (nodes, blocks, views) + inline editing using the standard profile and found no errors. It worked as expected and they were included in the same order than previously. Maybe is just *luck*.

2) simpletest.js is included after tableselect.js, but it works as expected (which makes me ask: is *included* order the same than *executed* order? I guess yes and the order is not relevant anymore, but clarification would help).

How can we verify that the order is not relevant anymore? Git log and check the changes on the files?

In the meantime, while testing 2) I found a different problem. We have e.g testing groups Action and action, and clicking on the checkboxes has the desired behavior. But if we click on the labels, always the same group is checked/unchecked. The problem relies on having the same lowercased element id in both checkboxes. If we think about keeping the casing is works for my browser (Google Chrome), but we should not rely on that as per http://www.w3.org/TR/html401/struct/links.html#h-12.2.1 it is invalid to have the same id even with different casing.
I will create an issue for that tomorrow, too late here...

#39: manually tested both of these.

1. This indeed works fine in HEAD, but it is not guaranteed. This is why we need "before" and "after" relationships: contextual.toolbar.js must run after contextual.js. But it does NOT depend on contextual.js: if contextual.js hasn't run, then it short-circuits itself: if there are no contextual links on the page, then contextual.js won't have been loaded, and there is no need to show the corresponding toolbar tab (the pencil icon) in the toolbar.
   This need is discussed several times in #1762204: Introduce Assetic compatibility layer for core's internal handling of assets but apparently it doesn't have its own dedicated issue.
   Due to lack of strong guarantees, I'm suggesting to keep this weight declaration in place for now. Until we have the ability to declare "before" and "after" relationships.

2. How can we verify that the order is not relevant anymore? Git log and check the changes on the files?

   Exactly.

   Also: oops, I was clearly confused. SimpleTest's JS depends on tableselect.js is equivalent with setting the tablselect.js weight lower! My bad. So this change is actually fine already :) (That hook_js_alter() implementation should have been removed when it was converted into a library, but that was clearly forgotten.)

3. This is only working correctly thanks to an implementation detail of contextual.js.

   This case is similar to the one in point 1, but instead of needing an "after" relationship, this one needs a "before" relationship.

Therefore, I propose to remove the changes I quoted in #38.1 and #38.3. The rest we can keep. That brings down the total number of "weight" usages to *two*. That'll make actually removing it easier, and a follow-up issue to add support for "before" and after" simpler too.

Pinging catch for his thoughts on adding "before" and "after" besides only "dependencies" to asset library declarations.

Control lists being broken by Drupal.attachBehaviors is somewhat related to this, but doesn't have to block this from moving forwards. I'm going to re-open this, as as still have explicit weight definitions in our libraries.

Without #39 being addressed, this is impossible. We'll need not only dependencies, but also before and after.

#2781577-17: Properly style outside-in off canvas tray lists yet another case where this is necessary; the only alternative right now is to implement hooks to alter weights. That example cannot specify dependencies: ['classy/dialog'], because then it'd depend on a particular theme to be loaded, which is not at all true. What it needs, is after: ['classy/dialog'].

Because this didn't happen, we now have #2905036: Lower weight of classList library to optimize aggregation :(

Let's get this going again!? :)

Note: this could and probably should be a separate issue, but I'd thought I'd raise my thoughts here for now.

Personally, I think the main problem is that we're not really using any of the JS_* groups anymore. Instead, we've been abusing it and placing everything in JS_LIBRARY by default (which, ironically, is not JS_DEFAULT).

From LibraryDiscoveryParser::buildByExtension():

// Unconditionally apply default groups for the defined asset files.
// The library system is a dependency management system. Each library
// properly specifies its dependencies instead of relying on a custom
// processing order.
if ($type == 'js') {
  $options['group'] = JS_LIBRARY;
}

This constant was chosen because of the "everything is now a library" movement that swept through core. While true, I think it's important to note that there should be a very clear distinction between what a "Drupal library" is and what a "Standalone JS library is" (outside of the Drupal-sphere).

This should have defaulted to JS_DEFAULT, not JS_LIBRARY.

True "libraries" (independent libraries that don't require other dependencies, like classList, domready, jQuery, underscore, etc.) are being grouped with the rest just because they're considered a "Drupal library" (which is a different concept).

Only these true libraries are the ones that should be in the JS_LIBRARY group IMO. This change alone would likely allow core to get away from arbitrary weights for these JS libraries.

We could even default to JS_LIBRARY at this point if the Drupal library doesn't specify any dependencies.

I'm sure all of this was done in an effort to reduce the number of aggregated bundles, but from what I've seen, even with this setup, we're still at a minimum of around 2-4 JS bundles depending on the site. Changing this to the above won't really affect this, but it would give clarity as to what is a "true library" and what isn't.

---

This all being said, I do think this issue is also important. Sometimes, one needs to be explicit as to "when" a library should be included. Having a mechanism that checks for a "before" and/or "after" dependency would be nice. Arbitrary "weights" are confusing and meaningless.

> we're still at a minimum of around 2-4 JS bundles depending on the site

I opened #2905036: Lower weight of classList library to optimize aggregation a while ago, which is a trivial change to save 1 additional aggregated JS file. That got pushed back due to a missing/unclear comment, I don't know how to really improve it to make it clearer, help welcome. This would IMHO be a nice performance improvement for everyone.

Anybody interested in taking this on? I can promise reviews 🤓

Things are moving in core on this front! #2258313: Add license information to aggregated assets just landed and #3302755: On-the-fly JavaScript minification is now unblocked.

I'm hoping we can start pushing this forward too soonish, because it'll unblock #3232810: [PP-1] Allow setting aggregation groups for js files in library definitions, which is blocking #3224261: [PP-2] Front-end performance: concatenate CKEditor 5 JS assets for every unique configuration.

I guess the only way to go about this would be to leave the weights as-is because that might cause mayhem in contrib to remove them. even in core dependencies are not set properly and it's "hidden" by the weights.

This was exactly my fear 😬

I don't yet see how we can introduce this without breaking BC 😬 Maybe … just … maybe … this could work:

1. Keep all existing weights, but add before + after
2. Trigger deprecation errors for every non-core asset library that uses weights
3. Cross-reference the existing weights of non-core asset libraries with the weights of all assets in the tree of assets in that asset library's dependencies, and deduce before and after based on this

⇒ this should in theory allow the asset library system to not rely onweight anymore, while still retaining BC

Thoughts? 🤓

At the very least I think that before/after should be a library-level key, not a file-specific key.

Absolutely!

P.S.: nice, shockingly few failures when using just Drupal core! 🥳 Looks like it should be doable to get it down to zero?
P.P.S.: would be cool if eventually we'd test this patch against contrib modules that use weights and have functional/functional JS test coverage as a way to validate the approach? 🤓

#76 sounds like how we ought to do it. My only question is with #2 I would assume we'd want to blanket trigger deprecations when we find weight and remove any declarations from core in the first place.

#3303067: Compress aggregate URL query strings was committed last week — yay! Let's get this moving now? 🤓

The Needs Review Queue Bot tested this issue. It 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.

hi @nod_, seems like rebase kinda "kills" notifications, sorry I've missed your comment. Added an explanation

MR removes all the weights throughout core, but before/after logic is still missing. However, as you can see all tests are passing, except the new one (before/after), and one that expected a specific weights (which are not there by the nature of the change)

accomplished so far:

1. no assets use weight explicitly
2. all tests are passing, except the new before/after and jQuery UI order one (as it needs to be heavily refactored)
3. btw tests pass without after/before implemented at all
4. weight is still being used
   • to sort assets by their level (?) (base, layout, component, state, theme)
   • by some alters in core

Afaik, everything works as expected, but probably some manual testing is required

The one test that is failing checks whether jQueryUi assets have a specific weights, which is not true anymore. Thus, we need a decision what to do with it: remove or refactor to test whether everything is working properly rather than just checking weights.

A few places where weights are still in use:

1. ckeditor5_js_alter() - https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/cke...

Not sure what exactly Drupal tries to achieve here, someone else needs to take a look

2. AttachedAssetsTest::testAlter() - https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/tests/Drupa...

Tests altering weight via a hook, but this should not be a case anymore. Remove?

3. settings_tray.theme.css - https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/set...

There is a mention of this issue, but a lot of things have changed since it has been added (not sure it is relevant anymore)

Next steps (imo)

1. Refactor existing library discovery to use different key for specifying an asset level (?) (btw, what should be the naming here?) - (can be a follow-up issue)

2. Deprecate usage of weight key with a deprecation message (can be a follow-up issue)

Yay, #3222107: Library order asset weights do not work properly when a large number of javascript files is loaded between two jQuery UI libraries landed, which means we'll be able to make this change with more confidence! 😊

Is there an option to just use native es6 imports and type=module and just let the browser sort this out now we don't have to support IE

One question is how does it work when a library that has a before/after is added but the library it should be loaded before/after is not added to the page at the same time?

So, in short, it collects all needed libraries first (pass one), and only after that (with the full list) it adds before/after logic (pass two)

Setting this to NR, as some guidance needs for the next steps, see #89

Moving to NW for the issue summary update

And failure in MR seems to be legit to the issue at hand.

Was hoping this would fix an issue I'm having with Paragraphs Library and Entity embed. For some reason in the final Entity embed modal while adding a paragraph library item, jquery ui's dialog button styling is overriding Claro's leading to some pretty ugly buttons:

This does not happen when embedding an entity in a paragraph on a node form, only via Paragraphs Library (i.e /admin/content/paragraphs/add/default)

Unfortunately the MR doesn't solve this particular issue.

EDIT: Further debugging - it seems like /core/themes/claro/css/components/jquery.ui/theme.css is being added multiple times to the page. Once when the page loads, once when I add a paragraph, and then again when I open the embed modal so that would explain why styles are getting overwritten.

@acbramley, well, this is good to be honest, as this issue is pure DX change, i.e. it should not fix anything (yeah, maybe, accidentally)

@Taran2L no worries - this was a red herring anyway. My issue - #3378341: claro.jquery.ui css assets may be added the page multiple times