fix(patchUrlMappings): recreate script elements when patching their src attribute

[elainemckinley]

Currently, when a new script tag is created via an imported JavaScript package, we are changing the src attribute for that script tag when it goes through the normal MutationObserver flow. However, a refetch doesn't happen; MutationObserver only goes off when the HTML element is added, but never stops the outgoing request.

With this change, if the MutationObserver sees that it's modifying a <script src= attribute, it'll instead create a new script tag in the DOM next to the one it would have modified, and then destroy the original. Unfortunately this will still show the error from the request blocked by the CSP, but it will go off separately and make the new request with the second script tag as well, loading in the content.

I'd be curious to collect thoughts on this solution. It's obviously destructive to do this for things like img tags (since React for instance would break when it goes to rerender), but I'm hoping this will solve the problem for script tags.

Includes the change from #239, since it was needed to fix my local test case as well.

[elainemckinley]

🙃

[matthova]

Great callout. We may want to clarify this one is run from the root of the repo

[elainemckinley]

I couldn't node.clone() here because script nodes have internal state on whether or not the node has started its fetch. See https://html.spec.whatwg.org/multipage/scripting.html and search the page for "already started", or this StackOverflow answer.

[matthova]

Looks great. Thanks for calling out how we got here. It may be worth leaving a comment to explain the motivation

Well done 👏🏻

[matthova]

Copying my feedback from Andrew's PR

Super-opinionated-pedantic nit

We should rename the function to recursivelyRemapNodeAndChildNodes

or we can call attemptSetNodeSrc inside the original mutation's added nodes

  if (patchSrcAttributes) {
    const callback: MutationCallback = function (mutationsList) {
      for (const mutation of mutationsList) {
        if (mutation.type === 'attributes' && mutation.attributeName === 'src') {
          attemptSetNodeSrc(mutation.target, mappings);
        } else if (mutation.type === 'childList') {
          mutation.addedNodes.forEach((node) => {
            attemptSetNodeSrc(node, mappings);  // Heres the added line
            recursivelyRemapChildNodes(node, mappings);
          });
        }
      }
    };

Either works. I'm pro the code-snippet above as it should result in less function calls. But we should double-check that it still solves the core issue

[matthova]

looks great! 🚢