Ajax.js commands stuck when adding JS with nomodule attribute

New path modifiers (module! & nomodule!) have been introduced in loadjs v4.3.0. Documentation on them can be found from loadjs's readme. I suppose these path modifiers could be used when loading JavaScript assets having respective attributes set. So, instead of assigning the attributes in the "before" callback/hook, we should use the new path modifiers for cases when loading module or nomodule JavaScript assets.

If the above sounds like a good plan, I can contribute to make the modifications.

New path modifiers (module! & nomodule!) have been introduced in loadjs v4.3.0. Documentation on them can be found from loadjs's readme. I suppose these path modifiers could be used when loading JavaScript assets having respective attributes set. So, instead of assigning the attributes in the "before" callback/hook, we should use the new path modifiers for cases when loading module or nomodule JavaScript assets.

If the above sounds like a good plan, I can contribute to make the modifications.

I updated the issue to include type="module" case which is part of the same problem with nomodule. I also added a new proposed solution to the issue summary.

I'll start implementing the changes to ajax.js to support loadjs's new path modifiers.

I found out new information about the issue when testing it more: JavaScript with type="module" works quite well already without the fix. The problem I encountered is related to how JavaScript is incorrectly handled as CSS by loadjs when it has file extension ".css". That problem is related to Vite module with dev server enabled. Although the fix to this issue might stop the symptoms of that problem, it should be properly handled as a separate issue, which I just created: https://www.drupal.org/project/vite/issues/3505461 🐛 Ajax form API adds Vite CSS-in-JS modules as CSS Active

I removed type="module" from the issue summary, and continue working on trying to create a test for the changes which are supposed to fix the nomodule case.

Thanks for adding test coverage.

Unfortunately this change appears to have broken the javascript tests, re-ran twice but still failed.

I initially thought the failing CI pipeline might be due to some external factor that effects everyone, but now it seems it's not. I'll look into the failing tests more closely.

Rebased and fixed expected script file size on a perfomance test. Pipeline now shows green, so this is ready for review.

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.

Have not reviewed but appears to have merge conflicts

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

The performance tests are really annoying because of the constant conflicting changes to the expected values. I will rebase yet again tomorrow. If someone could be kind and review this regardless of conflict, I'd appreciate. I'm quite certain the conflict will happen again and I don't want this to get stuck on a review loop because of that.

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.

I rebased to make the bot happy.