fix: Only append fragment when element is HTMLElement

[lsbyerley]

If merged, this PR will add a check to the $appendNodesToHTML function in lexical-html to only append the fragment if element is an instanceof HTMLElement.

An error occurs at this line when a TextNode override is created and this override returns a TextNode as opposed to an HTMLElement.

https://github.com/facebook/lexical/blob/d10c4e6e55261b2fdd7d1845aed46151d0f06a8c/packages/lexical-html/src/index.ts#L125

Example CustomTextNode override:

class CustomTextNode extends LexicalTextNode {
  static getType() {
    return 'custom-text';
  }

  static clone(node) {
    return new CustomTextNode(node.__text);
  }

  exportDOM() {
    const text = this.__text;
    const element = document.createTextNode(text);
    return { element };
  }
}

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 21, 2023 3:10am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 21, 2023 3:10am

[facebook-github-bot]

Hi @lsbyerley!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

[acywatson]

This is good - we’d also need to change the type of the element property in DOMExportOutput to be HTMLElement | Text

[lsbyerley]

This is good - we’d also need to change the type of the element property in DOMExportOutput to be HTMLElement | Text

Keep the null or replace with text? element: HTMLElement | Text | null;

[acywatson]

This is good - we’d also need to change the type of the element property in DOMExportOutput to be HTMLElement | Text

Keep the null or replace with text? element: HTMLElement | Text | null;

Keep null, good call

[acywatson]

@lsbyerley can you sign the CLA?

[lsbyerley]

@acywatson yep will do, i am waiting on an approval on my end

[zurfyx]

Thank you! Please, do also check the failing tests, it seems like unit tests are not passing

[lsbyerley]

I'm not entirely sure what else is failing. With my most recent changes, all unit tests are passing locally for me.

edit: looks like some e2e tests are failing, i'll check those out

[lsbyerley]

@acywatson do you have any insight as to why the majority of the e2e tests are passing except a few? https://github.com/facebook/lexical/actions/runs/5026862456/

[acywatson]

@acywatson do you have any insight as to why the majority of the e2e tests are passing except a few? https://github.com/facebook/lexical/actions/runs/5026862456/

Possibly just flakiness - do they fail locally?

[acywatson]

@lsbyerley are we still just waiting on the CLA here?

[lsbyerley]

@acywatson Correct. Hoping to have CLA approval today or tomorrow. Will update the thread when ready

[derekhubbard]

@acywatson The CLA has been signed, and looks like the CLA github bot already picked up on that - nice!

Let us know if we can help resolve the Vercel deployment issues. It doesn't look like I have access, so not sure how to troubleshoot.

[acywatson]

@acywatson The CLA has been signed, and looks like the CLA github bot already picked up on that - nice!

Let us know if we can help resolve the Vercel deployment issues. It doesn't look like I have access, so not sure how to troubleshoot.

Hm, looks like there are integrity and unit tests failures - might start with that + a rebase and see if that helps?

[lsbyerley]

@acywatson the unit tests are now failing because of a typescript error at the exportDOM function in LexicalTableNode. I don't recall seeing this before and I'm not sure why it doesn't like it now. Still looking into it

https://github.com/facebook/lexical/blob/b5d73eb2eb715890a6f262b263eeadb239d3137c/packages/lexical-table/src/LexicalTableNode.ts#L93C50-L93C50

[lsbyerley]

@acywatson unit tests should be passing now

[acywatson]

Awesome - I'm sorry to do this, but I just remembered that we are trying avoid using instanceof because it interferes with SSR in some cases. Instead, for this, we export isHTMLElement from @lexical/utils.

Can we use that?

[lsbyerley]

Awesome - I'm sorry to do this, but I just remembered that we are trying avoid using instanceof because it interferes with SSR in some cases. Instead, for this, we export isHTMLElement from @lexical/utils.

Can we use that?

No problem! I've updated the PR to use isHTMLElement instead

[lsbyerley]

@acywatson Looks like there are only a few e2e tests failing.. i can't seem to run them locally so I'm not exactly sure why they are.

[derekhubbard]

@acywatson Any update on this? Anything we can do to help get this in?