Proposal: Clean up core APIs

[trueadm]

Change add* to register*

I had some really nice feedback tonight from some of the old folks who used to work at Meta on DraftJS. They mentioned that the current pattern of addListener, addTransform etc was confusing – as it's assumed to be just "adding" something. They expected there to be an opposite API of removeListener and removeTransform.

They mentioned that we could do what other popular libraries do, and create an observe or use a register prefix, which infers that you're creating something that has a return signature. This would mean the following changes:

• addListener -> registerListener
• addNodeTransform -> registerNodeTransform

This should hopefully make it clear that these APIs return the removal variants of themselves.

Let's break out command listeners

They're not really listeners, and never have been. Listeners don't expect the function to return anything, but command listeners do – and they also have priorities! So let's come up with a better naming for both the listening part and the dispatching part! My current thinking is:

• addListener('command', function, priority) -> registerCommandHandler(function, priority)
• execCommand(commandType, payload) -> dispatchCommand(commandType, payload)

Let's remove root listeners

I really don't feel they push folks down the path of success. They encourage adding listeners to the contenteditable, but really they should probably be delegating events to the document and, where possible, leveraging commands instead.

[kraisler]

Agreed. The create syntax really clarifies the need for a teardown.

[acywatson]

addListener('command', function, priority) -> createCommandHandler(function, priority)

Do we want to consider doing what @thegreatercurve suggested in #1412?

createCommandHandler(commandName, function, priority);

e.g.

createCommandHandler('insertImage',() => {...}, 1);

[fantactuka]

I'm not sure createListener / createNodeTransform / createCommandHandler clearly imply that its result is a teardown right away. Just by its name I'd expect something like this:

const listener = createListener();
...
listener.unlisten()

[trueadm]

@acywatson that would actually make performance worse, as there are far too many listeners that listen to more than just a single command.

[trueadm]

I like Amy’s idea around the register prefix instead of create. What do others think?

[acywatson]

@acywatson that would actually make performance worse, as there are far too many listeners that listen to more than just a single command.

Would it? Right now you have to run every single registered listener every time any command is executed anywhere. With this approach, you would execute only the listeners that are registered for the command type that was fired, right?

[trueadm]

@acywatson If you think about it computationally, the logic we already have now means that although we may call many functions, each function is not only filtering the logic for the commands it cares about, but also providing actual logic too. If you move to have an extra step, we first need to filter by commands, but then also there'd need to be an additional step to filter to run the actual logic. Take this:

https://github.com/facebook/lexical/blob/main/packages/lexical-react/src/shared/useRichTextSetup.js#L39-L234

You'd have to pass in a huge array of all the commands you care about, which means converting that to some kind of map at creation, then during runtime, you'd have to further filter through it. Switch statements get converted to jump maps in a JIT, which is much faster than doing Map hashmap lookups.

[tylerjbainbridge]

+1 on Register & Dispatch.

[acywatson]

we may call many functions, each function is not only filtering the logic for the commands it cares about, but also providing actual logic too.

Yes, this is the problem. Why force users to filter? Also hard to type the payloads. That might be possible with John's approach?

If you move to have an extra step, we first need to filter by commands, but then also there'd need to be an additional step to filter to run the actual logic. Take this:

https://github.com/facebook/lexical/blob/main/packages/lexical-react/src/shared/useRichTextSetup.js#L39-L234

You'd have to pass in a huge array of all the commands you care about, which means converting that to some kind of map at creation, then during runtime, you'd have to further filter through it.

I don't think you actually need to do both. You pre-process the filtering via the map structure on command creation. In the callback body there should be no filtering necessary.

Switch statements get converted to jump maps in a JIT, which is much faster than doing Map hashmap lookups.

I'll take your word for it on this one, but are we really talking about that big of a difference here?

---

Not sure where the "extra step" is coming from. Are you talking about the Map lookup?

//useRichText.js
createCommandListener('indent', () => {doIndentStuff()}, 1);
createCommandListener('outdent', () => {doOutdentStuff()}, 1);
createCommandListener('insertText', () => {doInsertTextStuff()}, 1);

const commandListenerMap = new Map();
export function createCommandListener(commandName, callback, priority) {
   if (!commandListenerMap.has(commandName)) {
     const listeners = new Set();
     listeners.add(callback);
     commandListenerMap.set(commandName, new Set());
   }
   etc...
}

export execCommand(command, payload) {
  const listeners = commandListenerMap.get(command);
   listeners.forEach((listener) => {
     listener(payload);
   });
}

[trueadm]

@acywatson So taking a step back. When I originally designed commands it was exactly this API. However, after doing some profiling, I could see that there was quite a bit of overhead in triggering commands, given they're on the hot-path.

Looking into it, it was because we were doing dozens of Map lookups on each keypress. It turned out that calling lots of functions with switch statements was just that much faster. My theory is because switch statements can be statically optimized by the JIT, whilst dynamic coding for Maps means it has to bail out of the optimized path. Especially, as we not only have to do a Map lookup, but also a traversal through a Set, which seems quite expensive.

This was far more apparent on Firefox too – which meant out typing perf regressed from mid 3ms to 4ms just from this change alone!

[trueadm]

I just did some tests with for…of loops and it seems that this possibly negates much of the performance penalties I saw before! I'll dig more into this space tomorrow.

[thegreatercurve]

+1 to getting rid of the root listener. If we're spring cleaning, should we also consider getting rid of textcontent as well? A user could potentially infer either using update and inferring text content changes based on the active and previous editor states.

[thegreatercurve]

This might be a good point to bring up an additional proposal. Could we incorporate the names of the listener type into the listener function name as well? Like the below:

registerListener('update', () => {}) -> registerUpdateListener(() => {})` 

At the moment, on the web side, we loosely overload the registerListener function and then define the custom types in a .d.ts or .flow.js file.

We can't really this on the iOS side though as the compiler is a lot stricter. We can't match the listener type to the correct callback argument type, without encountering compiler errors or forcing the developer implementing the listener to define their own types in the callback.

Hence, why we're using this approach for the moment in the iOS version: https://www.internalfb.com/diff/D34879711

CC @amyworrall

[amyworrall]

I approve of all these proposals, including @thegreatercurve 's suggestion to rename the methods to registerUpdateListener, registerTextContentListener, etc.

[trueadm]

@thegreatercurve We should keep textcontent listeners, purely because it avoids the user from having to use update listeners too much (they fire much more frequently than textcontent listeners do).

We used to have explicit listeners, i.e. registerUpdateListener etc, the reason we moved away from them was to make it feel more web like. However, I'm not really seeing any real benefits from that right now, so you're probably right!

[thegreatercurve]

Cool, I'll try and add them back in on the web side once all the other changes have been added.

[trueadm]

Re-opening as there's a third point: "Let's remove root listeners". We can maybe make that its own issue though?

[thegreatercurve]

Adde separate issue for root listeners: #1589