Contrib.social

Ajax.js commands stuck when adding JS with nomodule attribute

Open on Drupal.org →
Created on 19 January 2023, over 2 years ago

Problem/Motivation

(Note: this is a regression that affects 9.5+ and 10+)

When loading a script that has the nomodule attribute via ajax.js, the command execution will get stuck.

With https://www.drupal.org/node/3293812 → , Drupal introduced a new add_js command. The implementation of the add_js command uses the loadjs library. The logic of this command adds the attributes defined for the script, for example in a xxx.libraries.yml file, before loadjs attempts to load the file.

If one of those attributes is nomodule (for example for a polyfill dependency), then the success or error callbacks from loadjs will never be called because the browser (any recent one) completely ignores the script and will not fire any onload, onerror or onbeforeload event that the loadjs listens to for that file.

That results in the counter used by loadjs to determine when to call the success/error callbacks, to not be decremented for this script and thus the callbacks are never called and the ajax.js add_js command promise is not resolved etc.

This is a regression from < 9.5 which had a different mechanism to load the javascript dependencies via ajax.

Steps to reproduce

The following code illustrates the issue at the loadjs level with a stripped down version of the logic from the add_js logic from the ajax.js file. The "success" callback is never called. Without the scriptEl.setAttribute('nomodule', ''); the script is loaded and the success callback is called.

<!DOCTYPE html>
<html>
<head>
  <meta charset="utf-8">
  <meta name="viewport" content="width=device-width, initial-scale=1">
  <title>Test Load JS</title>
</head>
<body>
  <script src="https://cdn.jsdelivr.net/npm/loadjs@4.2.0/dist/loadjs.min.js"></script>
  <script>
    loadjs(['https://cdn.jsdelivr.net/npm/css-vars-ponyfill@2.4.8/dist/css-vars-ponyfill.min.js'], 'css-vars-ponyfill', {
      async: false,
      before: function(path, scriptEl) {
        console.log(path);
        scriptEl.setAttribute('nomodule', '');
      }
    });

    loadjs.ready('css-vars-ponyfill', {
      success: function() {
        console.log('success');
      },
      error: function(depsNotFound) {
        console.log(depsNotFound);
      },
    });
  </script>
</body>
</html>

The effect can be seen on a fresh Drupal install following those steps (just for the sake of illustrating the problem):

  1. Spin up a new Drupal 9.5.x site
  2. Enable the "Block" module
  3. Edit the "core.libraries.yml" file and under drupal.collapse, replace the line misc/details-aria.js: {} with misc/details-aria.js: { attributes: { nomodule: true } }
  4. Clear the cache
  5. Go to /admin/structure/block, click a "Place block" button, in the popup, chose a block and click "Add block"
  6. With the nomodule change from (3) above, the content of the popup will not change to the block settings because the ajax command to update it will not be executed
  7. Just to confirm, remove the change from (3), refresh the cache and try again and this time the popup's content will be updated properly to go to the next step to add the block

Proposed resolution

I'm not sure at which level (Drupal or Loadjs) the issue should be solved. It's not really a problem in loadjs (by default, it doesn't handle attributes to add to a script, those are added by the Drupal ajax.js logic).

I think the remediation could be in the ajax.js add_js logic with 2 options:

1. Do not load a file with the nomodule attribute
2. Or, do not add the nomodule attribute

All recent browsers support the nomodule attribute. The versions supporting this attribute match the Drupal browser requirements → so option 1 would probably be fine.

See also https://www.drupal.org/project/drupal/issues/3181050 → and https://www.drupal.org/project/drupal/issues/3263823 →

Note: skipping scripts with the nomodule attribute is also what jquery seems to do: https://github.com/jquery/jquery/pull/4282

Remaining tasks

  1. [ ] Decide where this should be solved
  2. [ ] Patch ajax.js or loadjs.js
🐛 Bug report
Status

Active

Version

9.5

Component
Ajax  →

Last updated about 8 hours ago

Created by

🇯🇵Japan orakili

Live updates comments and jobs are added and updated live.
  • Regression

    It restores functionality that was present in earlier versions.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment about 2 years ago →
    cilefen
  • Comment 3 months ago →
    🇫🇮Finland jessesivonen

    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.

  • Comment 3 months ago →
    🇫🇮Finland jessesivonen

    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.

  • Comment 3 months ago →
    🇫🇮Finland jessesivonen

    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.

  • Comment 2 months ago →
    🇫🇮Finland jessesivonen

    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.

  • Merge request !11171Issue #3334704: Fix Ajax commands getting stuck when adding JavaScript with nomodule attribute → (Open) created by Unnamed author
  • Comment 2 months ago →
    🇫🇮Finland jessesivonen
  • Pipeline finished with Failed
    2 months ago
    Total: 560s
    #420729
  • Comment about 2 months ago →
    🇺🇸United States smustgrave

    Thanks for adding test coverage.

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

  • Comment about 2 months ago →
    🇫🇮Finland jessesivonen

    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.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 1341s
    #434444
  • Pipeline finished with Success
    about 2 months ago
    Total: 347s
    #434669
  • Comment about 2 months ago →
    🇫🇮Finland jessesivonen

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

  • Comment about 2 months ago →
    needs-review-queue-bot

    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.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 517s
    #450250
  • Pipeline finished with Success
    about 1 month ago
    Total: 1212s
    #450347
  • Comment about 1 month ago →
    🇫🇮Finland jessesivonen
  • Comment about 1 month ago →
    🇺🇸United States smustgrave

    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!

  • Comment about 1 month ago →
    🇫🇮Finland jessesivonen

    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.

  • Pipeline finished with Success
    about 1 month ago
    Total: 916s
    #452176
  • Comment about 1 month ago →
    🇫🇮Finland jessesivonen
  • Comment 30 days ago →
    needs-review-queue-bot

    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.

  • First commit to issue fork.
  • Pipeline finished with Success
    29 days ago
    Total: 690s
    #458228
  • Comment 29 days ago →
    🇫🇷France prudloff Lille

    I rebased to make the bot happy.

  • Status changed to Needs work 3 days ago
  • Comment 3 days ago →
    needs-review-queue-bot

    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.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024