[5.x]: Fix JS modules not loading correctly in the control panel

[engram-design]

There's an issue for any field that contains a <script type="module"> in its assets when loading its field settings. From a new field interface, selecting the field via dropdown will load the HTML/CSS/JS for the field asynchronously. This is notable with Vizy, Hyper, Icon Picker and others, where we use Vue and Vite to handle field settings.

This results in an error when trying to load a script that's a module - the browser can't handle it.

This seems to stem from this change where you fetch the script first via jQuery's $.getScript() function, but this doesn't work well with modules. In addition, you're only passing in an array of URLs to this function, whereas we need to know if the URL is a module or not.

I did toy with fetching all attributes of the <script> element and use that, but decided to keep it simple.

So firstly, we store the script type and src, and pass into _loadScripts(). We then have to swap out jQuery's $.getScript() function for our own, but I've tried to keep it almost 1-for-1.

Sorry for the noisy PR, but figured I should rename some variables for clarity. I also didn't touch this._existingJs because it's just comparing URLs, not loading them.

[brandonkelly]

It’s important that lazy-registered JS is loaded via $.getScript(), which uses Ajax + eval() rather than adding a new <script> tag to the DOM, to workaround browser crossdomain restrictions (see #13715).

Maybe the PR could be adjusted to keep things the way they were, but start ignoring <script type="module"> tags altogether?

[brandonkelly]

Also, we’ve been using $.getScript() since 4.5.6. Does this bug not affect Craft 4 for some reason?

[engram-design]

Working fine in Craft 4, but you also treat scripts differently in Craft 4. Refer to _appendHtml where you just collect all the scripts and append to the body with $parent.append(nodes);

You can't exclude them from _loadScripts() as they are otherwise not added to the DOM. I suppose you can add a conditional in _loadScripts() before it's actually loaded, but you still need half of this PR to pass in the script type, not just the URL to load.

[engram-design]

Okay, so a more lighter touch would be:

const nodes = $.parseHTML(html.trim(), true).filter((node) => {
  if (node.nodeName === 'LINK' && node.href) {
    if (!this._existingCss) {
      this._existingCss = $('link[href]')
        .toArray()
        .map((n) => n.href.replace(/&/g, '&'));
    }

    if (this._existingCss.includes(node.href)) {
      return false;
    }

    this._existingCss.push(node.href);
    return true;
  }

  if (node.nodeName === 'SCRIPT' && node.src) {
    if (!this._existingJs) {
      this._existingJs = $('script[src]')
        .toArray()
        .map((n) => n.src.replace(/&/g, '&'));
    }

    if (!this._existingJs.includes(node.src)) {
+      this._existingJs.push(node.src);
+ 
+      if (node.type === 'module') {
+        return true;
+      } else {
+        scriptUrls.push(node.src);
+      }

-      scriptUrls.push(node.src);
-      this._existingJs.push(node.src);
    }

    // return false either way since we are going to load it ourselves
    return false;
  }

  return true;
});

So that if this is detected as a module, it bypasses _loadScripts() altogether, and returns true to have it part of nodes which is then used in $parent.append(nodes); but not in await this._loadScripts(scriptUrls);

[brandonkelly]

Yeah that’s what I was thinking. Can you adjust the PR and verify it works for you?

[engram-design]

No worries, done.

[brandonkelly]

Is it expected that the <script type="module"> tag would be omitted, if the same URL is added in a subsequent Ajax request? (Because on the second time, if (!this._existingJs.includes(node.src)) won’t pass.)

[engram-design]

I'm not fully across what _existingJs tries to achieve, but I assume it's to prevent scripts from being added to the DOM multiple times?

But we're returning a node not just the URL in that filter() call, so the first time it'll return the <script type="module" src="http://localhost:8080..."> and then it'll check against that src whether it's been handled already and not add it or load it. Unless I'm reading that logic wrong?

But I would expect that once loaded, a duplicate script shouldn't be loaded again - not just for script modules?

[brandonkelly]

I assume it's to prevent scripts from being added to the DOM multiple times?

Right.

But I would expect that once loaded, a duplicate script shouldn't be loaded again - not just for script modules?

I think so, but worth testing, e.g. by creating multiple Hyper fields in succession from the same CP page.

[engram-design]

So all good with the field settings, switching back and forth between types seems fine. I've also tested creating multiple fields with the new entry type + add field editor, which takes care of Matrix-like fields and sections, etc.

Similarly, for entries, scripts are only loaded once for multiple Hyper fields.

[brandonkelly]

On second thought, decided to just revert to how we are loading scripts in Craft 4.5.6+, if that’s working fine. I’m a little concerned with splitting out how we load things depending on type="script" vs type="module" because there’s still the crossdomain issues to consider, and now we’re also potentially loading things in the wrong order (all type="script" first, then all type="module"), which might cause dependency issues.

[engram-design]

Fair call, I'll give that a quick test. Thanks for making sense of this PR!

[brandonkelly]

5.0.0-beta.7 is out now with that change.