[lexical][@lexical/selection] Feature: add a generic classes property to all nodes for easier customization

[GermanJablo]

Many places have discussed how node customization and extension could be made simpler (e.g. [1], [2]).

We can divide the solutions into 2 groups:

1. make some methods not required when extending nodes by doing the work automatically. See my PRs about a generic clone, and also the one about export/importJSON. While they were closed for being stale they have been well received and I think there is still good will for a change in that direction. The challenge however is that the automatic methods could end up having worse performance.
2. Minimize the number of use cases in which it is required to extend a node. While there have been some PRs that have helped in that direction (like the new DOMSlot API), in this PR I want to propose a new way to customize nodes with more flexibility and without the need to extend and replace nodes.

Introducing Classes

Most of the time when users want to customize a node they just want to add a property to it, which ends up being reflected as a class in the dom element.

To satisfy that need we can introduced two new methods to all nodes: getClasses and setClass.

export function CoolRedPlugin() {
  const [editor] = useLexicalComposerContext();

  return (
    <button
      onClick={() => {
        editor.update(() => {
          $forEachSelectedTextNode((textNode) => {
            // setClass mutates the classes object where the key-value pairs follow the
            // format prefix-suffix for string values, or just prefix for boolean values.
            textNode.setClass('bg', 'red'); // adds the class bg-red
            // Perhaps you don't want to allow the same node to have
            // both text and background color defined at the same time...
            textNode.setClass('text', false); // ...so here you remove the class text-[color].
            textNode.setClass('cool', true); // adds the class cool (true values don't add a suffix)
          });
        });
      }}>
      Make text red and cool
    </button>
  );
}

EDITED: expand this to see the first API proposal

export function CoolRedPlugin() {
  const [editor] = useLexicalComposerContext();

  return (
    <button
      onClick={() => {
        editor.update(() => {
          $forEachSelectedTextNode((textNode) => {
            // Allows mutation of the classes object where the key-value pairs follow the
            // format prefix-suffix for string values, or just prefix for true boolean values.
            textNode.mutateClasses((classes) => {
              classes.bg = 'red'; // adds the class bg-red
              // Perhaps you don't want to allow the same node to have
              // both text and background color defined at the same time...
              delete classes.text; // ...so here you remove the class text-[color].
              classes.cool = true; // adds the class cool (true values don't add a suffix)
            });
          });
        });
      }}>
      Make text red and cool
    </button>
  );
}

Under the hood the way I am implementing it is with a flat object whose values can be just strings or true.

type MutableClasses = {[classSuffix: string]: true | string};
type ReadOnlyClasses = {readonly [classSuffix: string]: true | string};

class LexicalNode {
  __classes?: ReadOnlyClasses;
  getClasses(): ReadOnlyClasses {
    const self = this.getLatest();
    return self.__classes || {};
  }
  mutateClasses(fn: (classes: MutableClasses) => void) {
    const self = this.getWritable();
    self.__classes = self.__classes || {};
    fn(self.__classes);
  }

Considerations

1. why didn't I define it as an array of classes (strings), imitating the Element.classList API?
   • For convenience and performance. A CSS property with multiple values is often mapped to a class. With an array, changing the background color for example would imply going through the whole array and eliminating the conflicting classes. This would be more verbose to program, slower, and more error-prone.
2. What about the styles property?
   • This classes API is more flexible and easier to extend for the reasons I just gave. In addition, a class has the advantage that it can apply several styles at once, and its JSON serialization is smaller.
3. You can see I am not using __classes in the createDOM, exportJSON, importJSON, clone and exportDOM of each node, but in the places where those methods are invoked for 3 reasons:
   • it would require modifying more files.
   • if users extend a node they would need to repeat that process in several methods which degrades the value of this feature.
   • I am following a pattern that Lexical is already using with some properties. For example, __indent is applied in the reconciler and users do not have access to it.
4. These methods are not type-safe, (their purpose is precisely for users to use them as they wish). We could add a generic to getClasses<T> and muteClasses<T>.
5. To make this functionality easy to use in TextNodes, I created a new utility $forEachSelectedTextNode from $patchStyleText. There are other methods that could use it because they are very similar, such as $RangeSelection.formatText.
6. I'm making sure that the JSON serialization is as lightweight as possible (no export of forbidden values and no export if empty or undefined), something I've been wanting to do for a while for all the properties of all the nodes by the way :)
7. This PR does not introduce opinions about how the importDOM of the classes should be. If users want to paste HTML from external sources and maintain or transform their classes, they will probably want to extend their importDOM.

Test plan

The PR contains tests.
Also, if you want to test it in the browser you can paste the CoolRedPlugin above in the playground editor.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 11, 2024 1:19pm
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 11, 2024 1:19pm

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	31.18 KB (0%)
lexical - esm	31 KB (0%)
@lexical/rich-text - cjs	40.15 KB (0%)
@lexical/rich-text - esm	32.83 KB (0%)
@lexical/plain-text - cjs	38.79 KB (0%)
@lexical/plain-text - esm	30.15 KB (0%)
@lexical/react - cjs	42 KB (0%)
@lexical/react - esm	34.23 KB (0%)

[GermanJablo]

The changes in this file are only a rename to avoid collisions and can be ignored.
I have also wondered what would happen if a user has extended a node by adding the classes property to it. Perhaps we could reserve a prefix and use something like $classes instead?

[etrepum]

I like the general idea here but I am not sure that this is the ideal API design (I do not have a concrete alternate proposal, this isn't something I have thought much about). Some initial thoughts:

• There's no consideration for more semantic meaning or how this interacts with themes
• It may not work very well for users who are using utility classes (e.g. tailwind)
• Namespace collisions (as you've identified in the example)
• Doesn't really distinguish between classes used for createDOM or exportDOM
• The implementation seems to copy more often than is necessary, given the access patterns __classes should be the readonly type and mutateClasses should have you work on a copy (and it should allow you to return a different object if you want)
• The prefixed variant seems useful for semantic reasons (e.g. state: 'active' and state: 'inactive' being mutually exclusive without having to toggle them separately) but is maybe a bit too prescriptive for exactly what the output class name looks like (although it is BEM compliant so it's not without precedent)
• It's likely that a lot of createDOM/updateDOM implementations won't be compatible with this up front particularly because LexicalNode doesn't have an implementation of it and this PR only considers TextNode
• No consideration for importDOM (as identified in the description)

[GermanJablo]

Thank you for your thoughts!

(1), (2) and (6): I see no problem. Users can define the classes they want without any kind of prescription. They can choose state-active or just active. They can choose classes that match their utility framework or not.

(3): I think it would be good to reserve a prefix, since this problem actually applies every time Lexical adds a property or method. Or these occasions can also be marked as breaking changes...

(4) and (8): whether we should export the classes to the DOM is debatable. I don't think we should import them by default. It is left to the user to define the conversion, something they can do without replacing the nodes.

(5) that would involve making a copy in each call to mutateClasses. I'm not sure I follow you, but if you have a suggestion on the API and could put it in the code comments or commit it would be great.

(7): This PR works with all nodes. I made forEachSelectedTextNode because it is the most difficult use case, but you can modify the example plugin to add a class to a paragraph for example (getRoot().getFirstChild().mutateClasses(...)). It works fine.

As a side note, I would like to add that I have reviewed all the nodes I have replaced and talked to other users who have heavily customized their nodes, and in all cases we have found that this API could have saved us the work.

[etrepum]

I think overall I'm -0 on this proposal. I like the idea behind it, I think it would be great to have a bit more generic control over DOM nodes without subclassing (especially when mixing in some additional state). However, this approach seems like it's baked in at an awkward level of abstraction. Technically I think it should work if other maintainers agree that this is the approach we should adopt.

I would prefer to have something that is integrated with the normal lifecycle of createDOM/updateDOM/exportDOM. Yes, this would require more careful plumbing, and wouldn't necessarily "just work" for third party node classes that aren't calling the super methods to a base class in lexical or otherwise have explicit support for this, but I think that the trade-off for consistency and future-proofing would be worth it.

[etrepum]

Checking the equivalence between to classes objects should really be in a function where it's easier to early exit and use a more performant for loop instead of every

Suggested change

	Object.keys(node1Classes).length === Object.keys(node2Classes).length &&
	Object.keys(node1Classes).every(
	(key) => node1Classes[key] === node2Classes[key],
	)
	(node1ClassKeys === null ||
	(node1ClassKeys.length === Object.keys(node2Classes).length &&
	node1ClassKeys.every((key) => node1Classes[key] === node2Classes[key])))

[GermanJablo]

Array.every exits early if it finds a counterexample

[etrepum]

Do we really want to re-create the node every time classes change? That seems odd.

[GermanJablo]

Well... I could rebuild just the class attribute, yes. But I don't think this behavior is odd considering that this is how updateDOM works with any other property in Lexical.

[etrepum]

This should all be in an optimized function per previous comment in LexicalNormalizer

[GermanJablo]

Array.every exits early if it finds a counterexample

[etrepum]

The more I look at this the less I like that this adds DOM manipulation to every node that's outside of createDOM and doesn't give the author of the node class any control over what's happening

[GermanJablo]

It is an opt-in feature. If you do not use setClass none will appear.
The same happens with __indent.

[etrepum]

Regarding prefixes I think the simplest thing to do would be to document that the two-underscore prefix is reserved for use by Lexical, and implementing their own classes in the same style may result in namespace collisions. However, we also add methods occasionally without any sort of prefix, so I guess the real answer is that users just have to live with possible compatibility issues over time unless they do their own prefixing because that's just how OOP works.

There are plenty of use cases for aria roles, dataset, and other attributes (particularly id, maybe title or microdata related attributes) with the existing nodes. For aria we don't really have semantic elements so you may want to specify the role or region. For other attributes the most obvious one would be id, why shouldn't you be able to add an id attribute anywhere for deep linking purposes, especially when the editor is in readonly mode or for export? Makes just about as much sense as being able to add classes anywhere, and it requires storing state on the node so it's not something you can solve with pure html.export configuration. The dataset property has use cases similar to classes, for things where this metadata should be invisible. You could also technically use dataset (or any other arbitrary attribute scheme) to target elements with CSS, so if this code managed the dataset property then it could be used for the exact same purpose as this (but would arguably be a bit more useful, since you can store arbitrary key/value string pairs in dataset, so it can be used for things like colors or unique/external ids).

[GermanJablo]

Thanks for your feedback again Etrepum.

Extending nodes is a very complex thing to do and with a lot of footguns. On top of that, there is another additional problem that I didn't emphasize in the description, and that is that extending nodes is not composable.

For example, in Payload the community can create their own plugins. If two of them extend the same node, it doesn't work.

This PR handles a lot of things for the user in a composable way. I've seen experienced users fail at many of those things, like making JSON serialization lightweight. I don't know if this API is perfect, but I think it adds value and solves a long-recognized problem at Lexical.

Does this mean that users will never have to use Node Replacement again? Not at all. That's not the purpose of this PR either. If they have to add an aria or any other attribute for some reason, they can extend the node. And if a very common use case emerges (which I doubt), a similar API for another attribute may be considered. But I think this will make life easier for all users in the vast majority of cases.

I join you in waiting to hear what the other team members think.

[etrepum]

You're right that extending nodes is not composable, so maybe the right solution is to come up with an extension mechanism that is composable rather than hack in support to only mess with the class attribute in particular.

[etrepum]

If we add an arbitrary bag of data that is guaranteed to be serialized/deserialized (at least with JSON) to the LexicalNode, then you could write a plugin that performs everything this proposal accomplishes with a similar interface, but also unlocks other possibilities.

• $setNodeClass(node, className, value) would update the data structure on the node, e.g. node.setDataKey('classes', (prev) => ({...prev, [className]: value}))
• The ClassPlugin registers an update listener to traverse dirty nodes and update the classList in their DOM accordingly (but you could also use a similar plugin to update any DOM for any node, or sync their changes with some external thing, based on other properties).

Missing pieces are:

• Arbitrary data property for each node that participates in the EditorState lifecycle (e.g. getWritable before updating, gets cloned, etc.)
• JSON serialization for the data property (would either need to be plumbed correctly, or hacked in like children are or like this proposal did for classes)

For full fidelity you'd probably want similar extension points for importDOM and exportDOM, something like middleware where you are able to get the result of what would happen without the plugin, and then have the opportunity to modify the result based on the context. This would probably be useful for other purposes.

This data property would also be supported by RootNode (just by virtue of where it would be implemented in the class hierarchy) so we would also finally have a way to store versioned document level data too (this pretty much covers any use case for isolated decorator nodes which don't really work).

[GermanJablo]

I find the idea interesting, although it feels like a major abstraction for a feature that no one has asked for and that it could be implemented without breaking changes even if this PR had already been merged.

As long as the JSON serialization ends up being the same, you could have the styles stored in the format that exists today, and the classes in that of this PR.

And speaking of format, one of the things I like most about this PR is that classes follow the key-value format with the option to result in prefix-suffix, since manipulating an array of strings is often awkward. Maybe for things whose real abstraction is not a boolean but a set of options like background-color it could be used data-key=value, but I don't know.

You also mentioned a ClassPlugin. I would like to avoid that. However this is implemented, I would love for users to be able to set a class to the node and be done.

For full fidelity you'd probably want similar extension points for importDOM and exportDOM, something like middleware where you are able to get the result of what would happen without the plugin, and then have the opportunity to modify the result based on the context. This would probably be useful for other purposes.

Same thing I said just now. I would love exportDOM to work out of the box. One of the main points of all this is to reduce the complexity of customizing nodes. ImportDOM is trickier because the content can come from an external source like the clipboard, but for that there is already this option.

[zurfyx]

Thank you German, we appreciate your contributions as usual! We discussed this PR in the previous session.

Overall, there's some benefit in the newly introduced setClass API, and it does without any doubt make live easier in certain scenarios. However, we would be keen to have this class shortcut as a separate module rather than built-in into the core. Here's why:

Cost
This feature can be seen as an enhancement, a shortcut to CSS classes. Plugins can replicate this same behavior by accessing the rendered DOM element and applying the classes directly. It's inconvenient but the fact it's redundant and low number of times that this happens makes it hard to justify the budget size increase (even if rather small on the core).

The recent DOM Slot API would instead fall into the category of low-use but a must-have to enable certain use cases.

Similarly, I'm personally not keen on the update extra props redundancy but this is unfortunately the product of legacy, since the $ functions came later.

Backward compatibility
This PR introduces opinionated behavior that might not necessarily work well with existing products. The classes prop can potentially conflict and there is no workaround, and a few other items identified in your discussion above.

It is fair to say we have done many backward incompatible changes in the past year but storage is an area that must be backward compatible, or at least fully understand the impact that these can have on existing products.

---

We still think there is value in the class shortcut, do you think we should explore it as a utility function? We can also use the VC time this week to discuss it over if you prefer, I would love to hear more about your actual use case

[GermanJablo]

Hi @zurfyx, thanks for the review!

Plugins can replicate this same behavior by accessing the rendered DOM element and applying the classes directly.

Even if we patched the DOM outside of Lexical, the classes would have to be properties of the nodes. How are you visualizing the persistence of the classes if this is not the case?

but the fact it's redundant and low number of times that this happens makes it hard to justify the budget size increase

What do you mean by redundant and budget size? I understand that Meta or other users may have lost interest if they have already achieved their goals in other ways. But I am trying to understand the disadvantages of introducing something like this. The cost in bundle size and runtime performance is negligible.

The recent DOM Slot API would instead fall into the category of low-use but a must-have to enable certain use cases.

It helps that you talk about it as a "must-have". That PR started after my proposal for a scrollable-node. Yes, DOMSlot is more convenient, but it was possible to achieve its goals in another way without introducing a new API.

If that API was accepted, I think this one has much more merit to be so. As I said before, "I have reviewed all the nodes I have replaced and talked to other users who have heavily customized their nodes, and in all cases we have found that this API could have saved us the work.".

And to put into context the usefulness of this, it would be good to remember everything a user has to do today if he wants to make a colored TextNode:

Expand to see example

export type SerializedColoredTextNode = Spread<
{
  color: string;
  type: 'colored';
  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,
  };
}
}

export const nodes = [
  ColoredNode,
  {
    replace: TextNode,
    with: (node: TextNode) => new ColoredNode(node.__text),
    withKlass: ColoredNode,
  },
  // ...
]

I could list 100 common mistakes that aren't obvious and could go wrong when doing that. It doesn't seem like this should be the preferred way to customize a node. But still, this isn't my main reason for wanting to introduce a new API (more on that below).

Backward compatibility

Are you seeing some incompatibility that I am not? How could this API break existing products?

I would love to hear more about your actual use case

Improving DX is important, no doubt. But my main interest is what I mentioned above: "extending nodes is not composable. For example, in Payload the community can create their own plugins. If two of them extend the same node, it doesn't work".

This may not be crucial for a user like Meta who has full control over the editor's source code. But on platforms like ours this composability is crucial.

[etrepum]

To your point I think that extending/overriding nodes in a more composable fashion is a great idea, and an ideal to move towards, but I don't think that adding this really gets us much closer to that except for one opinionated use case. It just gives us a property that's managed in a different way than almost everything else is that we would have to support, and it doesn't really compose well with the existing ad-hoc direct classList manipulation or theme facilities. I think if we were to do something like this it should be a more holistic proposal that improves and/or integrates with all of it.

What about having PayloadCMS override TextNode, provide any extensibility hooks you want in whatever fashion you think would make sense for that ecosystem, and have plugin authors extend or otherwise augment your implementation instead of baking this directly into the reconciler?

[GermanJablo]

For the record, I am super fine with introducing a more general or “holistic” API like the one you mention. Also, I think you have convinced me that data-key=value is more appropriate for key-value objects. I can try to make a proof of concept. Do you have an idea of what the API would look like? Maybe something like this?

type Attributes = {
  classList: string[],
  id: string,
  data: Record<string, string>,
  aria: Record<string, string>,
}

class LexicalNode {
  attributes: Attributes
}

Or maybe just type Attributes = Record<string, string> so they can put arbitrary things (maybe tabIndex, role...?).

About the second paragraph, the problem is that it would break users who are already extending TextNode.

[etrepum]

Regarding existing users who have already extended TextNode directly, I don't think that Lexical's reconciler should perpetually carry this burden to avoid a one-time refactor that would give them this extra functionality. Lexical is not 1.0. While backwards compatibility for a period of time is always a critical concern to support its major consumers, it's expected that the API is going to continue to evolve and projects will have to track it especially if they want new features.

If you really did want to simplify the ColoredTextNode implementation you pasted above all you would have to do is write two functions $getTextColor and $setTextColor that parse and manipulate the style. Something like this:

// getStyleObjectFromCSS and getCSSFromStyleObject are not currently exported but are in lexical-selection
function $getTextColor(node: TextNode): string {
  return getStyleObjectFromCSS(node.getStyle()).color || 'defaultColor';
}

function $setTextColor(node: TextNode, color: string): void {
  node.setStyle(getCSSFromStyleObject({...getStyleObjectFromCSS(node.getStyle()), color}));
}

I think it would be a bad idea to commit to a specific representation for managing attributes, because it prevents future optimization and refactoring. I think the API should be some combination of methods and/or $functions to read or manipulate them. IIRC in order to support today's collab this data structure would have to be both JSON serializable and read-only (or managed carefully with properties).

There are a couple things to consider here:

• Many plug-in use cases really just need per-node state that ends up in EditorState, and that storage doesn't necessarily need to be also serialized to the DOM
• A flat classList representation doesn't necessarily cover all use cases well, particularly since some people use utility classes. I think one of the roughest edges right now is that there isn't a more managed way to do this, if you scan the code for removeClassNamesFromElement and audit carefully you can find a lot of potential issues when there are overlapping sets of classes based on states. In your use case the hyphenated BEM style modifiers would also fall into this sort of bucket, which is why you were incentivized to come up with the opinionated approach that would let you determine the diffs and allow only one at a time by prefix.
• There are differing opinions on how things like classes should work for export, import, and managed editor DOM (e.g. RFC: Allow $generateHtmlFromNodes to not export classes #6968).

I don't have a concrete proposal for an API that covers everything well.

[GermanJablo]

Yes, it works for styles, but that wasn't the point of the example. Any case where you need to add another common property (like a class) to a node requires all that ceremony.

I think it would be a bad idea to commit to a specific representation for managing attributes, because it prevents future optimization and refactoring. I think the API should be some combination of methods and/or $functions to read or manipulate them.

That's a good point. Many DOM APIs make a lot of sense in specific contexts and could be easily replicated. The lowest level ones are setAttribute and getAttribute. Since they would be needed to provide something generic and the more specific ones can be derived from them, they seem like a good starting point.

My suggestion would be to provide setAttribute and getAttribute methods. JSON serialization would be a plain string object, just like you see in HTML, and Lexical should take care of that serialization as I do in this PR.

That would allow us to optimize or refactor some APIs later. For example, if we wanted to add classList.toggle/add/remove/contains, we could modify the importJSON and exportJSON of attributes.class to be a Set for performance reasons.