Skip to content

Add Current Node Index in replace Callback #1240

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

Closed
yaman3bd opened this issue Dec 25, 2023 · 6 comments · Fixed by #1260 or #1261
Closed

Add Current Node Index in replace Callback #1240

yaman3bd opened this issue Dec 25, 2023 · 6 comments · Fixed by #1260 or #1261
Assignees
Labels
feature New feature or request

Comments

@yaman3bd
Copy link

Problem

I'm working with HTML/JS code fetched from a CMS. This code may contain multiple scripts. I'm using next/script which requires each script to have a unique id. However, when there are many scripts, it's challenging to assign the correct id since the scripts are dynamically mounted.

Here's how I'm currently handling scripts:

parser(html, {
  replace(domNode) {
    if (domNode instanceof Element && domNode.type === "script") {
      const __html = domNode.children
        .filter((child) => child instanceof Text)
        .map((child) => (child as Text).data)
        .join("");

      const props = attributesToProps(domNode.attribs);

      return (
        <Script
          {...props}
          dangerouslySetInnerHTML={{
            __html
          }}
        />
      );
    }
  }
});

I could assign a unique id to each script, but this would require handling many cases and tracking which script has the current id and which script is currently mounting to avoid id duplication.

so I’ve come up with this solution:

import React from "react";

import Script from "next/script";

import parser, { Element, Text, attributesToProps, htmlToDOM } from "html-react-parser";

function parseHTML(html: string | null | undefined, fixedID: string) {
  //get all script tags and map them to an array with their index
  const scripts = new Set(
    html
      ? htmlToDOM(html)
          .filter((node) => node instanceof Element && node.type === "script")
          .map((_, index) => index)
      : []
  );

  return (
    html &&
    parser(html, {
      replace(domNode) {
        if (domNode instanceof Element && domNode.type === "script") {
          const __html = domNode.children
            .filter((child) => child instanceof Text)
            .map((child) => (child as Text).data)
            .join("");

          const props = attributesToProps(domNode.attribs);

          //get the first script tag and remove it from the set
          const id = `script-${scripts.values().next().value}`;
          scripts.delete(Number(id.replace("script-", "")));

          return (
            <Script
              {...props}
              id={`${fixedID}-${id}`}
              dangerouslySetInnerHTML={{
                __html
              }}
            />
          );
        }
      }
    })
  );
}

export default parseHTML;

I can’t use useMemo in scripts because it’s not in a React component or a hook. So, each time a re-render happens, the scripts will be recalculated. Also, I think that looping through the DOM to filter only the script tags and map them into an index may cause performance issues because the html may be large or contain many elements, and each time a re-render happens, the code will run again.

Suggested Solution

the best solution is adding the current node index as a parameter in replace callback. so I can easily have a unique id and have smth like: const cacheKey = ${fixedID}-${index}.

Keywords

next/script, dynamic script handling, unique id, replace.

@yaman3bd yaman3bd added the feature New feature or request label Dec 25, 2023
@yaman3bd yaman3bd changed the title Add Current Node Index inreplace Callback Add Current Node Index in replace Callback Dec 25, 2023
@remarkablemark
Copy link
Owner

@yaman3bd can you use transform?

parser(html, {
  replace(domNode) {
    // ...
  },
  transform(reactNode, domNode, index) {
    if (domNode instanceof Element && domNode.type === "script") {
      reactNode.props.id += index;
    }
    return reactNode;
  },
});

@yaman3bd
Copy link
Author

I cannot, it throws this error:

TypeError: Cannot assign to read only property 'id' of object '#<Object>'

also tried this:

Object.assign(reactNode.props, {
  id: `${scriptId}-${index}`
});

same error, also I have these TS errors in transform:

Screenshot 2023-12-25 at 11 30 56 PM

Screenshot 2023-12-25 at 11 31 17 PM

Screenshot 2023-12-25 at 11 31 39 PM

I tried to solve the errors, no luck

@remarkablemark
Copy link
Owner

Try React.cloneElement:

transform(reactNode, domNode, index) {
  if (domNode instanceof Element && domNode.type === "script") {
    return React.cloneElement(reactNode, { id: `${fixedID}-${index}` });
  }
  return reactNode;
}

@yaman3bd
Copy link
Author

it worked, thanks a lot for your help.
but I have one question, using replace to change the element, and then using transform to clone the same element only to pass id, would not cause performance issues?

@remarkablemark
Copy link
Owner

@yaman3bd it shouldn't cause performance issues since both calls are executed in the same block: https://github.com/remarkablemark/html-react-parser/blob/v5.0.11/src/dom-to-react.ts#L47-L58

@remarkablemark remarkablemark added question Further information is requested and removed feature New feature or request labels Dec 25, 2023
@remarkablemark remarkablemark added feature New feature or request and removed question Further information is requested labels Jan 10, 2024
@remarkablemark
Copy link
Owner

Update: I released a version that added the index as the 2nd argument in replace:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
2 participants