Feature: Add runtime invariants for methods with unsound types: updateDOM, updateFromJSON, afterCloneFrom

[etrepum]

Description

Follow-up to #6970 and #6992. There's basically a variance violation in these methods, they are only correct when this is not up-casted to a base class (these methods violate LSP). There isn't really a straightforward typing rule we can use to prove this that has reasonable DX, but we can add a simple invariant to prove that there isn't a casting violation.

class ExtendedTextNode extends TextNode {
  afterCloneFrom(prevNode: this): void {
    super.afterCloneFrom(prevNode)
      .setNewProperty(this.getNewProperty());
  }
}

// This will throw an error about a missing `getNewProperty` method at runtime,
// but passes type checks due to the up-cast to TextNode
const extendedTextNode: TextNode = $createExtendedTextNode();
extendedTextNode.afterCloneFrom($createTextNode());

We can add an invariant to check this, it still won't get caught by the type checker, but will provide for better runtime errors:

class ExtendedTextNode extends TextNode {
  afterCloneFrom(prevNode: this): void {
    invariant(prevNode instanceof this.constructor, 'prevNode must be an instance of ExtendedTextNode');
    super.afterCloneFrom(prevNode)
      .setNewProperty(this.getNewProperty());
  }
}

Affected methods with this soundness violation:

• afterCloneFrom
• updateDOM

updateFromJSON has a similar variance violation, but it can't really be detected so simply, since the serializedNode argument is not an instance at all. We could provide a function to check the constructor type chain though.

• updateFromJSON

function $checkSerializedType(node: LexicalNode, serializedNode: SerializedLexicalNode) {
  const nodeKlass = node.constructor;
  const registeredNode = $getEditor()._nodes.get(serializedNode.type);
  // the JSON can be from the exact type, or a more specific type
  invariant(
    registeredNode !== undefined && (
      registeredNode.klass === nodeKlass ||
      nodeKlass.isPrototypeOf(registeredNode.klass)
    ),
    '$checkSerializedType node of type %s can not be deserialized from JSON of type %s',
    nodeKlass.getType(),
    serializedNode.type,
  );
}

Alternative

An alternative might be to change the protocols entirely, maybe with some sort of static method called for each class when it's added to the editor and to plumb these calls through the editor in a provably type-safe way, but that's not backwards compatible. Something like this might work:

export interface UpdateDOMFunc<T extends LexicalNode> {
  (node: T, prevNode: T, dom: ReturnType<T['createDOM']>, config: EditorConfig, next: () => boolean): boolean;
}
export interface UpdateFromJSONFunc<T extends LexicalNode> {
  // None of this is truly type safe, these are just casts,
  // the correct solution is to parse
  (node: T, json: ReturnType<T['exportJSON']>, next: (json?: ReturnType<T['exportJSON']>) => T): T;
}
export interface AfterCloneFromFunc<T extends LexicalNode> {
  // No need for middleware here, we can just force all of them to be called in order
  (node: T, prevNode: T): void;
}

class NodeRegistration<T extends LexicalNode> {
  // This could be used instead of static clone and static importJSON
  registerCreateNode($createNode: () => T): void;
  registerAfterCloneFrom($afterCloneFrom: AfterCloneFromFunc<T>): void;
  registerUpdateDOM($updateDOM: UpdateDOMFunc<T>): void;
  registerUpdateFromJSON($updateFromJSON: UpdateFromJSONFunc<T>): void;
}

class TextNode extends LexicalNode {
  static registerNode(reg: NodeRegistration<TextNode>): void {
    reg.registerCreateNode($createTextNode);
    reg.registerUpdateDOM((node, prevNode, dom, config, next) => {
      // this is the equivalent of the super call
      // depending on the node this might be called before, after,
      // or not at all based on the requirements of this subclass
      if (next()) {
        return true;
      }
      // … some logic specific to this node
      return false;
    });
  }
}

I'm sure we could come up with a backwards compatible transition from one to the other, if we leave the methods somewhere on the base classes for a transitional period so that super calls work for nodes that do not support the static registration protocol.

Impact

This will improve error messages for incorrect usage of the library in places where the type checker can't help.

[zurfyx]

Since the introduction of $applyNodeReplacement I've always wanted to see it evolve into a sort of Node builder. This function solves the actual problem (node replacement) but 1) it's optional, there's no enforcement enforcement of any kind 2) it's not extensible.

A Node creation mechanism that is controlled by us allows us to do extend the creation logic seamlessly, runtime checks and optimizations.

I like that your proposal touches on this idea.

---

static registerNode(reg: NodeRegistration<TextNode>): void {

Why is this instance pre-created? I would have expected the user to create a NodeRegistration instance where the Node would be part of it, and then Lexical would take either a Node or a NodeRegistration class (for backward compatibility reasons). But I can also see how your solution integrates more seamlessly with the current model and potentially work best with the extended Nodes.

[etrepum]

It doesn't really matter whether the NodeRegistration is created by the user (no static method requirement with that, but it is a separate identifier to export and use) or by the editor. If it's created by the user then it has to be a sort of inert abstract configuration object that editors will evaluate whenever they are constructed.

If it's created by the editor then it can be "tagless final" such that the methods are registering themselves with the editor when they are called, rather than building a configuration object that is evaluated later. It was just the first thing that came to mind because it's similar to how the editor works now.