[lexical]: Breaking: add ability to use type discrimination for lexical node types

[AlessioGr]

BREAKING: This is technically a breaking change, as this PR narrows types. Additionally, users may experience TypeScript errors when extending nodes with this new type. Here is an example of such an error (the Tab node extending the TextNode):

I have fixed this issue for TextNode extending here, as it's a common node to extend. Though this will happen for other types, forcing the user to use type assertion. This is definitely a con of this PR, though I still think the benefits considerably outweigh the costs. Read the discussion in this PR for some ideas to alleviate this issue

Description

This type improvement is essential when working with a type union of all serialized nodes. In practice, this happens a lot whenever you want to traverse the editor state in a type-safe way, e.g. to convert it to JSX. Here is an example: https://github.com/payloadcms/website/blob/a9de2236aa5a21e9c30d7cfca2d33622a623e13f/src/components/RichText/serialize.tsx#L30

In this example, we utilize a switch statement for node.type (line 144). Before this change, the node types would not be discriminated inside the respective case blocks, as type was always a string. Now, they are discriminated perfectly.

E.g. within the

 case 'heading': {
  const Tag = node?.tag
  return (
    <Tag className="col-start-2" key={i}>
      {serializedChildren}
    </Tag>
  )
 }

block, node is discriminated to a SerializedHeadingNode. I can access node.tag safely and only see properties of the heading node

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
lexical	❌ Failed (Inspect)			Aug 23, 2024 0:36am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 23, 2024 0:36am

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	29.38 KB (0%)
lexical - esm	29.24 KB (0%)
@lexical/rich-text - cjs	37.74 KB (0%)
@lexical/rich-text - esm	31.05 KB (0%)
@lexical/plain-text - cjs	36.41 KB (0%)
@lexical/plain-text - esm	28.42 KB (0%)
@lexical/react - cjs	39.61 KB (0%)
@lexical/react - esm	32.52 KB (0%)

[etrepum]

I've implemented this sort of thing in my own code like this:

export type SerializedEmojiNode = Spread<
  {
    type: ReturnType<typeof EmojiNode.getType>;
    shortcode?: string;
  },
  SerializedTextNode
>;

export class EmojiNode extends TextNode {
  // …
  static getType(): "emoji" {
    return "emoji";
  }
  // …
}

I think it would be a good idea to adopt a style like that, or something else that ties the node to the serialization in a type-safe way. There could also be a utility type that does the type spread using getType to reduce a little bit of the redundancy as well.

It looks like your code is failing because the pre-commit stuff didn't run (e.g. prettier) so it's not passing ci-check

[AlessioGr]

@etrepum I think prettier did run everywhere, I do have pre-commit hooks enabled. Getting lots of flow errors in CI though, not sure why yet. Will investigate.

Regarding using type: ReturnType<typeof EmojiNode.getType>; I initially considered this approach but ultimately decided to hardcode the type instead. Here are my reasons for this decision:

• Simplicity and Readability: Hardcoding the type is more straightforward and easier to understand when examining the codebase. When users hover over the type in their IDE, for example, it takes less time to comprehend what the type represents:
  

• Improved Type Suggestions: I've noticed that my IDE sometimes fails to provide auto-suggestions for the type prop when using the ReturnType helper. Additionally, it occasionally includes extraneous suggestions. Here's a comparison:

vs.

This might be IDE-specific - haven't tested this in VS Code. I think WebStorm does not use the official TypeScript language server yet

• The node types will not change anyways - that'd be a huge breaking change which we cannot automatically migrate using node versioning. Therefore, while hardcoding may not strictly adhere to DRY principles, it wouldn't have any adverse effects in this context.

• Type Performance: Hardcoding is less demanding for TypeScript to compile

I've also been making a conscious effort to minimize the use of utility types in our codebase, unless they significantly reduce repetition. I've observed that when types are chained together in complex ways (using generics, unions, intersections, etc.) and numerous type helpers are employed, the TypeScript compiler sometimes struggles to properly evaluate the type, or stops evaluating the type completely, likely due to performance considerations. Moreover, these utility type chains can occasionally alter the type in unexpected ways.

Furthermore, when hovering over such complex types, IDEs often display extremely long type definitions, as they no longer preserve the original type names. So, I'm trying to use utility types only when they really help a lot. This is just one small utility type, but it would contribute to more significant issues appearing eventually if this is used within complex types

What do you think about this?

[AlessioGr]

Will keep this PR as draft for now. I noticed that this change has one major issue. Types like a TabNode (which extends TextNode) cannot be assigned to TextNode anymore, due to the incompatible type types.

Will need to figure out a better solution that still allows this

[etrepum]

The IDE related issues can mostly be addressed by mapping over the final type, e.g. https://www.totaltypescript.com/concepts/the-prettify-helper

[AlessioGr]

The IDE related issues can mostly be addressed by mapping over the final type, e.g. totaltypescript.com/concepts/the-prettify-helper

Didn't know about that type, I'll give it a try! Got some very nasty types where this might be useful

Regarding that type incompatibility issue: We could export some kind of base type alongside every serialized type. E.g. SerializedBaseTextNode and SerializedTextNode, SerializedBaseCodeNode and SerializedCodeNode etc.

The base node types would have their type property weakly typed as string - those would then be used to type the importJSON / exportJSON / other methods within the class, as to allow extending the node classes easily without having to deal with type errors.
Additionally, those should be used for things like function arguments, to allow passing the extending nodes.

The normal, non-base nodes should then be used anywhere else, as they accurately represent what data is saved in that node, and allow for type discrimination.

Would love to hear some thoughts regarding this approach

[etrepum]

I'm not sure what the best solution is, haven't thought about it too much, but the use of static methods (and backwards compatibility with all existing code) definitely makes things a bit awkward. Realistically the type discrimination isn't typically a problem for most lexical apps because the JSON is usually just some opaque type that the editor serializes and deserializes so I think that's why it hasn't been a priority for anyone else to make those types more ergonomic.