Success/Error callbacks never called when the script element has the "nomodule" attribute. #108

orakili · 2023-01-19T06:49:34Z

Refs: https://www.drupal.org/project/drupal/issues/3334704

If a script element to be injected has the "nomodule" attribute (for example added in a before) then the browser will not fire any onload, onerror or onbeforeload event when the script is added to the page.

As a result, the function wrapping the decrement of numWaiting will not be called and the callback chain will be stuck.

Here's a tiny example with the issue (the success and error callbacks are never 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>

Modern browsers all support the nomodule attribute so maybe the code could check if the script element has the attribute and skip loading it unless it's < IE11?

The text was updated successfully, but these errors were encountered:

amorey · 2023-01-19T07:42:44Z

Thanks for the bug report. In terms of the desired behavior, which behavior is preferable for modern browsers (i.e. not < IE11):

skip loading of nomodule scripts altogether
load nomodule scripts but don't increment numWaiting

orakili · 2023-01-19T08:19:06Z

Both options seem reasonable and on modern browsers would have the same effect (script ignored).

Another option could be to load the nomodule script but call the callbackFn directly to decrement numWaiting?

Not necessarily a good reference but JQuery seems to skip loading scripts with the nomodule attribute: jquery/jquery#4282.

To be honest, I would go with the option that requires the less code changes :) to keep the library as tiny as possible.

Sorry if this is not a useful comment.

amorey · 2023-01-19T08:38:22Z

Thanks! I'll take a look to see which one requires the least code changes.

amorey · 2023-01-19T10:49:53Z

Another option is to add support for module/nomodule path modifiers:

loadjs(['module!/path/to/foo.mjs', 'nomodule!/path/to/foo.js'], function() {
  /* foo.mjs loaded in browsers that support type="module" */
  /* foo.js loaded in browsers that don't support type="module" */
});

What do you think about that approach? After taking a look at the source code I think that might be the simplest to implement.

orakili · 2023-01-19T12:48:34Z

That's an interesting new feature to explicitly indicate how to load a script. Unfortunately that would not solve the issue for implementations currently adding attributes to the script element via the before callback like the Drupal 9/10's ajax system but maybe that's something implementations could then fix on their side using this new feature. Not sure.

amorey · 2023-01-19T16:28:05Z

Yes, Drupal would need to change how they use loadjs to load nomodule scripts on their side. I think specifying module/nomodule in the path should be easier than in before() but I'd love to get their feedback first before settling on a solution. How long does it usually take them to address issues like this? If it would speed up the bugfix, I could publish a release candidate and you could issue a PR to patch the issue on their side. What do you think?

orakili · 2023-01-20T05:26:13Z

I would not expect a fast response to the Drupal issue. The scenarios in which case a Drupal site would be affected by the issue are limited so I'm not sure it will have a lot of traction. Pinging @theodoreb who I believe introduced loadjs to Drupal and may have an opinion.

In any case, I'd be happy to test whatever change is made to loadjs and create a patch for Drupal accordingly.

theodoreb · 2023-01-20T12:33:45Z

Thanks for the ping. Seems simple enough to address at the different levels.
Without thinking too much I'd say the jquery behavior of not loading the script makes sense although it's less nice to implement in loadjs.

We could live with the path modifiers, as long as the "module" one is not required to distinguish it from "nomodule"

amorey · 2023-01-21T07:42:52Z

@theodoreb Thanks for the feedback. The browser loads script tags withtype="text/javascript" by default so isn't a "module" modifier required in some form if you want load a javascript file as a module?

If it helps we can make the default script type configurable like this (or something similar):

loadjs(['/path/to/foo.mjs', '/path/to/bar.mjs', 'nomodule!/path/to/foobar.js'], {
  type: "module",
  success: function() {},
  error: function() {}
});

Which would be the equivalent to:

loadjs(['module!/path/to/foo.mjs', 'module!/path/to/bar.mjs', 'nomodule!/path/to/foobar.js'], {
  success: function() {},
  error: function() {}
});

Let me know what you mean by "module" not being required to distinguish it from "nomodule".

orakili · 2023-01-21T09:52:49Z

I think it meant still being able to add a script without the module! path modifier, like in the current version of loadjs:

loadjs(['/path/to/foo.mjs', 'module!/path/to/bar.mjs', 'nomodule!/path/to/foobar.js'], {
  // Load `/path/to/foo.mjs` with the current 4.2 behavior (~ type="text/javascript" from browser).
  // Load `/path/to/bar.mjs` with `type="module"`.
  // Skip `/path/to/foobar.js` in modern browsers (or add it but do something about the lack of fired events from the browser).
});

amorey · 2023-01-21T11:29:14Z

Yes, the default behavior wouldn't change. Here's the current proposal (with some slight modifications from the previous comment):

loadjs(['/path/to/foo.js', 'module!/path/to/bar.js', 'nomodule!/path/to/baz.js'], function() {
  // foo.js loaded with type="text/javascript" in all browsers (default behavior)
  // bar.js loaded with type="module" in modern browsers, skipped silently in legacy browsers
  // baz.js loaded with type="text/javascript" in legacy browsers, skipped silently in modern browsers
});

Note that modern/legacy will be determined by module support feature detection.

orakili · 2023-01-21T13:02:19Z

Sounds good.

amorey · 2023-01-23T13:37:41Z

I just pushed a release candidate with the functionality described above (v4.3.0-rc1):
https://www.npmjs.com/package/loadjs/v/4.3.0-rc1

Let me know if everything looks good and I can publish it as a new release.

orakili · 2023-01-26T04:18:07Z

Thanks. I tested the 4.3.0-rc1 version with the following ['module!dep1.js', 'nomodule!dep2.js', 'dep3.js'] in different browsers and got the expected results:

Recent Safari, Chrome, Firefox, Edge etc.: dep1 and dep3 loaded, dep2 ignored, success callback called
IE 11, firefox 50, Edge 15 etc.: dep2 and dep3 loaded, dep1 ignored, success callback called

amorey · 2023-01-26T17:44:54Z

Great, thanks for checking! Happy to hear it's working as expected. Let me know when there's a decision on the Drupal side to see if they have any requests before I publish it.

* Added module/nomodule path modifier support (#108) * Bumps version number to 4.3.0

amorey mentioned this issue Apr 11, 2024

4.3.0 #109

Merged

amorey added a commit that referenced this issue Apr 11, 2024

4.3.0 (#109)

429dd45

* Added module/nomodule path modifier support (#108) * Bumps version number to 4.3.0

amorey closed this as completed Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Success/Error callbacks never called when the script element has the "nomodule" attribute. #108

Success/Error callbacks never called when the script element has the "nomodule" attribute. #108

orakili commented Jan 19, 2023 •

edited

Loading

amorey commented Jan 19, 2023 •

edited

Loading

orakili commented Jan 19, 2023 •

edited

Loading

amorey commented Jan 19, 2023

amorey commented Jan 19, 2023 •

edited

Loading

orakili commented Jan 19, 2023

amorey commented Jan 19, 2023 •

edited

Loading

orakili commented Jan 20, 2023

theodoreb commented Jan 20, 2023

amorey commented Jan 21, 2023 •

edited

Loading

orakili commented Jan 21, 2023

amorey commented Jan 21, 2023

orakili commented Jan 21, 2023

amorey commented Jan 23, 2023 •

edited

Loading

orakili commented Jan 26, 2023

amorey commented Jan 26, 2023

Success/Error callbacks never called when the script element has the "nomodule" attribute. #108

Success/Error callbacks never called when the script element has the "nomodule" attribute. #108

Comments

orakili commented Jan 19, 2023 • edited Loading

amorey commented Jan 19, 2023 • edited Loading

orakili commented Jan 19, 2023 • edited Loading

amorey commented Jan 19, 2023

amorey commented Jan 19, 2023 • edited Loading

orakili commented Jan 19, 2023

amorey commented Jan 19, 2023 • edited Loading

orakili commented Jan 20, 2023

theodoreb commented Jan 20, 2023

amorey commented Jan 21, 2023 • edited Loading

orakili commented Jan 21, 2023

amorey commented Jan 21, 2023

orakili commented Jan 21, 2023

amorey commented Jan 23, 2023 • edited Loading

orakili commented Jan 26, 2023

amorey commented Jan 26, 2023

orakili commented Jan 19, 2023 •

edited

Loading

amorey commented Jan 19, 2023 •

edited

Loading

orakili commented Jan 19, 2023 •

edited

Loading

amorey commented Jan 19, 2023 •

edited

Loading

amorey commented Jan 19, 2023 •

edited

Loading

amorey commented Jan 21, 2023 •

edited

Loading

amorey commented Jan 23, 2023 •

edited

Loading