Possible command listener performance improvements

[thegreatercurve]

I'm working on adding command listeners into iOS, and found the below a little confusing as a n00b.

Issue

We allow engineers to add logic for different command types into the same command listener callbacks, forcing them to check for command type, whilst also allowing them to call listener callbacks out of order with different priorities.

editor.addListener(
    'command',
    (type, ...payload) => {
        if (type === 'insertImage') {
            // ...
            return true;
        } else if (type === 'insertVideo') {
            // ...
            return true;
        }

        return false;
    },
    0,
);

// The below is called first. 

editor.addListener(
    'command',
    (type, ...payload) => {
        if (type === 'insertImage') {
            // ...
            return true;
        } else if (type === 'insertVideo') {
            // ...
            return true;
        }

        return false;
    },
    1,
);

I don't think it's a major problem but on a larger application, it could make the command the logic a little hard to follow.

Proposal

i. Remove the need to check for command type from the function, by specifying it as an argument to addListener:

editor.addListener(
    'command',
    'insertImage'
    (payload) => {
        // ...;
    },
);

This should reduce the number of times every command listener is called, remove the overhead for developers to always need to check the command type, and it should allow us to better type the payload for native Lexical command types (i.e. we can remove this Flow any in favour of a conditional type).

ii. Remove priority ordering for command listeners in favour of ordering by when the listener is attached to the editor, or abstract it out of addListener by triggering Lexical native command listeners before user defined ones.

In the existing user plugins (i.e. ImageNode, HorizontalRulePlugin, MentionsPlugin), each command listener always has a very low priority, so these listeners are already called last.

Honestly, I just added it into HorizontalRulePlugin because I didn't really know what would happen if I set it to be high, which makes me wonder if we could just remove it altogether?

[acywatson]

This should reduce the number of times every command listener is called, remove the overhead for developers to always need to check the command type, and it should allow us to better type the payload for native Lexical command types (i.e. we can remove this Flow any in favour of a conditional type).

I've thought about this exact thing, but I was thinking that the reason it was done originally was to keep the overall number of registered listeners low. I was originally thinking that your change would be less performant, since we'd have to call more listeners, but really we could just run the listeners for a specific command type when it's executed, so I think it should work well.

ii. Remove priority ordering for command listeners in favour of ordering by when the listener is attached to the editor, or abstract it out of addListener by triggering Lexical native command listeners before user defined ones.

If we trigger them in order of attachment or native-first, it makes it impossible for the user to intercept and override the behavior of a given command, right? The idea behind the Command priority system is to enable this specific use case with loosely-coupled components. If you take away priority, what you have is basically DOM EventEmitters/Listeners.

[thegreatercurve]

Sorry for the late response @acywatson! Just saw your comment.

I was originally thinking that your change would be less performant, since we'd have to call more listeners, but really we could just run the listeners for a specific command type when it's executed, so I think it should work well.

Yeah, that's right. It removes the need to implement the additional checks within the listener callback as well.

If we trigger them in order of attachment or native-first, it makes it impossible for the user to intercept and override the behavior of a given command, right?

Basically, yeah, but what use cases are you thinking of in which they'd need to run listeners out of order in which they're attached? Maybe if they're using a third party wrapper of Lexical which implements a toolbar, but still the developer need to attach a listener to the editor instance, that could be a valid use case.

Either way, at the very least, we can probably default the priority to the top most priority, to avoid developers being required to pass it as an argument.

[trueadm]

The priority part is very important, see tables, or code blocks for examples where we operate at various different levels. We intentionally do this so developers avoid using DOM events directly, as we don’t want tree based propagation, but rather priority based propagation.