search

kubetail-org / loadjs

A tiny async loader / dependency manager for modern browsers (899 bytes)
MIT License
2.57k stars 149 forks source link

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

Closed orakili closed 3 months ago

orakili commented 1 year ago

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?

amorey commented 1 year ago

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

  1. skip loading of nomodule scripts altogether
  2. load nomodule scripts but don't increment numWaiting
orakili commented 1 year ago

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: https://github.com/jquery/jquery/pull/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 commented 1 year ago

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

amorey commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Sounds good.

amorey commented 1 year ago

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 commented 1 year ago

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:

amorey commented 1 year ago

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.