Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(patchUrlMappings): attemptSetNodeSrc on node itself, not only children #239

Closed
wants to merge 1 commit into from

Conversation

afgiel
Copy link
Collaborator

@afgiel afgiel commented Aug 1, 2024

No description provided.

@afgiel afgiel requested a review from matthova August 1, 2024 15:09
@@ -116,6 +116,7 @@ export function patchUrlMappings(
}

function recursivelyRemapChildNodes(node: Node, mappings: Mapping[]) {
attemptSetNodeSrc(node, mappings);
Copy link
Contributor

@matthova matthova Aug 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants