Decoupling plugin and nodes

[fantactuka]

Problem

Plugins and nodes are tightly coupled and it makes it harder to extend default nodes and maintaining existing plugins functionality.

Example

Let's take markdown plugin (copy/paste logic is also a good example), it exports bunch of node creation helpers ($createHeadingNode, $createCodeBlockNode, $createListNode, etc).

Then if I want to extend HeadingNode behaviour, for example to add id attribute to each heading so it can be used as an anchor (smth like <h1 id="getting-started-with-react">Getting started with React</h1>, see our README doing it). To do so I'd extend default node and add new behaviour on top:

class AnchorHeadingNode extends HeadingNode {
  static getType() {
    return 'heading';
  }

  createDOM<EditorContext>(config: EditorConfig<EditorContext>): HTMLElement {
    const element = super.createDOM(config);
    element.setAttribute('id', getAnchorID(this));
    return element;
  }

  updateDOM(prevNode: HeadingNode, dom: HTMLElement): boolean {
    dom.setAttribute('id', getAnchorID(this));
    return false;
  }
}

But now the problem is that all plugins that might insert HeadingNode (markdown, toolbar, copy/paste), they all keep using default HeadingNode, because they all use $createHeadingNode imported from default heading node file.

Potential solution:

Replace $createHeadingNode() implementation from

function $createHeadingNode(tag): HeadingNode {
  return new HeadingNode(tag);
}

to

function $createHeadingNode(tag): HeadingNode {
  const NodeConstructor = $getNodeFromRegistry(HeadingNode);
  return new NodeConstructor(tag);
}

...
// and smth like:
function $getNodeFromRegistry<T>(nodeKlass: T): T {
  return $getActiveEditor()._registeredNodes.get(nodeKlass.getType());
}

This will allow passing AnchorHeadingNode into LexicalComposer, and since it has the same type ('heading'), it'll be used whenever $createHeadingNode is used, so all callsites that create heading will use our extended heading node.

[trueadm]

I think that HeadingNode should have __anchored: boolean property. Headings are important is that they're semantically an anchor point, rather than other types of content. If this value is true, the id should be auto generated given the header's text IMO.

[fantactuka]

That's fair for headings, but the point remains: there's no way to extend default nodes behaviour and keep all plugins working with extended nodes instead of default ones. We can make similar case with links or mentions, when for certain surfaces we'll need to add data attribute on their dom nodes to support hover cards. Or if we want to add title="<current highlight language>" to code block. The only way to make it possible now is monkey patching default nodes:

import {LinkNode} from 'lexical/LinkNode';
const createDOM = LinkNode.prototype.createDOM;
LinkNode.prototype.createDOM = function() {
  const dom = createDOM.apply(this, arguments);
  dom.setAttribute('data-hovercard', this.getURL());
  return dom;
}

Then whenever we insert link it'll have required data attribute, but it does not look pleasing

[acywatson]

It sort of makes sense to me that the creators should return an instance of whatever class is registered to a particular type string instead of just being coupled to a specific class, but I'm struggling with the semantics a bit. Would we leave the creators in the Node modules? This would mean that if I extend TextNode and override getType to return "heading", then $createHeadingNode would return something that doesn't inherit from Heading. Where would we put the creators instead and how would we communicate their functionality to users?

Edit: I guess how we would communicate the functionality is a documentation problem, I was thinking more about "how do we make the behavior clear and intuitive?"

[fantactuka]

Edit: I guess how we would communicate the functionality is a documentation problem, I was thinking more about "how do we make the behavior clear and intuitive?"

Yea, creator from node file that might return its subclass instance is not intuitive at all. Plus proper typing can be challenging. I was thinking that if we override existing node (when passing into <LexicalComposer nodes=[...]>), then it has to be a sub class of existing node of same type (so I can't pass any random node class as an override of heading). Wonder if it'll help with typing

[trueadm]

That's fair for headings, but the point remains: there's no way to extend default nodes behaviour and keep all plugins working with extended nodes instead of default ones.

I don't think we want plugins to work with extended nodes automatically. That's magic and it will cause more bugs than it will solve. You can always opt to tell a plugin what nodes to use <FooPlugin heading={HeadingNode} />. Keeping the contract explicit ensures tight constraints, which is a good thing IMO.

[fantactuka]

use <FooPlugin heading={HeadingNode} />. Keeping the contract explicit ensures tight constraints, which is a good thing IMO.

Should it be then default pattern for plugins to accept $createXYZNode and $isXYZNode as a param to allow extensions instead of importing it from XYZNode file?

[trueadm]

Only if they need to create nodes that might regularly being extended. Another option is to create a localized node store to make this easier. store.createNode('paragraph'), but this doesn't need to live in core.

[wenerme]

I don't think we want plugins to work with extended nodes automatically. That's magic and it will cause more bugs than it will solve.

This is how tiptap Extension works, extend exists Mark & Node, add extra attributes.

<FooPlugin heading={HeadingNode} />

This more error prone, you assume all plugins expose every node they use, when there is one more node use, the plugin become wired, it's impossible to maintain this.

[GermanJablo]

I've been trying to prototype my project with lexical for a few weeks, and this is undoubtedly the biggest difficulty I encounter.

If I want to add a property to some node, for example to a paragraph, I have to create a "customParagraph" and then create my own enter command (and probably other keys) that override the lexical-rich-text behavior. So lexical-rich-text becomes of little use.

I also come from tiptap, and this is much more comfortable there.

Is it possible to rethink this approach? @trueadm

[trueadm]

Actually, I've been thinking about this topic a bit lately. One pattern to have emerged, after this thread (as this is actually quite an old thread now given the project's momentum in the last few months), is that maybe we should be leveraging commands more. Just to be clear too – the correct way of extending a node is not to hack something somewhere, it's to extend the class you want to extend. This approach is type safe and also using the language by design.

Right now, people often just do $createParagraphNode directly, but maybe we should instead be educating them to dispatch an CREATE_PARAGRAPH_COMMAND. Then if someone wants to insert a custom paragraph node, it's trivial to listen to the command at a higher priority and do that. We'd probably only need to do this fore all the core nodes, and nodes that are in Lexical's own packages – custom nodes likely wouldn't need to take this approach.

This would address all the issues that have been raised in this thread that I can see, without breaking the type safety we depend on (TypeScript/Flow strictness) and the design ethos that Lexical has (which is heavily inspired by React).

[wenerme]

This approach is type safe and also using the language by design.

tiptap way to add extra attribute is also type safe

$createParagraphNode directly, but maybe we should instead be educating them to dispatch an CREATE_PARAGRAPH_COMMAND

replace command and extend attributes is different requirement, more like imperative vs declarative, a built-in command can be replace/chained, that's good, but if add support to replace command is just for change the initial node that $createParagraphNode provided, maybe should reconsider this.

[trueadm]

tiptap way to add extra attribute is also type safe

I think we have different definitions on the soundness of type safety. What TipTap does would not work with Flow in strict mode. It can work with TypeScript, but that's because TypeScript is more permissive here.

replace command and extend attributes is different requirement, more like imperative vs declarative, a built-in command can be replace/chained, that's good, but if add support to replace command is just for change the initial node that $createParagraphNode provided, maybe should reconsider this.

I think we are looking at different ideas internally around this. The concept of building from a node factory, that has a similar design to commands might be one direction though.

I mean, the idea around the command was just the concept. I think we'd have a verbose API for it that wouldn't be that of a command.

[trueadm]

Some ideas we've circled around internally:

// New API
editor.registerNodeFactory(ParagraphNode, ({x, y, z}) => 
  new CustomParagraphNode(x, y, z);
, LOW_PRIORITY)

const paragraphNode = $createNodeFromFactory(ParagraphNode, {x, y, z})

// Using commands
editor.registerCommand(CREATE_PARAGRAPH_COMMAND, createNodeFactory(({x, y, z}) =>
  new CustomParagraphNode(x, y, z);
)), LOW_PRIORITY)

const paragraphNode = editor.dispatchCommand(CREATE_PARAGRAPH_COMMAND, {x, y, z})

An alternative to using commands, without breaking the existing API might be to introduce some way of capturing a value using closures:

editor.registerCommand(CREATE_PARAGRAPH_COMMAND, (createNode) =>
  createNode(({x, y, z}) =>
    new CustomParagraphNode(x, y, z);
  );
  return true;
), LOW_PRIORITY)

let paragraphNode = null;
editor.dispatchCommand(CREATE_PARAGRAPH_COMMAND, (createNode) => {
  paragraphNode = createNode({x, y, z});
})

That looks quite boilerplate-y though, you could extract it into a helper:

function $createNodeFromCommand<T: LexicalNode, P>(
  COMMAND: LexicalCommand<P>,
  payload: P,
): T {
  let node = null;
  editor.dispatchCommand(COMMAND, (createNode) => {
    if (node !== null) {
      throw new Error('Node was already created')
    }
    node = createNode(payload);
  })
  if (node === null) {
    throw new Error('Node was not created')
  }
  return node;
}

const paragraphNode = $createNodeFromCommand(CREATE_PARAGRAPH_COMMAND, {x, y, z});

We're still working out what works and what doesn't. :)

[GermanJablo]

Let's see if I'm understanding correctly.

Your proposal would be to add an API to the lexical code that allows you to prefix the create paragraph command with a create customparagraph command?

Would it be the idea to do the same for all other types of nodes? ordered list, unordered list, checklist, headers, quotes, code, etc.?

And if this is so... once that is done, all the helper functions of those nodes such as $isLastItemInList or commands like REMOVE_LIST_COMMAND would continue to work in the custom node as it is an extension of the class. I am right?

[trueadm]

Basically, all $create* functions would route through the command system using a respective command. CREATE_X_NODE, allowing for plugins to define a different node factory for a given command. There'd be one for paragraph, heading, quotes, code, lists, etc. The $is functions should already work as expected, as they are instanceof checks, as the custom nodes should always extend the base node anyway. No other changes are needed.

[GermanJablo]

I've been thinking about this a lot, and the conclusion I've come to is that the different ElementNode types shouldn't be different classes, but should be handled with the __type property.

Before I get to the reasons, I want to talk about shared properties. I refer to them as the properties that different element nodes share in common and affect the appearance of the node. In Lexical currently this would be indent. This property has presented some challenges that have been resolved in a somewhat strange way in my opinion. However, while indent is enough of a property to make an argument for what I'm going to say next, I want to note that this isn't the only shared property that someone might want to incorporate into an editor. Notice for example how applications like Notion, Dynalist, Remnote, Roam Research, and Workflowy, have properties like color (at element level), collapse, hide, shared, __backlinks, etc. The possibilities are many.

So, some reasons why the different types of ElementNode should not be different classes, but should be handled with the __type property:

1. When converting one elementNode to another, the shared properties are lost (for example, convert an indented paragraph to a heading and the indentation will be lost). The reason for this is that since they are different classes, a new instance is required. Of course, by changing the type of a node one could also copy the desired properties to the new node. EDIT: It seems this has been fixed only for the indent case, and not for arbitrary user-defined properties.

2. Even if some properties are used only by some nodes, it would be ideal if the other nodes keep that information. The reason for this is that users often undo operations using the reverse operation instead of ctrl + z. For example, ListItem currently ignores the __indent property because it handles it with nesting. But if I have indented paragraphs, I would like the indents to be preserved when I convert them to a list and return them to paragraphs. Or if I have a checklist checked, convert it to something else, and return to the checklist, the "completed" property should be preserved. I recommend seeing this notion blog article where they talk about this.

3. If the different types of elementNodes are subclassed and we have a shared property, then the same display logic must be defined in the updateDOM and createDOM of each node, which makes it very difficult to maintain the code. In Lexical right now there is a workaround with the __indent case that is handled in Reconcilier.ts. Besides being a bit confusing, the rest of us can't do the same for other shared properties we might want to add.

4. Lexical is actually already doing what I'm proposing (in part). I mean HeadingNode. If H1-H6 are handled in the same class, I don't see any reason not to use paragraphs also in the same class, for example.

5. When converting one type of elementNode to another, the key changes. If someone wanted to implement links to the paragraph, this would result in very brittle links. All of the apps I mentioned above and others like Onenote allow element-level links.

6. This is perhaps more personal, but I think that a functional API instead of a class-based API like the one I propose below would offer a much friendlier development experience. Even element nodes could be defined as React components (as slate does for example).

My proposal then is for an api that allows registering

• new elementNodes properties (indent, color, etc.)
• new values ​​for the __type property, with their respective rendering displays.

EDIT: I could help with this.

EDIT 2:

What I write below I do not consider as essential or necessary as the above. I thought to open it in a new thread but it's quite related.

Looking a bit at the SlateJS API I mentioned in point 6 (do we agree that it's beautiful?), I think another improvement that could be made to ElementNode is that instead of forcing that 1 ElementNode = 1 HTML tag node, the following more flexible conditions could be imposed:

1. An ElementNode should always have a top-level element.
2. An ElementNode should not have text nodes (obviously I mean the DOM inherent to the node, not its child nodes).

The first condition would be to make deserialization much easier. The second condition would be to make selection much easier. Right now, if someone wants to implement a node that isn't 1-to-1 related to an HTML tag, their only option is to make a decoratorNode (which isn't always desirable), or to create a bunch of nodes in a very complex way ( as happened to @gutenye here (see the codesandbox) and is happening to me in other use cases).

About DecoratorNode and TextNode All my comment so far is about ElementNode, because that is where the improvements that this change would bring are most evident. However, TextNode and DecoratorNode are also designed by class extension, and could well be changed to a master class that could have methods extended. At the moment, I can see that making the change with them could unify the entire library in a coherent architecture and a more friendly API (everything that needs to be defined to give a certain style to a text, for example, seems a bit excessive to me). However, I still haven't gotten around to looking in depth at what other pros and cons would be to making the same change on these 2 nodes. I'm still investigating, I'll keep this thread updated with my conclusions.

[echarles]

@EgonBolton I am still discovering lexical as this thread. A lot of considerations you raised in you last comment resonates to me and I wonder how possible it would be to evolve the API in a non-breaking way. I guess proposing a refactoring that makes the existing plugins and deployments (as I understand Facebook already uses Lexical for some of their products) will not be easy and may be pushed-back. Do you see a roadmap to evolve the API without breaking the existing?

[GermanJablo]

@EgonBolton I am still discovering lexical as this thread. A lot of considerations you raised in you last comment resonates to me and I wonder how possible it would be to evolve the API in a non-breaking way. I guess proposing a refactoring that makes the existing plugins and deployments (as I understand Facebook already uses Lexical for some of their products) will not be easy and may be pushed-back. Do you see a roadmap to evolve the API without breaking the existing?

Lexical has a versioning system for nodes that is explained in the documentation. However, as far as I understand, the best that can be achieved with this is to keep backwards compatibility for a period of time to release all the breaking changes together. There have already been some breaking changes in 3.0.0 and a pretty big one is expected soon. In the comments for the latter you can see that the intention is to keep it simultaneously with the old model for some time. If this change I'm proposing pleases the Lexical team, I think it would make sense to release it along with this other change.

Going back to your question, I suppose that if any company (besides Meta) is already using Lexical in production, sooner or later they should make a script to fix the nodes in their database.

[acywatson]

@EgonBolton Thanks for your input and the time you've take to explore and update. I haven't had the time yet to thoroughly investigate your proposal, but we've started a discussion internally around the direction we want to take moving forward. As I have more time to think about this and our point-of-view evolves as a team, we'll update here.

I wonder how possible it would be to evolve the API in a non-breaking way

This is not something that I think we should be overly concerned about. For the community, this is why versioning exists. Internally, if a change in design of this magnitude is what we feel is the right thing to do, we will deal with breaking changes. As @EgonBolton pointed out, we already have a solution place to insulate ourselves and others against such an eventuality.

Generally speaking, I'll say that we have gotten good feedback with respect to our API and the associated developer experience both internally and externally. We want to take the time to consider what people like about what we're doing and how that trades off against any major proposed changes. At the same time, we're certainly aware that there are use cases that are not as refined as we'd like them to be and there is work to do to get them right. To that end, your constructive input, ideas, and willingness to help are greatly, greatly, appreciated.

[GermanJablo]

I haven't had the time yet to thoroughly investigate your proposal, but we've started a discussion internally around the direction we want to take moving forward. As I have more time to think about this and our point-of-view evolves as a team, we'll update here.

Awesome. I look forward to hearing more about this.

if a change in design of this magnitude is what we feel is the right thing to do, we will deal with breaking changes.

Happy to hear that!

Generally speaking, I'll say that we have gotten good feedback with respect to our API and the associated developer experience both internally and externally. We want to take the time to consider what people like about what we're doing and how that trades off against any major proposed changes. At the same time, we're certainly aware that there are use cases that are not as refined as we'd like them to be and there is work to do to get them right. To that end, your constructive input, ideas, and willingness to help are greatly, greatly, appreciated.

Of course, to everything I said previously I want to add that I too am very grateful to have this amazing tool. I just hope I can contribute to making it even better than it already is.

[YousefED]

Thanks @EgonBolton for your extensive write-up. I've created BlockNote to bring a structured block-based editor to the Prosemirror ecosystem and recognize a lot of your painpoints. Figured it might help to share my experience.

To work around PM's limitations I created a single "block" element type with attributes specifying the "style" (heading / listitem / paragraph etc.). This is a bit hacky, as normally you'd use the node type / element type for this. The largest disadvantage is that it will break compatibility with many community plugins in the ecosystem.

I'm investigating Lexical to see if I can support both frameworks. Its API design looks really clean. However, for a block-based editor it seems to suffer from the same challenges:

• Properties are lost when changing the Type (your items 1, 2)
• The Key changes when changing the Type (your item # 5)

It might be possible to hack around these points with #2924, but it doesn't seem ideal. Happy to discuss this further

[echarles]

Thx for sharing toughs @YousefED and congrats for BlockNote. Just wanted to confirm I have followed similar road and came to similar conclusions. Node proxies may not bring solutions to current limitations. I would favor rather building the needed foundations in the core, to support items 1, 2, 5...

[johny-ss]

I also don't think https://github.com/facebook/lexical/pull/2924 solves the issues being discussed here (@zurfyx). What happens if I want to add the same property to several nodes at the same time?

What TipTap does would not work with Flow in strict mode. It can work with TypeScript, but that's because TypeScript is more permissive here.

@trueadm If Flow is an impediment here, may I ask what is the point of using it? Is it a Meta requirement?

[acywatson]

Really glad to see the discussion revived/continuing here. I view figuring this out as one of the big things we want to solve before moving to v1.

@trueadm If Flow is an impediment here, may I ask what is the point of using it? Is it a Meta requirement?

Just to answer this one point - we do use Flow internally, but Dominic is referring to the fact that Flow generally prioritizes type soundness/safety, whereas TypeScript is less "strict" by design and prioritizes balancing verifiable correctness with productivity. So the difference is really about how soundly we're able to type this design, but I'd have to PoC it to see if there are actually soundness gaps in whatever solution we end up pursuing.

[GermanJablo]

Thank you @YousefED and @echarles for sharing your experience. And congratulations Yousef for BlockNote.

Grouping some nodes in the same class (as is done right now with the headings H1-H6) would solve the problem that properties are lost when converting from one type of node to another (including the key).

I have done a proof of concept by unifying ParagraphNode and HeadingNode into a single node: BlockNode. The createDOM function is a switch that renders one way or another depending on the type, which can be changed with the setType method.

Before (keys change): https://www.loom.com/share/21ccb9ca61d04dcc89b6eb9bcf613e22

After: https://www.loom.com/share/08d29e50e9b34f68becab376f4dd1835

The idea would be to bundle ListItemNode and CodeHighlightNode as well, but then there would be dependencies between the Rich-Text, Code, and List packages. Maybe someone can come up with a design pattern that allows the class to be extended externally? (obviously, I mean without using js native class extension).

Of course, there is still the option to clone all properties. But in addition to the fact that it seems like unnecessary work, the dependency between the different packages would still be implicit, since, for example, the Checked property would have to be added to paragraph.

[YousefED]

Thanks @EgonBolton

Grouping some nodes in the same class (as is done right now with the headings H1-H6) would solve the problem that properties are lost when converting from one type of node to another (including the key).

Your solution of changing all nodes to the type of "BlockNode" is definitely possible, but feels a bit hacky at the same time. The architecture could work, but you'd also lose some of the benefits of having different node types in the first place. One of them is that indeed, you'd have to reimplement all different node types.

If you're interested, I brainstormed some pros and cons if this approach as well and documented this here: https://github.com/YousefED/BlockNote/pull/45.

I'm also fairly sure the Lexical team is also aware of different challenges around node extensibility and are researching what the ideal approach should look like!

[trueadm]

I feel like we can close this now that https://github.com/facebook/lexical/pull/3367 has landed. Please reopen if you feel like something is missing.

[DaniGuardiola]

I have a question about node replacement @trueadm. Please excuse me if I missed something in this thread, I couldn't read all of the latest updates in depth.

Would this break some behaviors if, for example, I replace the ParagraphNode and some code is checking by class identity directly? How safe is it and what gotchas would I need to be aware of?

For context, my current use case is that I want to add a "Notion-style" handle that shows up to the left of paragraphs and other nodes. I know there is already an example of this in the playground, but it depends on moving and hiding/showing a single handle on mouseover which unfortunately presents some limitations I want to get around. So my goal is to essentially extend the createDOM function of paragraph nodes (and others like HorizontalRuleNode) to render the handle inside of each instance of the node directly.

Thanks in advance 🙏

[trueadm]

You don’t want to do anything complex in createDOM, especially not event handling. That’s logic that needs to be attached in another plugin via a mutation listener for your node type. You can simply replace the built in paragraph with your custom paragraph block if you need a custom node.

[acywatson]

especially not event handling

Unless I misunderstood, it's not about event handling, but about rendering a "handle" (which I take to mean some visible UI element you can click on) beside each paragraph.

However, I do agree with Dominic that right now you shouldn't be doing anything in createDOM other than just setting classes and and creating a single DOM element.

but it depends on moving and hiding/showing a single handle on mouseover which unfortunately presents some limitations I want to get around

What limitations are these?

How safe is it and what gotchas would I need to be aware of?

We think it's pretty safe. Every places in the code should be using instanceof checks to refine node types, so your Paragraph subclass should pass those checks.

[DaniGuardiola]

Thanks for your answers! @acywatson the main limitation is that if my handle also opens a menu, right now I can't show another handle if the menu is already opened, without a fair amount of complex logic like having two handles and switching between them or some trickery like that. I'm making some progress on it, but I still feel like there must be a simpler way.