Repository navigation
Generalize setters/getters, clone, importJSON and exportJSON #3763
Description
The Problems
In #1262 (comment) 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:
- 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.
- 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.
- 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
Activity
echarles commentedon Jan 24, 2023
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 commentedon Jan 24, 2023
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.
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.
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 #2557).
trueadm commentedon Jan 31, 2023
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 commentedon Feb 16, 2023
Update: This proposal can be divided into parts. The first PR that is to eliminate the
Clonemethod is already pending review:Update2:
ExportJSON/ImportJSONon the wayThe reason I started with Clone() is because I found a way to generalize it without even requiring
_propertiesas I proposed above.[-]Set/Get primitives = Shared properties + reduced verbosity[/-][+]Generalize setters/getters, clone, importJSON and exportJSON[/+]GermanJablo commentedon Mar 6, 2025
I'd say this has been largely solved with the State API and would be completely solved with (1) the $config API and (2) a plugin to insert DOM attributes using the state API (perhaps in the future).
It also remains to be seen how the DOM import/export problem could be solved.
Although there are still some things to improve, I'm going to close this issue because it's very large in scope, and ideas have evolved since then.
etrepum commentedon Mar 6, 2025
I have some ideas for how to solve the DOM import/export problem (#7259) but I'm waiting for the $config API to be discussed before going forward with any configuration related to NodeState