remarkablemark / html-react-parser

📝 HTML to React parser.
https://b.remarkabl.org/html-react-parser
MIT License
2.14k stars 129 forks source link

domNode instanceof Element is suddenly false? #633

Open Jahrhause opened 2 years ago

Jahrhause commented 2 years ago

Expected Behavior

domNode instanceof Element should be true

Actual Behavior

domNode instanceof Element is false

Steps to Reproduce

After upgrading from version 1.4 to 3.0 domNode instanceof Element is suddenly returning false

Reproducible Demo

import parse, { HTMLReactParserOptions, domToReact, Element, DOMNode } from 'html-react-parser';
import React, { FC } from 'react';

const Textblock: FC = () => {
  const htmlFromCMS = '<h1>Weird</h1><p>dsdsdsdsdsd</p>';

  const options: HTMLReactParserOptions = {
    trim: true,
    replace: (domNode) => {
      console.log(domNode instanceof Element);
      console.log('/******************************************************************/');
      if (domNode instanceof Element && domNode.attribs && domNode.type === 'tag') {
        switch (domNode.name) {
          case 'h1':
            return (
              <h1 className="sydb-textblock__heading sydb-textblock__heading--h1">
                {domToReact(domNode.children, options)}
              </h1>
            );
          case 'h2':
            return (
              <h2 className="sydb-textblock__heading sydb-textblock__heading--h2">
                {domToReact(domNode.children, options)}
              </h2>
            );
          case 'p':
            return (
              <p className="test">
                {domToReact(domNode.children, options)}
              </p>
            );          

        }
      }
    },
  };
    return (
      <article>
        {parse(htmlFromCMS, options)}
      </article>
    );
};

export default Textblock;

What could have happend?

Environment

remarkablemark commented 2 years ago

It should still work as expected. See CodeSandbox.

Try reinstalling your packages:

rm -rf node_modules
npm i
jlegreid commented 2 years ago

@Jahrhause @remarkablemark I am also having this issue, I have tried reinstalling my packages and it didn't seem to help. For now I have just replaced my domNode instanceof Element checks with domNode.type === 'tag' and that achieves the same result for my purposes at this time.

remarkablemark commented 2 years ago

Thanks for the heads up @jlegreid and sharing your workaround. I'll keep an eye out if more users are experiencing this.

tremby commented 2 years ago

I'm seeing the same thing.

Jahrhause commented 2 years ago

Just tried to downgrade to version 2.0.0 and now everything is back to the way it should be, so must be something in version 3.

For v.3 I used this workaround as of now:

const options: HTMLReactParserOptions = {
    trim: true,
    replace: (domNode) => {
      let element = domNode as Element;

      if (element.attribs && element.type === 'tag') {
        switch (element.name) {
          case 'h1':
            return (
              <h1 className="sydb-textblock__heading sydb-textblock__heading--h1">
                {domToReact(element.children, options)}
              </h1>
            );
          case 'h2':
            return (
              <h2 className="sydb-textblock__heading sydb-textblock__heading--h2">
                {domToReact(element.children, options)}
              </h2>
            );
          case 'p':
            return (
              <p className="test">
                {domToReact(element.children, options)}
              </p>
            );          
        }
      }
    },
  };

This way I still get the typings I need.

remarkablemark commented 2 years ago

Thanks for the updates everyone. Could I ask someone to try regenerating their lockfile as well as nuking node_modules?

rm -rf node_modules package-lock.json
npm i

Let me know if the error still persists if you try that.

tremby commented 2 years ago

@Jahrhause

For v.3 I used this workaround as of now: ... This way I still get the typings I need.

A bit better, imo, is to use a type predicate:

Define a function like

function domNodeIsElement(domNode: DOMNode): domNode is Element {
  return domNode.type === "tag";
}

Then, as for usage, as soon as you've done

 if (!domNodeIsElement(domNode)) return;

any code after that knows that domNode is an Element.

tremby commented 2 years ago

@remarkablemark I would but I've just set off travelling for a week. Hopefully someone else can test that in the mean time.

remarkablemark commented 2 years ago

Sounds good @tremby

TheRarita commented 2 years ago

Not sure if this is related, but I am getting when using code from readme about typescript - https://github.com/remarkablemark/html-react-parser#replace-with-typescript

Property 'attribs' does not exist on type 'DOMNode & Element'.  
Property 'attribs' does not exist on type 'Comment & Element'.ts(2339)

https://codesandbox.io/s/kk0ovr?file=/src/App.tsx:252-259

remarkablemark commented 2 years ago

@TheRarita I think your CodeSandbox link is incorrect?

TheRarita commented 2 years ago

@remarkablemark Sorry, and double sorry. I corrected the link and also realized where my mistake was. I did not realize that Element should be imported from this library and it is not the general base Element. The name is a bit confusing but also, I did not read it properly since it is clearly imported in the snipped I linked.

remarkablemark commented 2 years ago

No worries @TheRarita. Thanks for the update.

charkour commented 2 years ago

Hey @remarkablemark, after upgrading to v2 to v3, I had the same error. Removing and clearing the node_modules fixed the issue. Had to do the same to the CI/CD pipeline.

Any idea why this would seemingly break when node_modules are cached during the version update?

charkour commented 2 years ago

Okay, an update on the situation. After removing and reinstalling node_modules, domNode instanceof Element correctly returns a boolean value when the page is SSR'd, but not CSR'd (on Next.js). So any sort of page navigation that runs the parser will always return false for the domeNode instanceof Element check.

remarkablemark commented 2 years ago

@charkour Just confirming, domNode instanceof Element check is working on the server-side, but not working on the client-side?

charkour commented 2 years ago

Hey, thanks for the quick response. I was initially a little confused and reported the wrong thing.

In v3, the script tags will execute when the page is rendered on the server, but not when rendered on the client.

In v2, script tags are executed both on the server and client.

In both cases the domNode instanceof Element check is being used in the replace method.

If I have time at work this week, I could make a minimal repro demo if that is helpful.

remarkablemark commented 2 years ago

@charkour A reproducible demo would definitely be helpful!

JiriLojda commented 1 year ago

@remarkablemark Hi, I encountered the same issue so I created a simple reproduction. It was originally created with npx create-react-app --template=typescript. (CodeSandbox | GitHub) I hope it is helpful.

remarkablemark commented 1 year ago

@JiriLojda Thanks for creating the reproducible example. I can confirm that your CodeSandbox isn't working correctly, but when I forked and created my own CodeSandbox, it seems to be working as expected.

jt3k commented 1 year ago

faced with same problem. I am using version 3.0.4 and the problem is the same as in version 1.4.5. also my environment: node 8.16, typescript 3.9.9, webpack 4.17

inside the replacer, I am met by a domNode instance of the wrong Element that can be imported from

import parse, { Element } from 'html-react-parser';

i created a check inside the replace function,

    console log({
      assert1: domNode.constructor === Element,
      assert2: domNode instanceof Element,
      Element1: Element,
      Element2: domNode.constructor
    });

when inspecting in devTools, I found that the links lead to two identical files with the same content:

webpack:///node_modules/html-dom-parser/node_modules/domhandler/lib/node.js
webpack:///node_modules/html-react-parser/node_modules/domhandler/lib/node.js
image

This seems to be why typescript shows an error and the script doesn't work on the client. before, I just wrote "any" in those places where the typescript swears. but I would like to solve the problem more gracefully.

I'm surprised the playgrounds in your readme.md doesn't have this problem. so the problem is more likely on my side, maybe due to the lack of some kind of tree-shaking.

I haven't tested this in a production build, only in dev mode. I don't have much time, but I'm willing to help with any checks on my part

Thanks!

remarkablemark commented 1 year ago

Thanks for sharing your findings @jt3k. In your case, I think performing a type assertion makes sense since Element are 2 distinct objects.

haleedev commented 1 year ago

running into same issue. using latest version of html-react-parser 3.0.15 and using react 18.2. wanted to ask if there are other use cases . it works in other versions like v2 or v1

charkour commented 1 year ago

This is the workaround we are using, @haleetwilio

// the workaround: https://github.com/remarkablemark/html-react-parser/issues/616
// the bug: https://github.com/remarkablemark/html-react-parser/issues/633
const isElement = (domNode: DOMNode): domNode is Element => {
    const isTag = ['tag', 'script'].includes(domNode.type);
    const hasAttributes = (domNode as Element).attribs !== undefined;

    return isTag && hasAttributes;
};
haleedev commented 1 year ago

@charkour thank you for quick response. This definitely helps! :) @remarkablemark is there a plan to fix the bug in coming future? or is the use case too small (or the root cause is not clear yet) that we just have to use the workaround method? totally understand if the answer is latter - just want to inform our team accordingly so we can plan ahead

aside from this, thank you for the work on this package. it's awesome :) helping us a ton

charkour commented 1 year ago

@remarkablemark had previously asked for a reproduction and I couldn't consistently reproduce it, but it seemed to happen when npm was caching TS types while switching between versions of this package.

remarkablemark commented 1 year ago

@haleedev as mentioned above, this bug can't be consistently reproducible, so there is no plan to fix this. The current workaround suffices and it's documented in the README.md

sauron918 commented 1 year ago

Running into same issue in v4.0.0, no switching between versions were previously done.

remarkablemark commented 1 year ago

@sauron918 can you try deleting and reinstalling node_modules?

bricejar commented 1 year ago

Hi, just used this library for the first time and the same problem appeared randomly.

remarkablemark commented 1 year ago

@bricejar what version are you using? Can you use the workaround provided by one of the previous commenters?

bricejar commented 1 year ago

@remarkablemark I am using 4.2.5 and yes I am using a workaround for now. Thank you !

jt3k commented 1 year ago

Maybe there is a way to solve the problem without resorting to a workaround? let's fix it? because it’s clearly obvious what the reason is, instances are created by different Element classes, so inside the replacer function typescript does not recognize it as a correct Element instance

Valithor commented 2 months ago

Installed the package today, got the same problem right away