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

domNode instanceof Element is suddenly false? #633

Open
Jahrhause opened this issue Jul 14, 2022 · 33 comments
Open

domNode instanceof Element is suddenly false? #633

Jahrhause opened this issue Jul 14, 2022 · 33 comments
Labels
question Further information is requested wontfix This will not be worked on

Comments

@Jahrhause
Copy link

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

  • Version: 3.0.1
  • Platform: Next.js
  • Browser: Chrome
@remarkablemark
Copy link
Owner

It should still work as expected. See CodeSandbox.

Try reinstalling your packages:

rm -rf node_modules
npm i

@jlegreid
Copy link

@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
Copy link
Owner

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

@tremby
Copy link

tremby commented Jul 15, 2022

I'm seeing the same thing.

@Jahrhause
Copy link
Author

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
Copy link
Owner

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
Copy link

tremby commented Jul 15, 2022

@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
Copy link

tremby commented Jul 15, 2022

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

@remarkablemark
Copy link
Owner

Sounds good @tremby

@remarkablemark remarkablemark added the question Further information is requested label Jul 15, 2022
@remarkablemark remarkablemark pinned this issue Jul 15, 2022
@TheRarita
Copy link

TheRarita commented Jul 21, 2022

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
Copy link
Owner

@TheRarita I think your CodeSandbox link is incorrect?

@TheRarita
Copy link

@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
Copy link
Owner

No worries @TheRarita. Thanks for the update.

@charkour
Copy link

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
Copy link

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
Copy link
Owner

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

@charkour
Copy link

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
Copy link
Owner

@charkour A reproducible demo would definitely be helpful!

@JiriLojda
Copy link

@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.

JiriLojda added a commit to kontent-ai/react-components that referenced this issue Nov 7, 2022
…placer

- it is a (hopefully temporary) workaround for an issue where the recommended instanceof check doesn't work (see remarkablemark/html-react-parser#633)
@remarkablemark
Copy link
Owner

@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.

JiriLojda added a commit to kontent-ai/react-components that referenced this issue Nov 23, 2022
…placer

- it is a (hopefully temporary) workaround for an issue where the recommended instanceof check doesn't work (see remarkablemark/html-react-parser#633)
@jt3k
Copy link

jt3k commented Nov 24, 2022

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
Copy link
Owner

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

@haleedev
Copy link

haleedev commented Mar 27, 2023

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
Copy link

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
Copy link

haleedev commented Mar 27, 2023

@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
Copy link

@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
Copy link
Owner

remarkablemark commented Mar 27, 2023

@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

littleMatch03 pushed a commit to littleMatch03/ai-components that referenced this issue May 2, 2023
…placer

- it is a (hopefully temporary) workaround for an issue where the recommended instanceof check doesn't work (see remarkablemark/html-react-parser#633)
@sauron918
Copy link

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

@remarkablemark
Copy link
Owner

@sauron918 can you try deleting and reinstalling node_modules?

@remarkablemark remarkablemark added the wontfix This will not be worked on label Jul 2, 2023
@bricejar
Copy link

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

@remarkablemark
Copy link
Owner

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

@bricejar
Copy link

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

@jt3k
Copy link

jt3k commented Oct 18, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests