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

forgot tags

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 :)

Reroll.

Looks like contextual links work. Tour is still broken.

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

You're right, wasn't clean. Little small adjustment to make it easier on the eyes.

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?

Reroll, There are two cases of conditional dependencies. I think the only problem could be with views-contextual but doesn't seems broken.

Anyway, last patch was before yml files so here it is.

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 :_)

The last submitted patch, 20: core-js-lib-diet-1945262-20.patch, failed testing.

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.

The last submitted patch, 28: 1945262.patch, failed testing.

The last submitted patch, 29: 1945262.patch, failed testing.

@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.

We can't do that, otherwise we end up with contextual.js before jquery, drupaljs and the rest and things crash. Need to remove all weights or none.

Wen we remove all the things, we can implement behavior order execution by extending 📌 Control the list of behaviors run by Drupal.attachBehaviors Needs work and having something declared in the Drupal.behaviors objects.

Drupal 8.0.6 → was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 → is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

I apologize about the whole "diet" part of the initial title and patch name, it was a poor attempt at wit. https://the-pastry-box-project.net/marc-drummond/2015-december-21

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.

Drupal 8.2.0-beta1 → was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

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'].

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

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.

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

Drupal 8.9.0-beta1 → was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule → and the Allowed changes during the Drupal 8 release cycle → .

> 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 🤓

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

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

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

Just looking at what fails if we remove the weights.
Added some before keys, they don't do anything yet.

with failing tests

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.

anyway, trying to bring down failures as low as possible before putting the weights back.

For the before/after processing I was thinking of doing that in LibraryDependencyResolver but I don't want to add new dependencies if they don't already exist in the list of dependencies.

bunch of failures left, probably missing dependency declaration.

For the before/after implementation i'm not sure at which level it makes the most sense to implement and I'm worried about circular "dependencies". At the very least I think that before/after should be a library-level key, not a file-specific key.

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? 🤓

That should make all test pass beside the 2 new explicitly failing tests.

I like the plan, theorically it should be possible to do it like that.

The last submitted patch, 77: core-noweights-1945262-77.patch, failed testing. View results →

#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.

📌 Compress aggregate URL query strings Fixed 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)

explanation is good, needs to be in comment inside the file

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)

Excellent, had a quick look and I like what I see :)

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?

Yay, 🐛 Library order asset weights do not work properly when a large number of javascript files is loaded between two jQuery UI libraries Fixed 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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

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.

@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 - 🐛 claro.jquery.ui css assets may be added the page multiple times Fixed

catch → changed the visibility of the branch 1945262-replace-custom-weights to hidden.

catch → changed the visibility of the branch 1945262-replace-custom-weights-my to hidden.

Attempted a rebase here. We re-organised internal.jquery_ui in core.libraries.yml since this was last worked on, and it wasn't that easy to reconcile. That plus some test changes were the main conflicts though, and fortunately the test failures are loud when you get things wrong so weren't too bad to correct.

I've pushed the rebase to a new branch/MR https://git.drupalcode.org/project/drupal/-/merge_requests/9158 since the old one was named '10.1.x' and so it was easier to compare the state from a year ago to the rebase.

Also hid two MRs here which appeared to be dead ends.

Pipeline is green now, so ready for review again.

catch → changed the visibility of the branch 10.1.x to hidden.

Wonder if the issue summary could be updated?

Wonder if this change will need a CR? Won't tag but just asking

Updated the issue summary and added a CR.

After commit, https://www.drupal.org/docs/develop/creating-modules/adding-assets-css-j... → will need an update.

This seems pretty solid as far as JavaScript goes, but CSS is a bit trickier because we mix the CSS_AGGREGATE_GROUP constants with weight in LibraryDiscoveryParser. I'm not sure what's best to do about that yet.

So no way to deprecate 'weight' key?

I would deprecate weight as a follow-up, as it's quite a piece of an extra work

Could that follow up be created?

I also am leaning towards deprecating weight in a follow-up, but would like more feedback/reviews here before opening it, in case we discover we need to do it here after all.

Bitten by this too - and I thought 🐛 Logic error in Drupal's lazy load for asset aggregation Active was the actual fix, but it ain't (yet). Reverting fixes some things for us (in our case, gin admin theme which breaks stuff in some cases for webmasters on node edit forms where the JS completely broken) .. will try to figure out if I can create a patch for gin - hints welcome as I'm not to familiar yet with the aggregation system)

This will probably need a rebase now that 🐛 Logic error in Drupal's lazy load for asset aggregation Active landed. That issue fixed some bugs in the current ordering logic, so we should be in a good position to validate this one now.

Still need to figure out what to do with CSS groups per #107. It may be that we can defer that to the follow-up that attempts to deprecate weight altogether.

@catch, updated branch with the latest changes from the upstream (actually a different issues was causing merge conflict), should be good to go now

Looks good, thank you

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

False positive

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed missing return type in new tests.

Re-solved the threads but appears to need a manual rebase (200 commits back), surprised the bot never picked it up.

the conversion to OOP hooks broke this MR, corresponding code should be moved to the class files, I can take a look later this week

This keeps causing problems, now also in Experience Builder, with a crazy work-arounds just having landed that would not have been necessary if this had been fixed.

So I tried testing this with the Tour module now in contrib. And it broke things out right, saying it couldn't find jquery even though I had it as a dependency. Tour may have a bug or it's dependencies off but this change could break existing things with no guidance where to go.

@smustgrave this is probably due to weight: -1 in tour module: https://git.drupalcode.org/project/tour/-/blob/2.0.x/tour.libraries.yml?...

Could you try the MR without the weight?

If tour needs to appear before something else, it might need to replace the weight declaration with the API added here.

Yup tried without the weight -1 and even tried

after:
- core/jquery

Similar to XB we have had to backport this to workaround some dependency issues and it is working well. We have several libraries that all depend on shared libraries. We were running into cases where the libraries were included in different aggregates and the dependencies were included in the minimal representative subset for both and ending up on the page twice.

Since the optimized aggregate urls don't track explicitly what other dependencies have been loaded like with ajax it would have required a lot of tinkering with weights and making duplicate libraries like with XB to get what the dependency sorting gives us out of the box.

This fixes https://www.drupal.org/project/drupal/issues/3506870 🐛 Tabledrag missalignment Active

Does this change make it so that I can declare before/after relative to a library that is not on the page? In other words, if that library is on the page, load this library before/after it, but do not add that library to the page if it would not already be there.

@liam morland yes that's exactly the idea.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rerolled

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rerolled