[lexical] Feature: registerMutationListener should initialize its existing nodes

[etrepum]

Description

When registerMutationListener is called, if there are any existing nodes with that type, then the listener should immediately be called once with any existing nodes in the DOM from the previously reconciled state.

This makes it easier to implement mutation listeners correctly, especially for plug-ins that may be loaded lazily, and mirrors the behavior for registerRootListener and registerNodeTransform where it is expected that the code will be run for existing content (though there are other types listeners that do not have this behavior, I think it really makes the most sense for mutation listeners to behave this way). Some existing mutation listeners either did not support lazy loading, or did this incorrectly (e.g. by checking for an exact node type match instead of considering node replacement as in #6349).

This new "synthetic" mutation can be detected from the listener by checking for the registerMutationListener update tag, and it can be skipped entirely with the new {skipInitialization: true} option argument (mutation listeners registered in this way will have the same semantics as before this PR).

Closes #6356
Closes #6349

Test plan

Unit tests are included. Only one existing unit test needed to be changed to use the new {skipInitialization: true} option.

I did have to fix an egregious react rules violation in CodeActionMenuPlugin that caused an infinite rendering loop with this change, the registration needed to be wrapped in useEffect. The previous behavior was basically a memory & performance leak, fortunately only in playground code.

[vercel]

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	Jul 14, 2024 11:09pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 14, 2024 11:09pm

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	28.53 KB (0%)
lexical - esm	28.34 KB (0%)
@lexical/rich-text - cjs	36.99 KB (0%)
@lexical/rich-text - esm	28.17 KB (0%)
@lexical/plain-text - cjs	35.61 KB (0%)
@lexical/plain-text - esm	25.51 KB (0%)
@lexical/react - cjs	38.86 KB (0%)
@lexical/react - esm	29.44 KB (0%)

[zurfyx]

Thanks Bob! This is intentional:

1. Conceptually Nodes aren't "created" when you register the function.
2. The listeners weren't meant to be called immediately upon registration (the Root listener was meant to be a one-off).

Note that this is also a dangerous breaking change, the API is backward compatible but the behavior is not. Internally, we commonly pair this function with a Map to extend the behavior of the Node independently and these now repeated events can cause trouble with the initialization.

#4099 is a potential option to mitigate the issue. I think it is reasonable to have this build-in via a property (don't have strong opinions on 2.) but at the same time I'd advocate for a more flat API instead of yet another configuration object.

Would like to hear what other think about this as well.

[etrepum]

$nodesOfType is not good for this use case because it only considers exact type, not node replacement by subclass, so it creates a different initial set than this version of registerMutationListener. If $nodesOfType worked I wouldn't really have been motivated to fix registerMutationListener this way.

[etrepum]

Other than the number of calls, the breaking API change is unlikely to cause problems because it will just emit an initial created event where previously you had to do a separate read to discover them and this was only implemented correctly one time in the monorepo (it was implemented wrong in two places, and not implemented at all in several other places). Implementing registerMutationListener in this way removes the need for #4099 altogether, making the API simpler, at the cost of a mostly harmless breaking API change.

Alternatively the new behavior could be added with an option, or a separate function/method, but I would prefer the breaking change because it makes it much easier to write correct code and will probably even fix a few latent bugs.

[fantactuka]

1. Conceptually Nodes aren't "created" when you register the function.
2. The listeners weren't meant to be called immediately upon registration (the Root listener was meant to be a one-off).

It depends on how editor's state is initialized. If initialEditorState is async then mutation listener will be initialized before it's executed and called for initial notes. It's likely less common use case though

Note that this is also a dangerous breaking change, the API is backward compatible but the behavior is not. Internally, we commonly pair this function with a Map to extend the behavior of the Node independently and these now repeated events can cause trouble with the initialization.

+1, I did check registerMutationListener + $nodesOfType (commonly used to handle initial nodes) internally and it relies on nodeID which should avoid double initialization, but there could be usages that just go and mutate some DOM for 'created' mutation without tracking nodeID at all

[fantactuka]

Re: registerMutationListener vs registerNodesOfType, I do agree with Bob that keeping simpler API is better here. Can think about rollout plan to simplify migration out of nodesOfType+registerMutationListener (e.g., making it opt-in for patch version bumb, and opt-out for minor (or v1 if we do it soon enough))

[etrepum]

An opt-in migration path sounds fine to me, we can use the existing option argument for that and explicitly use {skipInitialization: false} to get the new behavior now. The only place I was motivated to use {skipInitialization: true} in the monorepo was the TOC plugin (the one correct init-before-register implementation) and only because it is tracking state for multiple mutation listeners in one place so it would've been awkward to initialize it efficiently with the pair of initial calls.

[zurfyx]

$nodesOfType is not good for this use case because it only considers exact type, not node replacement by subclass, so it creates a different initial set than this version of registerMutationListener. If $nodesOfType worked I wouldn't really have been motivated to fix registerMutationListener this way.

This is not intentional, this function should look into replacements like we do elsewhere.

Re: registerMutationListener vs registerNodesOfType, I do agree with Bob that keeping simpler API is better here. Can think about rollout plan to simplify migration out of nodesOfType+registerMutationListener (e.g., making it opt-in for patch version bumb, and opt-out for minor (or v1 if we do it soon enough))

@fantactuka Can you expand on what a simpler API means for you? In either case, it would be a one-liner.

The concern I raised about the current implementation is that previously created nodes are also reported as created, and the (not-so-important) fact that we need to introduce another object to carry more conditionals once more.

[etrepum]

The registerNodesOfType utility from #4099 is useful, but for different use cases (sometimes you do care about update or destroy, rather then tracking a single set). In fact, it would be even simpler to implement registerNodesOfTypeListener with this version of registerMutationListener because you can get rid of the separate initialization code.

export function registerNodesOfTypeListener(
  editor: LexicalEditor,
  klass: Klass<LexicalNode>,
  listener: (nodes: Set<NodeKey>) => void,
  fireImmediately: void | boolean = false,
): () => void {
  let nodes = new Set<NodeKey>();
  return editor.registerMutationListener(klass, (mutations, {updateTags}) => {
    let newNodes: Set<NodeKey> | null = null;
    for (const [key, mutation] of mutations) {
      const created = mutation === 'created';
      const destroyed = mutation === 'destroyed';
      if (created || destroyed) {
        if (newNodes === null) {
          newNodes = new Set(nodes);
        }
        if (created) {
          newNodes.add(key);
        }
        if (destroyed) {
          newNodes.delete(key);
        }
      }
    }
    if (newNodes !== null) {
      nodes = newNodes;
      if (fireImmediately || !updateTags.has('registerMutationListener')) {
        listener(nodes);
      }
    }
  }, {skipInitialization: false});
}

Other than a potential complexity/performance trade-off of tracking previously created nodes separately (which is opt-in here), what are the use cases for having a mutation listener that ignores initial creates? I think in most cases the existing code is just wrong (doesn't support lazy loading at all or doesn't correctly track existing nodes), and if it is right then it is probably already has "defenses" to work around the fact that it is not seeing a complete picture of the DOM nodes (e.g. tracking nodes in a separate map so that an update can be considered to be a create) so the worst case is usually just doing extra work (which would've happened in the TOC plug-in if I didn't opt-in to skipInitialization) or breaking a very sensitive test (e.g. tracking the number of calls). There was one case where it exposed a bug (CodeActionsMenu calling register outside of an effect with no clean-up!).

[etrepum]

@ivailop7 good catch, the new TableHoverActions plugin did require {skipInitialization: false} for correct lazy initialization.

[ivailop7]

@ivailop7 good catch, the new TableHoverActions plugin did require {skipInitialization: false} for correct lazy initialization.

Thanks!

[ivailop7]

Leaving to @zurfyx to merge

[zurfyx]

For the sake of predictability I wonder if we should always call it regardless. This is sort of inline with my previous comment but less radical, I understand that libraries like Rxjs follow similar patterns.

⭐️ This predicatibility also gives users more flexibility and the option flag becomes more of a convenience rather than a must-have.

[etrepum]

I don't have a strong preference one way or the other, correct code won't notice a difference. The current zero check is "more" backwards compatible in that the behavior only changes if the mutation is registered after the initial reconciliation, so I think more tests will end up having to change a bit (the ones based on call count rather than meaningful substance). Since we ended up with an option and a deprecation phase I think either is fine. Anyone else have an opinion about this?

[zurfyx]

Is this the best term for users? Initialization describes the work done behind the users, for users it's merely skipping the first one. Do we even need? ⭐️

[etrepum]

We need an option because on the call it was discussed that we need the default behavior to be exactly the current behavior. When the default changes, it should be very rare to have to specify this option, which is why it should have a name that implies that the default is false. I don't have a strong preference as to whether it's called "skipInitialization" or "doNotSendCreateEventForExistingDOM" or whatever.

[zurfyx]

Stamping, sorry for the back and forth, let's maybe take some time next session to discuss the nits that are relevant for the version bump. Merging as soon as the tests pass since @ivailop7 and @StyleT have already given their thumb up