Library order asset weights do not work properly when a large number of javascript files is loaded between two jQuery UI libraries

Nice find

After thinking about it a bit, if non-integer weights are allowed I then I feel like changing the divisor to 3000 really isn't enough, this patch changes it to 30,000 instead. With this you'll need to load 3,000 JS files before the chance of a loading order issue comes up; assuming the weights used are tenths. If it's valid to use weights like -10.00001 then this approach is fundamentally flawed.

I think the real fix is to only allow integer weights and possibly increasing the divisor value as well. As it is, even with integer weights you can run into this problem if you're loading over 1,000 JS files. If it would work just as well to use something like 30,000 as the divisor, then I don't think there's a reason not to, that would make it very unlikely to run into this issue as I would imagine a site using 30,000 JS files would probably run into other issues.

can we get a patch with both the tests and the code change? because there is no guarantee this is fixed by the code :)

The "real" fix is #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering where weights are not even a thing anymore.

In the meantime I think this is a good enough fix.

Mentioned in the MR, but I think this is pretty close to RTBC, but I'd like the test coverage for this to not be dependent on jQuery UI since that has an expiration date likely happening before this fix is no longer needed. To be safe, there should probably be a new test-only patch with the refactored tests, while the MR can be the tests+fix.

Feel free to ping me on Drupal Slack when the test is refactored and I'll re-review, I'll be happy to see this one land.

When we get to D10, and drop IE support, can we solve this with ES module imports and let the browser sort it out 😈?

Patch in #11 worked for us.

I'd divide by 7000 instead of 3000.

Patch in #11 worked for me too

Patch doesn't works for me if bigpipe is enable.
drupal 9.3.14

I have bigpipe enabled and it works just fine.
Why do you think it is connected with bigpipe?

I have errors only when bigpipe is enabled.
The error occurred on all pages.

Patch #11 works on drupal/core (9.4.2)

I can confirm that the .diff in #11 is working for me on Drupal 9.4.3

Marked #3272295: JavaScript errors in admin when using autocomplete and CKEditor 5 as a duplicate of this, thanks @nod_ for pointing me here!

Patch #11 also works on drupal/core (9.4.6)!

Patch #11 works also well on D9.4.8
The patch applies well, but after some tests, some js and css files are not loaded anymore (found) and blocks some pages of the website.

It sounds like the patch is consistently working. However, the merge request needs rebasing before it can be merged: https://git.drupalcode.org/project/drupal/-/merge_requests/954#note_34300

If we could get that done, I feel like this would be RTBC.

bad news for me, in fact even if the patch applies well to the core of Drupal 9.4.8,
I have some js and css files that are no longer loaded (found) and that block pages of the site.
And by removing the patch it works again...
[...]
I need the patch to use it with the new ckeditor 5 module for D9.
I tested the patch in version #8
For the moment it seems to work without any inconvenience.

Re-rolled patch #11 in drupal 10.1.x

#8 is working fine. Thanks for the patch!

The latest dev release of the Webform module is running into this issue via translations.

@see #3329097: Issues with CKEditor 5, tokens, translations

The patch works as expected but I'm not seeing any immediate workarounds

Merge request !954 on D9.5.1 works.

Attaching media is completely broken in Umami profile when JS aggregation is enabled.
I think this can be considered as major issue.

jrockowitz wrote in #36:

The latest dev release of the Webform module is running into this issue via translations.

The issue with CKEditor vs CSS/JS aggregation is NOT fixed in the most recent 6.2.x-dev version.

Notice this issue has nothing to do with translations, but with incompatibility of CKeditor and CSS/JS aggregation. If aggregation is disabled, you can write text in the textboxes with CKeditor, otherwise you cannot, because there are JS errors blocking CKEditor from working as expected.

For the errors see #20 @: #3329097: Issues with CKEditor 5, tokens, translations

This seems a major issue.

It is a major issue. But the symtoms can differ from site to site depending on which css or js files you load... also in which order they are loaded.
Having said that: they can cause ALL kinds off issues!
The difficulty of diagnising this as the problem for any random js or css issue makes it a major issue in my opinion.
Since it is hard to get from a strange error to this issue ...

As a workaround until this is fixed: I am using #11 but I am dividing by 7000. Rarely had an issue.

The offer in #12 (and the MR comment above it) still stands. Move the tests to not depend on jQuery UI assets, and it's ready for RTBC. Making that requested change will also address the test that is failing on the more recent patches/MRs. That's one of the scenarios I wanted to prevent with that request.

Here's what I said further up and it still stands:

I'm concerned about the tests for this fix being integrated into jQuery UI tests. aIt's very possible jQuery UI will be removed before the fixes in this issue are replaced by https://www.drupal.org/project/drupal/issues/1945262. Since this fix benefits users well beyond those using, jQuery UI, I'd like to be sure the tests aren't accidentally removed with jQuery UI. (even if it means adding tests that are VERY similar to the jQuery UI ones)

Mentioned in the MR, but I think this is pretty close to RTBC, but I'd like the test coverage for this to not be dependent on jQuery UI since that has an expiration date likely happening before this fix is no longer needed. To be safe, there should probably be a new test-only patch with the refactored tests, while the MR can be the tests+fix.

Feel free to ping me on Drupal Slack when the test is refactored and I'll re-review, I'll be happy to see this one land.

Just refactor that test and I think this is good to go.

I agree. to #41
This is not jQuery specific.

Just ran into this with layout_paragraphs on Drupal 9.5.3. Shouldn't we mark this "Critical"?

Added

<h3>Workaround:</h3>
Disable JavaScript aggregation at <code>/admin/config/development/performance

to the issue summary.

#31 worked fine for me. Thanks !

Found this issue after having seen this kind of errors in the backoffice page console :

t / i / e .widget is not a function
i is not a constructor
Cannot set properties of undefined (setting 'formResetMixin") / (reading 'mouse')
$element.dialog is not a function

with a website using advagg, entity_browser and field_group

Maybe we should rename this issue in order to make it more easily findable for people.
e.g.
"Large number of javascript files may cause multiple random script errors due to loading order of js files"

The patch #31 works for me too, thanks! I had an error using the ebt_slick_slider module when adding media to the custom block: Uncaught TypeError: $element.dialog is not a function

We're running into this issue on several projects again and again in the last weeks with different results / error messages. I'm wondering why this didn't pop up earlier? Have there been related changes in recent core releases?

At the current state, I think it's important to find a solution here, should we perhaps increase the priority to critical?

In #10 @larowlan wrote

When we get to D10, and drop IE support, can we solve this with ES module imports and let the browser sort it out 😈?

Is that possible? Then we could at least look forward to a solution in 10.1.x (and should target that?!)

patch #31 works for us as well - had problems on taxonomy term edit pages after switching to ckeditor5

On my node form I have a CKeditor textarea field, a plain file field and a media field with media library widget.

Due to the broken js the files uploaded to the plain file field are not set to permanent which causes them to be deleted after 6 hours.

Hi, patch #31 works on my case: before the patch non admin users were not able to open the media library widget and other dialogs (when ckeditor5 was enabled). Thank you.

Both the patch in #31 and the MR are failing tests. The next step here would be to try to fix that test failure. Agreed this should be critical.

The MR needs to be re-done against 10.1.x and we need to use something other than jQuery UI to test it.

Created tests that aren't dependent on jQuery UI. Those tests also exposed that the same problem exists with CSS, so a fix was provided for that as well.

Updated IS to make it clear jQuery UI was where it was discovered, but the issue is not specific to it.

I need to give complete kudos for the tests.

#54 shows this covers the problem for js/css

So the actual change looks good to me and matches the proposed solution from the summary.

It pains me to put this back to needs work on the tests only, because its a long-standing critical - but I think we can improve the test in a few ways, and at least one of those changes (asserting a count) feels blocking

1. +++ b/core/tests/Drupal/FunctionalTests/Libraries/ManyAssetsLoadOrderTest.php
   @@ -0,0 +1,69 @@
   +  public function testLoadOrder() {
   

   If we're touching this, could we add a docblock here (if not can be fixed on commit)

2. +++ b/core/tests/Drupal/FunctionalTests/Libraries/ManyAssetsLoadOrderTest.php
   @@ -0,0 +1,69 @@
   +      if (str_contains($src, 'weighted_')) {
   +        // The test file filenames have numbers that match their loading weight,
   +        // extract just that number so we can use that to check file loading
   +        // order.
   ...
   +      if (str_contains($src, 'weighted_')) {
   +        // The test file filenames have numbers that match their loading weight,
   +        // extract just that number so we can use that to check file loading
   +        // order.
   

   Library entries support attributes, would it be simpler to add a data-weight attribute in the definitions and then we can use findAll('css', 'script[data-weight]') and similar for links.

   So that would remove the need for:
   * the array_filter
   * the explode, str_contains, substr, strpos

   I think it would make the test easier to read/understand.

3. +++ b/core/tests/Drupal/FunctionalTests/Libraries/ManyAssetsLoadOrderTest.php
   @@ -0,0 +1,69 @@
   +    $this->assertSame(array_filter($js_files_sorted), array_filter($js_files));
   ...
   +    $this->assertSame(array_filter($css_files_sorted), array_filter($css_files));
   

   Let's assert the count of files here, if both are empty, this will still pass (Same for the css).

   We can probably use ::assertGreaterThan(0, count($js_files))

   So its not a hard-coded number that is subject to fluctuation and therefore more expensive to maintain.

Addresses all the points in #59, great suggestions.

Reviewing the interdiff and looks as all points have addressed from #59

Neat trick with the data-weight.

Updated issue credits

Committed to 10.1.x

Moving back to 10.0.x for backport once we have a green test-run.

Backported in 10.0.x, needs re-work for 9.5.x because of the .es6.js files 😭 - going to be a lot of them, all empty

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -259,7 +259,7 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize, Language
-            $options['weight'] += count($javascript) / 1000;
+            $options['weight'] += count($javascript) / 30000;

So was the actual root cause for the problem the division by 1000? i.e. that there was not enough precision?

In any case: superb work, @bnjmnm! 👏 Plus, this helps make #1945262: Replace custom weights with dependencies in library declarations; introduce "before" and "after" for conditional ordering more feasible, by having better test coverage for our decades old weight-based system before we transition to something better! 🥳

+++ b/core/lib/Drupal/Core/Asset/AssetResolver.php
@@ -259,7 +259,7 @@ public function getJsAssets(AttachedAssetsInterface $assets, $optimize, Language
-            $options['weight'] += count($javascript) / 1000;
+            $options['weight'] += count($javascript) / 30000;

Hello, this works in my case.

Moving to needs work so it's clearer the patch needs a re-roll, vs. just a delayed cherry-pick.

Note that per #3357678: The block editor menu havent the "editor" (see #19 + #20 + #21) that this is causing CKEditor 5 to fail to load correctly in certain contexts too. This issue fixed it. It first shipped in a release in 10.0.9.

… but it's not yet fixed in 9.5.x. So this would appear to still be quite important to backport.

Caveat: I have not verified if this problem occurred in 9.5.

I have encountered this problem on any 9.5.x site I've enabled CKE5 without this patch so far - but only on editing a taxonomy term

9.5.x reroll

Wim Leers commented in #70:

Note that per #3357678: The block editor menu havent the "editor" (see #19 + #20 + #21) that this is causing CKEditor 5 to fail to load correctly in certain contexts too. This issue fixed it. It first shipped in a release in 10.0.9.

I'm working with 10.0.9 and I'm glad to tell that my problems with Webforms/CKEditor5 are solved now. Before it was impossible to write/edit text in some textboxes managed by CKEditor5 there.

Reroll for 9.5 looks good.

Committed/pushed the backport to 9.5.x, thanks!

Backport breaks HEAD of 9.5.x on PHP 7.3 (https://dispatcher.drupalci.org/job/drupal8_core_regression_tests/82519/...)

_If_ we decide to not rollback and do a 'hotfix', let's see if attached patch works.

Right..., measure twice, code once.
There are _two_ arrow syntax functions in the test.

Let's try this again...

Whoops. Committed/pushed to 9.5.x, thanks!

I'm using 9.5.9 and the problems seems to be solved for the admin user (user 1), but the problems seems to persist on user with low level of permissions.

Sorry, my mistake, it is fixed but has been released core versión with this patch.

#67 works for me- Thanks!!!

#72 work for me (Drupal 9.5.9, MySQL 5.7.30, PHP 8.1.7)

Confirming this works fine! Very much looking forward to the next 9.5 patch release with this fix!

Yay!

Updating title.

The issue is back with D10.1.
Since the other issue was postponed to D11.x, can we please make a patch for D10.1.

It was already in 10.1 right? see https://www.drupal.org/project/drupal/issues/3222107#comment-15007055

It's all the way back to 9.5

Thanks. You are right. Forgive me! Same symptoms ... I'll investigate...

It seems that https://www.drupal.org/project/simple_popup_views was this issue.
It behaved very much randomly like problems caused by this issue. All kinds of javascript file errors, without any logical source that varied completely from the number of installed and uninstalled modules. So I post it here. It may help someone.

It turned out that it was just a coincidence of multiple contrib modules which did forget to migrate core/jquery to contrib before the upgrade to 10.1 and some missing dependencies in this context. Sorry and thank you.

Does it work with 9.5.10? None of the patches apply to 9.5.10

Already committed to 9.5, see #77

Thank you! I'm on 9.5.10 and still having issues when aggregation is on. In random instances the ckeditor don't load properly.

@gwvoigt

Check out this issue: https://www.drupal.org/project/drupal/issues/3370930

We were also still seeing problems with missing ckeditor5 toolbars, and it was caused by aggregate files not getting written to disk due to mismatched hashes. There isn't a patch for < D10, but it's minimal changes and easy to generate a patch for if you aren't on D10 yet. Appears to have resolved our issues.

Is it possible that this also affects css at a simliar functionality?
When I aggregate css files not all styles are used.