facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
19.98k stars 1.7k forks source link

[RFC] Plugins, normalizers, and nodes #6066

Open zurfyx opened 6 months ago

zurfyx commented 6 months ago

[RFC] Plugins, normalizers, and nodes

Thanks for the feedback in #6025! Some very opinionated thoughts... (cc @acywatson @fantactuka @ivailop7 @etrepum @GermanJablo @abelsj60 @StyleT)

Lexical Core doesn't do much when it comes to product specific features. Facebook Comments has over 20 named features on the editor, most notably Mentions, Hashtags and Auto-reply prefixes. These features are now informally called "plugins", but there's no formal definition on what they are and the encapsulation we provide just evolved organically.

Multiple ways to hit the same goal

Since Lexical inception we have avoided having multiple routes to solving the same goal. There is often a better way and this can easily lead to inefficiences. A couple examples:

The proposals below are guided by this.

Node

The class-based Nodes are one of the key decisions behind Lexical. They perform well, they are type friendly and they can be overwritten safely.

Unfortunately, Nodes are overused now. Nodes used to represent the data (we intentionally excluded any reference to editor) and wrap simple configuration that either events and reconciler needed to know, but now they are responsible for too much to the point that you can build a full-fledged plugin in the Node itself.

There are opportunities for us to split the node responsibilities which not only will help devX but also reduce the budget size.

What should Nodes responsibilities be?

  1. Data, getters and setters. These are used by the core to build the EditorState.
  2. Configuration that gives the final shape to the Node and that the editor needs to know at all times, such as canInsertBefore or insertNewAfter that are needed to understand how to do the basics of text insertion and caret (see Read-only and lazy loaded Nodes section).
  3. Renderer. The reconciler needs to know how to render these Nodes, and this can happen as soon as the page is loaded and populated by a third-party source. While I recommend editable functions decoupling for read-only (see Read-only and lazy loaded Nodes section) I don't think there's obvious benefits for optimizing the size of headless mode which often runs on the server or is followed by a non-headless editor.

What should Nodes yield the responsibility for?

  1. Static transforms. We don't want users to build a full-fledged plugin inside the Node itself. It is not feasible to build most plugins this way, for example, plugins that have UI attached like CharacterLimit or plugins that depend on multiple nodes like Hashtag. This wildcard transform function inside the Node causes unnecessary overhead for developers to find where the code lives. We can improve the UX with a simple tweak (see Normalizer section).
  2. Import and export DOM functions. There is only a subset of Lexical that uses clipboard HTML so we don't have to bundle this heavy part into the core (see Revised EditorConfig section).
  3. Utility functions. It is convenient to have all functions listed underneath the class name but this introduces two risks, 1) an easy path for users to misuse them, via overrides, and 2) causes the bundle size to increase unnecessarily as we don't tree shake Node props. But it's also fair to say that we want to mimic the DOMElements API and provide the flexibility and reusability aspect that classes give us. However, there are methods that provide little value underneath Node, for example getCommonAncestor, with no usages in the playground, that should be moved inside @lexical/utils or getCordsFromCellNode that should either be reshaped inside TableCell or moved into the @lexical/table module. As a rule of thumb, if the method is only useful in certain environments, it's probably best as a utility.

Grey area:

  1. Multi-purpose, heavily used functions such as getChildren, insertAfter or remove. Mimicking DOMElements, convenience, (used by the reconciler), and the fact that isolating them wouldn't yield obvious bundle size wins justifies having them inside Node.

A plugin

Plugins fulfill the feature behavior by leveraging Nodes. They are independent code that listen to the Lexical lifecycle and commands to manipulate the EditorState.

The heavy use of React in this repo already helped us shape plugins as standalone units that are easy to identify:

function MyPlugin(): JSX.Element {
  useEffect(() => {
    return editor.registerNodeTransform(...);
  }, []);

  return <SomeUI />
}

The responsibility for plugins is to coordinate the CRUD of these Nodes, and the fact that they work like libraries comes with many advantages:

  1. Plugins understand the product lifecycle, they can tell when the UI mounts and dismounts and can communicate with their state management system.
  2. They can render UI beyond the editor itself. For example, the character count on the CharacterLimit plugin or the table of contents inside TableOfContents plugin.
  3. They can be lazy loaded, and they're not strictly required on editor load.

Given how effective this React encapsulation has proven to be so far, my proposal is to formalize the concept:

function MyPlugin(): JSX.Element {
  useEffect(() => {
    return editor.registerPlugin('MyPlugin', [
      editor.registerNodeTransform(...),
      editor.registerNodeTransform(...),
    ], [TableNode, TableRow, TableCellNode]);
  }, [...]);

  return <SomeUI />
}

The idea is that the plugin registration allows us for easier debugging and devX, where we pass the plugin identifier and optionally the Nodes it depends on for an onload runtime check. To optimize for cases where different logical units are required (i.e. different dependency array in React), the same plugin can be registered more than once.

Read-only and lazy loaded Nodes

A full-fledged editor can be heavy, lazy loading the plugins is only half of the equation, the other are the nodes. A Node like TableNode imports LexicalTableSelectionHelpers with 1.7k lines of code.

Nodes have to be loaded before we can render the EditorState but a method like remove doesn't play any role in the bootstrap.

My proposal is to decouple the read and edit functionality of the Nodes, where the edit version is a superset of the read-only version. This also allows us to ultimately build a read-only version of the editor, which can be useful for surfaces where we only use to editor to display the content.

The editor will provide hooks to provide the complete version of the node and the first update will be blocked until these nodes are provided (or at least the relevant Nodes).

This is worth its own issue so I'll cut it here.

Normalizer

By design, Lexical is resilient, functions like insertNodes are very versatile and even in the worst case scenario, a crash, the update flow will automatically roll back to the last good version (only a minority of events cause the editor to throw like two consecutive crashes or DecoratorNodes).

This was very well received internally, as misbehavior from our core code or product didn't lead to data loss.

However, versatility can be problematic, an invalid EditorState can cause certain plugins to misbehave indefinitely. This is why we started #3833 to discuss the idea of Schemas that other libraries like Prosemirror already come with.

To fix this problem, we want to introduce Normalizers (this is not a new idea, @GermanJablo, @acywatson, and others already explored it in #3833). The reason why I'm now convinced they are a good fit is because they respect the efforts on resiliency while addressing the invalid EditorState problem.

createEditor({
  normalizers: [
    createNodeNormalizer('TableRow', () => {
      // Revise children count of TableCellNode
    }),
  ]
})

Normalizers enforce document constraints to guarantee EditorState consistency, they can be seen as runtime rules with a fixer, (I named them nodeNormalizer because they more naturally within the Lexical API). They sit in between Nodes and Plugins, they do not need a plugin to run but at the same time they are not necessarily a 1:1 map with the Node (a TextNode is likely to be used in combination for many). Normalizers for custom product rules are reasonable, for example, prevent the use of HashtagNode within HeaderNode.

Normalizers are $ functions, similar to transforms, that can look at any part of tree and make the appropriate changes. They do, however, come with special rules:

  1. They warn if they do any changes to the EditorState. While we know that clipboard input may be out of our control, this can be useful to catch bad plugins.
  2. You choose when to run them. Complex transforms can be expensive if run too often, by default Normalizers will run on the immediate update and after transforms. We understand that most of these inconsistencies are originated from clipboard and collab, the external sources.
  3. Normalizers can run before any transforms and as often as after every transform. This option is provided to ensure consistency in the transforms logic, which can lead to crashes, but may not efficient when we consider that transforms already loop based on dirty Nodes.
  4. Normalizers never run on the bootstrap update nor in non-editable mode.

Normalizers replace the Node static transform, are optional, and can be lazy loaded in a similar fashion as described above.

Revised EditorConfig

Going back to "Nodes should yield the responsibility for" in the Node section, the only point we didn't cover yet is import/exportDOM.

These will be imported on createEditor, just like normalizers. This reinforces the modularity aspect of Lexical and makes it possible to have a much lighter version of a plain text editor. #6020

@lexical/rich-text is a decent fit, especially when plain text does not do HTML, but 1) it would introduce a dependency to a very clean module that works very closely together with the core and 2) I'm convinced that sooner or later @lexical/plain-text will also understand some HTML (which would unblock the copy-paste of mentions internally)._

createEditor({
  nodes: [
    TableNode,
    TableRow,
    TableCell,
  ],
  html: [
    ...tableHtml,
  ],
  normalizers: [
    ...tableNormalizers,
  ]
})

Or via the builder

createEditorBuilder({
  onError: ...,
  ...
})
  .addDependencies(TablePluginDependencies)
  .build();

Note that there is no 1:1 mapping between HTML and Node, but rather an HTML element can map to different Nodes, the first example reinforces this. The second is merely for convenience so that package owners can bundle all their logic together.

What about #6025

Yes, #6025 would need to be rewritten.

acywatson commented 6 months ago

Nice write up. Some initial thoughts:

Since Lexical inception we have avoided having multiple routes to solving the same goal. There is often a better way and this can easily lead to inefficiences.

This is just an opinion, but I don't think your examples really prove that there are significant inefficiencies here. I also think you discount the advantages of having multiple routes to the same goal - usually the other routes exist because ergonomics dictate that they should.

To create an S3 bucket, you can use the CLI, SDK, or Console. You can use S3 directly, you can use CloudFormation, or you can use the CDK to generate a CloudFormation stack.

All of these make sense for different people with different use cases.

I actually don't think this tenet has as much bearing on your argument/proposal as you suggest, but this is something that I'd push back on as a matter of principle.

Static transforms. We don't want users to build a full-fledged plugin inside the Node itself. It is not feasible to build most plugins this way, for example, plugins that have UI attached like CharacterLimit or plugins that depend on multiple nodes like Hashtag. This wildcard transform function inside the Node causes unnecessary overhead for developers to find where the code lives. We can improve the UX with a simple tweak (see Normalizer section).

I agree with your conclusion, I think, but how you got there doesn't make much sense to me. I don't understand the connection between the static transform API and "a full-fledged plugin", or even why a full-fledged plugin would be bad. The intention of this API was never to allow building plugin-like behavior on nodes (and AFAICT, it hasn't?). It's a simple ergonomic tweak - you literally have to pass a node in when you register a transform. So putting the transform on the node is just a declarative way of achieving the same thing.

Nodes have to be loaded before we can render the EditorState but a method like remove doesn't play any role in the bootstrap.

this is a good point. messy to implement. My only concern would be whether the juice is worth the squeeze.

These will be imported on createEditor, just like normalizers. This reinforces the modularity aspect of Lexical and makes it possible to have a much lighter version of a plain text editor. https://github.com/facebook/lexical/pull/6020

This API already exists, right? #4254

GermanJablo commented 6 months ago

In addition to the points mentioned, I want to suggest a couple more that I think follow the same purpose of this RFC. In this last month, I have been working on a spreadsheet library. I've gotten a lot of ideas from Lexical and the API is in many ways similar. However, I have made 2 decisions that I believe are in line with the reconsiderations of responsibility that are being made here:

  1. Selection and the rest of the state are independent: The Editor class (grid in my case) might not have any knowledge of the Selection class. In some environments such as headless editor, selection is not necessary, and as with the editing methods of a node, many unnecessary lines of code are used. I think this decoupling can be achieved without breaking changes, since the selection is accessed with $getSelection() instead of editor._selection.
  2. You don't install a package to use the library in headless mode. On the contrary, you install a package to be able to use it in the DOM. I always found it strange that Lexical does the other way around. I see that one of the goals of this RFC is to minimize the bundle size and responsibility of the core, and I think making sure that the core works without the DOM could be another strategy to optimize this.
acywatson commented 6 months ago

You don't install a package to use the library in headless mode. On the contrary, you install a package to be able to use it in the DOM. I always found it strange that Lexical does the other way around. I see that one of the goals of this RFC is to minimize the bundle size and responsibility of the core, and I think making sure that the core works without the DOM could be another strategy to optimize this.

This is a good insight

etrepum commented 6 months ago

My personal preference for this kind of thing is to allow "power users" the ability to achieve their goals but in all other cases convenience/usability "in the small" should be the preferred mode. I think in most cases people are not trying to implement every node twice, or in multiple pieces, to support readonly and editable nodes in order to optimize runtime overhead. Should it be possible? Sure. Should the developer have to manage it at the LexicalNode subclass layer? Probably not. If others agreed this was the approach to take I would also recommend other changes to the API, mostly removing "obvious required defaults" like namespace and onError in LexicalComposer, ErrorBoundary and ContentEditable in RichTextPlugin, or shouldBootstrap in LexicalCollaborationPlugin.

I think at this point we have the capability to present a better model. I personally prefer to keep code together unless there's a really good reason to split it up, and in most cases people have one editable use case and those transforms/schemas/normalizations/whatever are all very intrinsically linked to the node so it feels awkward to be required to have to implement some other thing for that node to function correctly (especially in a react-less use case). With this angle I think it would make sense for a Node to have a way to register itself with the editor to provide whatever necessary listeners/transforms/importers/etc. it has. Maybe this can be split up in to some sort of handshake so that the DOM or editable stuff can be split into a separate async loaded chunk if the editor does not request it.

The simplest way as-is is to just say to power users, well, if you want this complicated thing so badly, why not just register different nodes for the same type across read-only and editable use cases. Everything is already ready for that because the type of the node is not associated with the identity of its constructor (and even if it was, there's the node replacement escape hatch). The editable version of the node can subclass the read-only version of the node to inherit the functionality without burdening the read-only use case with the editable code.

Another solution of course is to not care about any of this and do some form of SSR such that only the output of the Lexical code is running on the client in a read-only use case.

My only concern about moving HTML serialization to editorConfig, at least in a separate way from the array of nodes to register, is adding more boilerplate for the users to write or to make mistakes if it's not computed. I'm all for having a way to do overrides, I just think the boilerplate for obvious behavior should be nonexistent whenever possible.

etrepum commented 6 months ago

Maybe a lot of these things could be solved if the Node was able to vend "static async" methods for configuring these things that could load the required code on demand (for bundle size optimization use cases)? And the editorConfig would have something like node replacers where you could use a node but provide your own versions of those static async methods (which could of course easily call out to the original implementations if you're extending rather than completely overriding)

abelsj60 commented 6 months ago

Hi Gerard. Long time. I'm out of date and behind the times, but since you pinged, I'll try to offer a few comments.

While I tend to agree with Acy on this — strongly in fact — I think you're a clever guy who steeps himself in his projects.

For instance:

You were the guy who looked at the issue at some point and realized that Lexical needed a dedicated, first-class TabNode, then spent a lot of time getting it to work right because it turned out to be quite a lot more complicated than it seemed.

Given that, I'm sure you're responding to a lot of real world intuitions from day-to-day work on the project.

(Yeah, I'm an odd duck, I think intuition is generally more valuable than "data." All day long. /shrug)

I'm not sure neutering Lexical's flexibility is the way to go about addressing that, though.

Maybe there's another way to explain and address the issues you're sensing in your work? Personally, I'm reading the bundle size concerns more as an attempt to make a persuasive argument than a big current concern that keeps you up at night.

Generally:

The Lexical Way

Some of what you say here makes a bit of a tangential point that may be relevant. For instance:

Nodes used to represent the data (we intentionally excluded any reference to editor) and wrap simple configuration that either events and reconciler needed to know, but now they are responsible for too much to the point that you can build a full-fledged plugin in the Node itself.

Is this something new users — or even advanced ones (if advanced means knowing a lot of the API) — understand? I know there have been attempts to improve the docs, but I wonder if people just don't get "the Lexical way."

Generally, people have a paying job to do and reach for whatever works fastest.

That Lexical can offer so many ways to help them is a great strength.

But, maybe you could help them find what works faster if you could better communicate the Lexical ways to them.

I don't mean the API — I mean how to think about Lex, which is really a one-for-one DOM swap.

RSC and SSR

Schemas OR non-linear node assembly

The last time I commented on the schema issue, my concern was Lexical objects that are not built in a linear fashion — stuff with children and grandchildren. It's hard to do.

I did it with LinedCodeNode. You have access to it in Lexical 401.

The code won't impress you, but it used to work and make the point. Perhaps you at FB deal with broken editor states a lot, so fixing them with a schema is important.

But, I struggled most with Lexical in the context of a node that oversaw multiple other nodes.

(I did a lot of that. I have things called LexicalInput, LexicalSelect, and LexicalCommandBar, for instance.)

I remember the table and experimental table were similarly complicated to understand in terms of createDom, updateDom, and the import/exports. I don't remember the ins-and-outs today, but one issue was you could create/update and import/export from the top level node or a child and there was no guidance beyond Dominic's use of the top-level in his tables.

Perhaps if you looked at this type of issue — including the tables — and considered an easier method of handling it all, you would get some ideas for changing the API in a way that would address your underlying concerns here. These strike me as among the most complicated uses of Lex, so making them easy may make a lot of things easy.

(You could even build an official LinedCodeNode, I remember Dom didn't love the current CodeNode. I'd agree.)

-- That's all I've got. Probably what I've said before... I'm behind, so maybe you, Acy, and co have already addressed all this, but I'd guess there's something under the surface here that's bothering you.

I'm not sure I like the idea of "simplifying" it the way described, and I tend to think Dominic was right to try to keep data flowing one way with plugins. But I can't restate why the way he did. Maybe that's the problem here — my inability to articulate this type of thing demonstrates why people aren't following the Lexical way as well as they could?

It's not the API they struggle with so much as understanding how to put it all together.

Generally, I think Lex is a great project, particularly since it exists to literally react to language. Given that AI is now language driven, and clearly going to swallow most of our machine interactions, what could be more important?

zurfyx commented 6 months ago

Selection and the rest of the state are independent: The Editor class (grid in my case) might not have any knowledge of the Selection class. In some environments such as headless editor, selection is not necessary @GermanJablo

I think this is reasonable, but with RangeSelection it will be hard to make it an independently module. Functions like remove or replace recompute the selection in arbitrary places.

Second, I think it'd make sense to pair this with some new listeners, registerSelectionListener and registerEditorStateListener, to convey the fact that these are separate concepts. The History plugin would also need to be updated accordingly.

This one deserves its own issue with a proper API and trade-offs evaluation.

I remember the table and experimental table were similarly complicated to understand in terms of createDom, updateDom, and the import/exports. @abelsj60

That's true, @GermanJablo did some good work on the API simplification but these methods you pointed out have never been revisited, updateDOM is particularly hard to understand.

convenience/usability "in the small" should be the preferred mode. I think in most cases people are not trying to implement every node twice, or in multiple pieces, to support readonly and editable nodes in order to optimize runtime overhead. Should it be possible? Sure. Should the developer have to manage it at the LexicalNode subclass layer? Probably not. If others agreed this was the approach to take I would also recommend other changes to the API, mostly removing "obvious required defaults" like namespace and onError in LexicalComposer, ErrorBoundary and ContentEditable in RichTextPlugin, or shouldBootstrap in LexicalCollaborationPlugin. @etrepum

I support the simplicity aspect for new users (and so does @abelsj60 based on the coment above) but I would be keen to explore options that didn't sacrifize power users. We can add code but we can't remove it, the last code example on the builder I provided shows a potential way to achieve so, by providing a very flexible lower level layer but promoting the abstraction which is easier to set up.

One of the conflicts we have faced between Meta, Bloomberg and other open-source folks utilizing Lexical is the number of features supported in TextNode importDOM. For example, at Meta, we don't use colors, whereas other companies would like this feature supported. Overriding the HTML, the API @acywatson created, unblocks this issue but the 200 LoC in importDOM still remain there.

At the same time, we have always supported modularity on Lexical. Hence, why we have 24 packages under @lexical, but we can provide bundles so that it's easier to spin up a rich text editor. In fact, we do this internally already, where our simplest API looks like <XYZEditor value={initialValue} onChange={callback} label={a11yLabel} toolName={forLoggingPurposes} />.

Maybe a lot of these things could be solved if the Node was able to vend "static async" methods for configuring these things that could load the required code on demand (for bundle size optimization use cases)? @etrepum

I'd be keen to read some code example on this idea and the potential implications on the bundling.

fantactuka commented 6 months ago

Normalizers never run on the bootstrap update nor in non-editable mode.

Agree with non-editable mode, but wonder if normalizers on bootstrap would be useful when new normalization rules are introduced or content was corrupted before normalizers were introduced. Although it should be easy to resolve with some sort of plugin that force-updates on bootstrap that would trigger normalizers

Read-only nodes

At some point we had an idea of (internal) gencode that would generate nodes for headless mode (running on HaaS) vs regular ones. We wanted it for different reasons, but separate read/write nodes could be a way to trim down bundle for read-only mode in case it's an actual problem for a project. I might be wrong about users' expectations, but if I had simple rich text editor (no fancy decorators, except maybe images), I'd traverse editor state to output HTML/react/whatever vs running editor instance.

Import/export DOM

An idea of co-locating it with node definition felt right at the time, although its flexibility seems to be a common problem and we tried to handle it with new concepts (IIRC, node overrides were mostly introduced to help with it) and patches, and it's still not great. With a wild west of markup that is being pasted into the editor in addition to custom nodes support and all sorts of formatting it's almost impossible to handle it all out of the box. Moving it back to editor config that might have some defaults (but the ones you can easily override) looks reasonable.

Must admit that exportDOM felt as a very natural part of the node (unlike import that had to include certain complexity to avoid conflicts with other nodes, e.g. priority), but for the sake of keeping things together it should be co-located with import

zurfyx commented 6 months ago

transforms/schemas/normalizations/whatever are all very intrinsically linked to the node @etrepum

Yes, but the ownership model is different. Methods inside the Class are created by the owner of the class, document rules do not necessarily have to adhere to this. Let's say that you want to ban the use of token TextNode inside HeadingNode, as a product owner you neither own TextNode nor HeadingNode. The options that you have are either override the Node (which is fairly convoluted) or a transform somewhere. Overriding the Node would work for this case, but it gets messy when multiple rules require overriding the nodes, particularly if one of these is part of a package you don't control.


An alternative option to keep the simplicity aspect you mentioned earlier is to expand the transforms options to fulfill normalization and promote the use of transforms immediately after editor creation, which we currently are not in LexicalComposer.

I see 4 major requirements of normalizers:

  1. They need to be registered immediately after editor creation. The first user update should already run through them all. In VanillaJS, plugins meet this expectation but the API for transforms on plugins have the implication they can be deregistered.
  2. Built-in and custom rules. Users may want to specify their own custom document rules.
  3. Simplicity: you can import the entire package dependencies with an import, just like you currently can for a Node.
  4. Optional. Normalizers attempt to fix broken document rules, in a perfect world normalizers wouldn't need to exist.

The revised EditorConfig would still retain the same API but behind the scenes, it would equal to:

createEditor({
  onError: ...,
  nodes: [
    TableNode,
    TableRowNode,
    TableCellNode,
  ],
  transforms: [
    $rowCellsNormalizer,
  ]
});

/// equivalent
createEditorBuilder({
  onError: ...,
  ...
})
  .addDependencies(TablePluginDependencies)
  .build();
GermanJablo commented 6 months ago

I think this is reasonable, but with RangeSelection it will be hard to make it an independently module. Functions like remove or replace recompute the selection in arbitrary places. @zurfyx

I think the heuristics with selection happen when trying to delete nodes. An error might be thrown that says something like: "You cannot delete nodes in an environment without selection."


I think this RFC is very broad and continues to branch. It's hard for me to follow the thread and I think there may be interesting points left behind. I suggest that it be divided into the following:

A. Discussions that have to do with decoupling parts of Lexical to improve bundle size or performance in certain scenarios:

  1. separate read and write properties of a node.
  2. centralize HTML serialization and perhaps createDOM outside of nodes
  3. no headless package, DOM as an extra module.
  4. decouple the selection from the editor.

B. Discussions that do NOT have to do with decoupling parts of Lexical to improve bundle size or performance in certain scenarios:

  1. RegisterPlugin
  2. Normalizers

If there is support, we can open these issues and copy the relevant information from this thread. If there is any idea or proposal that I have overlooked, please tell me.


Regarding the points that are in category A, I think their viability will depend on the complexity they imply. For example, if (A.1) involves having a class ParagrahNode_read and ParagraphNode_write, I don't know if the cost outweighs the benefit.