search

facebook / lexical

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

Generalize setters/getters, clone, importJSON and exportJSON #3763

Open GermanJablo opened 1 year ago

GermanJablo commented 1 year ago

The Problems

In https://github.com/facebook/lexical/issues/1262#issuecomment-1186170960 I explained a series of problems when it comes to extending nodes.

At that time I thought that the best way to solve it was to group different nodes in the same class (as is done now with the different levels of Heading). In fact later in the same thread, I offered a proof of concept. I still think this could potentially reduce code repetition in very similar nodes like headings and paragraphs, but after thinking about it a lot I came to the conclusion that it is not a viable solution, at least for all the problems that I described.

After that the node replacement API came out. Although it solves some problems, I think the first 3 items of my initial post are still valid:

  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.

Things I've Tried

So once I had the node replacements API available, I decided to ignore issues 1 and 2 for the moment and focus on issue 3. That is, I tried to reduce the excessive code repetition required to add a shared property to multiple nodes at once; and I did it using a combination of the node replacement API and the mixin pattern.

In the process I ran into many difficulties, but I came up with an idea that I think would solve all 3 problems at once.

New idea: how I think extending a node should look like.

As I mentioned in the other thread, note for example how applications like Notion, Dynalist, Remnote, Roam Research and Workflowy have properties such as color shared by what in Lexical we would call nodes of type element.

If a user wanted to do something like this with Lexical, for each of the nodes they would do something like this:

export type SerializedColoredTextNode = Spread<
  {
    color: string;
    type: 'heading';
    version: 1;
  },
  SerializedTextNode
>;

export class ColoredNode extends TextNode {
  __color: string;

  constructor(text: string, color?: string, key?: NodeKey): void {
    super(text, key);
    this.__color = color || "defaultColor";
  }

  static getType(): string {
    return 'colored';
  }

  setColor(color: string) {
    const self = this.getWritable();
    self.__color = color;
  }

  getColor(): string {
    const self = this.getLatest();
    return self.__color;
  }

  static clone(node: ColoredNode): ColoredNode {
    return new ColoredNode(node.__text, node.__color, node.__key);
  }

  createDOM(config: EditorConfig): HTMLElement {
    const element = super.createDOM(config);
    element.style.color = this.__color || "defaultColor";
    return element;
  }

  updateDOM(prevNode: ColoredNode, dom: HTMLElement, config: EditorConfig): boolean {
    const isUpdated = super.updateDOM(prevNode, dom, config);
    if (prevNode.__color !== this.__color) {
      dom.style.color = this.__color;
    }
    return isUpdated;
  }

  static importJSON(serializedNode: SerializedMentionNode): MentionNode {
    const node = new ColoredNode(serializedNode.text, node.serializedNode.color);
    node.setFormat(serializedNode.format);
    node.setDetail(serializedNode.detail);
    node.setMode(serializedNode.mode);
    node.setStyle(serializedNode.style);
    return node;
  }

  exportJSON(): SerializedHeadingNode {
    return {
      ...super.exportJSON(),
      color: this.getColor()
      tag: this.getTag(),
      type: 'colored',
      version: 1,
    };
  }
}

Instead, the solution I have in mind would reduce it to just this:

export class ColoredNode extends TextNode {
    static getType(): string {
    return 'colored';
  }

  createDOM(config: EditorConfig): HTMLElement {
    const element = super.createDOM(config);
    element.style.color = this.getProperty(color) || 'defaultColor';
    return element;
  }

  updateDOM(prevNode: ColoredNode, dom: HTMLElement, config: EditorConfig): boolean {
    const isUpdated = super.updateDOM(prevNode, dom, config);
    if (prevNode.getProperty(color) !== this.getProperty(color) {
      dom.style.color = this.getProperty(color);
    }
    return isUpdated;
  }
}

Note: I think there are ways to simplify this even further, but I'll leave it as a starting point for now.

Implementation

The way to avoid defining properties, getters and setters in each class would be with the following primitives:

class LexicalNode {
  //...
  __properties: object;

  getProperties(): object {
    const self = this.getLatest();
    return self.__properties;
  }

  setProperties(properties: object): object {
    const self = this.getWritable();
    self.__properties = properties;
  }

  getProperty(key: string): any {
    const self = this.getLatest();
    return self.__properties[key];
  }

  setProperty(key: string, value: any): void {
    const self = this.getWritable();
    self.__properties[key] = value;
  }
}

Cloning or serializing a class instance is tricky. But by making the properties a simple object we could do without explicitly defining the clone(), importJSON or exportJSON() methods for every subclass.

Each class would inherit those methods something like:

exportJSON() {
   return {
     //...
     properties: this.getProperties(),
     type: getType(),
   };
}

importJSON(serializedNode) {
   const node = $createXnode();
   //...
   node.setProperties(serializedNode.properties);
   return node;
}

clone(node: XNode): XNode {
   return new XNode(node.__properties, node.__key);
}

Of course, an interface could be defined to the __properties object, to get the typesafety.

The point is that the properties would be decoupled from the nodes. In other words: although li, or paragraph are not going to use the "checked" property, it would still be nice if they did, which is what I explained in points 1 and 2 at the beginning of the post.

Then, keeping any arbitrary properties when converting one node to another would be trivial. You would only have to clone the properties in the $setBlocksType function

This could fix:

echarles commented 1 year ago

Thx for starting this discussion @EgonBolton. I am supportive for the 2 first goal (and probably also the 3rd one which I understand less).

Quick questions:

GermanJablo commented 1 year ago

Thanks @Echarles.

Point 3 is basically about verbosity and repetition. I mean that extending a node for something as simple as adding a property requires a lot of boilerplate, unnecessary in my opinion (example above). If you also want to add that same property to different nodes, you must repeat that code, making it difficult to maintain.

How related is this proposal to the node replacement system #3367?

Both are intended to simplify the way nodes are extended. Before that solution, extending nodes was even more complicated. You would have to overwrite all the commands, plugins, or functions that created the node you wanted to replace.

If you have a POC at hand, a sandbox that demonstrates how you can convert an indented paragraph to heading (and back) while keeping the indent would be help demonstrate the value of this proposal.

In fact, it seems that they have solved just the use case of indents (I put it in the edit of problem 1). You can try it in the playground. However, this cannot be done by us with custom properties of our nodes (as requested by @aquarius-wing at https://github.com/facebook/lexical/issues/2557).

trueadm commented 1 year ago

I think this would be a very large change to the current architecture. Not saying it's a bad thing, but introducing such changes now would be very churning on the existing set of nodes and plugins.

GermanJablo commented 1 year ago

Update: This proposal can be divided into parts. The first PR that is to eliminate the Clone method is already pending review: Update2: ExportJSON/ImportJSON on the way

The reason I started with Clone() is because I found a way to generalize it without even requiring _properties as I proposed above.