Optimize Editor#normalizeNode #5970
Conversation
🦋 Changeset detectedLatest commit: be4b0f7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // Determine whether the node should have only block or only inline children. | ||
| // - The editor should have only block children. | ||
| // - Inline elements should have only inline children. | ||
| // - Elements that begin with a text child or an inline element child should have only inline children. | ||
| // - All other elements should have only block children. | ||
| const shouldHaveInlines = | ||
| !(element === editor) && | ||
| (editor.isInline(element) || | ||
| Text.isText(element.children[0]) || | ||
| editor.isInline(element.children[0])) |
There was a problem hiding this comment.
this might be worth factoring into its own function, would allow implementers to easily enforce certain blocks to only allow block children (eg. lists -- currently if a list somehow ends up with a text node as its first child, its items will all become unwrapped)
| for (let i = 0; i < node.children.length; i++, n++) { | ||
| const currentNode = Node.get(editor, path) | ||
| if (Text.isText(currentNode)) continue | ||
| const child = currentNode.children[n] as Descendant | ||
| const prev = currentNode.children[n - 1] as Descendant | ||
| const isLast = i === node.children.length - 1 |
There was a problem hiding this comment.
old implementation used i as an index into the snapshot of the node children passed into normalizeNode and n as an index into the children of the live node as its changed--this would be useful if we could use i to avoid Node.get calls by reading from the cached node whenever possible, but needing to know the prev child, which might be a newly added node, creates complexity.
| Transforms.unwrapNodes(editor, { at: path.concat(n), voids: true }) | ||
| } | ||
| n-- |
There was a problem hiding this comment.
pretty sure there's an invisible bug if this runs. we add some number of new child nodes and step back to read them--but we still only run the loop a number of times equal to the number of original children (everywhere else we add a child we skip over it and num iterations stay the same)
I dont think it caused issues due to the multi-pass nature that normalizeNode gets called but we can avoid running it more times than we need to by accounting for it.
| element = Node.get(editor, path) as Element | ||
| n++ | ||
| } | ||
| if (n === element.children.length - 1) { |
There was a problem hiding this comment.
using if instead of else-if here so we can make the before and after nodes in the same pass--this could cause issues if any wrappers around normalizeNode rely on doing something in between but I can't imagine why anything reasonably would
1 participant
Description
normalizeNode makes some expensive checks that really stack up when a lot of nodes change at once
Issue
Addresses #5945, thank you to the people in that thread for the research & analysis
Example
Running the test described in the above issue, pasting 10000 lines into the plaintext demo:
main:
this PR:
Current version takes ~11.9 seconds on my machine to handle the paste event once dispatched by React, ~9.7s of which is spent in
normalizeNode. This PR shrinks those to ~1.9s and ~0.4s respectively.Context
as @12joan pointed out, most of the time is taken up by
isEditor, but there's only one editor in any given tree and its passed as an argument to the function so checking if a node is an editor is as simple asnode === editor.This implementation assumes any children that needed normalizing have been normalized already and are thus valid nodes. It also assumes its being called with normalizing disabled and so it can predict eg. that exactly one node will be removed when
removeNodesis called on a path.Checking for inline/blockiness of children is disentangled into two separate loops.
Adding
children: []to a node that is neither text nor element to coerce it into an element now happens when you callnormalizeNodeon that node, not on its parent.My regression testing has consisted of the mocha/jest suite and anecdotal testing with examples but this seems like the sort of area that could cause unexpected issues, especially mingling with custom extensions, but from what I can tell it seems fine.
Checks
yarn test.yarn lint. (Fix errors withyarn fix.)yarn start.)yarn changeset add.)