Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
| return x.nodeType === 1; | ||
| } | ||
|
|
||
| export function registerNodesOfTypeListener( |
There was a problem hiding this comment.
Like the idea, not sure about the name. Perhaps we can think of something better - let me dwell on it a bit.
Other thing - it's a bit weird to have this be the only listener not accessed through a LexicalEditor instance. Not a blocker, just calling out the inconsistency. Maybe it's fine because this is, in a sense, a "higher order" listener.
There was a problem hiding this comment.
Other thing - it's a bit weird to have this be the only listener not accessed through a LexicalEditor instance
That's a fair point. My thinking process was that the Core is already pretty bloated. IMO MutationListeners is an advanced topic and something that only power users need. But no strong opinions on this
There was a problem hiding this comment.
I'm good with this - can you add a doc block at the top of the function? See examples in the recent docs PRs
| } | ||
| } | ||
| if (newNodes !== null) { | ||
| nodes = newNodes; |
There was a problem hiding this comment.
I'm not sure I understand why we reassign nodes here and whether it's needed at all. This var does not seem to be used as we only trigger listener with nodes of type that were added (newNodes)
Also, curious if users would expect this listener to be triggered with all nodes of type vs just newly added ones.
There was a problem hiding this comment.
Hm, fair point, I guess part of the behavior you inherit from the nodesOfType name but it makes sense that this is not what you would often expect from an observer. I guess that alternatively we can have a callback that returns (added: Set<NodeKey>, removed: Set<NodeKey>, nodes: Set<NodeKey>)
|
@zurfyx Can this PR be merged? Or can you mark it as 0.12.0 if we want to introduce this as a breaking change? |
|
I think we might be able to abandon this, since #6357 basically covers the same use case and there's a cache for |
|
Closing this PR due to staleness! If there are new updates, please reopen the PR. |
$nodesOfTypeis an expensive function, it iterates all the tree. It turns out that people are abusing this function as a handy shortcut (as we have no better utility) which can have performance implications on long documents.There's two types of misuse:
$nodesOfTypethan building more optimal tailored algorithm (via transform parent or making some hierarchy assumption).$nodesOfTypeis much easier to use thanregisterMutationListenerwhen tracking nodes as a whole as the mutation listener requires you to build your own data storage, do the first pass and handle creation and deletion.This PR addresses the second, simplifies the whole mechanism and makes it easier to fall onto the pit of success
contenteditableflagInternal utilization reference https://fburl.com/diff/prwdmjw5