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

Iterate through the live NodeList instead of copying to an array in $generateNodesFromDOM #4164

Merged
merged 1 commit into from
Mar 21, 2023

Conversation

acywatson
Copy link
Contributor

The HTML deserialization design is partially predicated on the user having the ability to manipulate the DOM tree in the conversion functions - this provides virtually limitless flexibility in how incoming HTML can be processed.

Previously, we copied the DOM nodes at the top level to an array, then iterated through that to initiate the HTML -> Lexical transformation. This works fine in most cases, but it can cause an issue in a couple of edge cases where you're trying to combine sibling DOM nodes into a single LexicalNode. There's no way to prevent those sibling nodes from being processed, since they have already been copied to an array, removing them from the DOM does nothing.

Alternative proposed solution was to mark the nodes as "processed" and then ignore them in the next pass, but this doesn't prevent their children from being processed. Also considered allowing the conversion to return "true" to stop further processing, but finally decided that this is just a bug.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 20, 2023
@vercel
Copy link

vercel bot commented Mar 20, 2023

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Mar 20, 2023 at 11:22PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Mar 20, 2023 at 11:22PM (UTC)

@github-actions
Copy link

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
packages/lexical/dist/Lexical.js 26.85 KB (0%) 538 ms (0%) 303 ms (+33.85% 🔺) 840 ms
packages/lexical-rich-text/dist/LexicalRichText.js 37.68 KB (-0.02% 🔽) 754 ms (-0.02% 🔽) 259 ms (-52.88% 🔽) 1.1 s
packages/lexical-plain-text/dist/LexicalPlainText.js 37.66 KB (-0.02% 🔽) 754 ms (-0.02% 🔽) 278 ms (+25.72% 🔺) 1.1 s

@acywatson
Copy link
Contributor Author

Tables e2e tests are broken on safari at the moment.

@acywatson acywatson merged commit c1493b3 into main Mar 21, 2023
@zurfyx zurfyx mentioned this pull request Mar 24, 2023
@fantactuka fantactuka deleted the html-nodes branch July 6, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants