facebook / lexical

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

Proposal: Lexical Schemas #3833

Open tylerjbainbridge opened 1 year ago

tylerjbainbridge commented 1 year ago

Lexical Schemas

The Problem

A lot of mistakes with Lexical are caused by developers adding nodes where they shouldn’t.

Schemas are an API proposal that’s designed to add guardrails around the insertion of nodes. They'll check if a given relationship (parent, child, sibling) is valid and allow you to normalize it if possible.

Slate.js solves this elegantly with their Schema (https://docs.slatejs.org/v/v0.47/guides/schemas) API and developers find it intuitive, so it makes a lot of sense to propose a similar API that’s familiar to users of Slate and other Rich Text solutions.

The Solution

We’ll stick with our current approach of encapsulating node-related logic on the class itself by introducing a new static method, getSchema, which returns a Lexical Schema.

The schema’s relation methods will be called at different points within the core insertion logic and aim to remove the many çase-specific patches and node methods throughout the codebase while catching bugs before hitting production.

If any of the validations fail it will warn the developer in the console and we’ll provide a normalize method that’ll act as an escape hatch and let you to fix the issue.

Example

Take the following Table Row example A row should never not be a child of a table, it should never have a child that isn’t a cell, and it should never be a sibling that isn’t another row. This is expressed almost verbatim in code below.

class TableRowNode extends DEPRECATED_GridRowNode {
  static getSchema(): Schema<TableRowNode> {
    return {
      relations: {
        parent(a: TableRowNode, parent: LexicalNode): boolean {
          return $isTableNode(parent);
        },
        child(a: TableRowNode, child: LexicalNode): boolean {
          return $isTableCellNode(child);
        },
        sibling(a: TableRowNode, sibling: LexicalNode): boolean {
          return $isTableRowNode(sibling);
        },
      },
      normalize(node: TableRowNode, error: LexicalSchemaError, editor: LexicalEditor) {
       if (error.type === 'invalid_parent') {
         // try to repair
       }
      }
    }
  }
}

// For Product specific validations an API like this could also be useful.
$extendSchema(TableRowNode, {
    relations: {
        parent(a: TableRowNode, parent: LexicalNode): boolean {
            // something
        },
        child(a: TableRowNode, child: LexicalNode): boolean {
            // something
        },
        sibling(a: TableRowNode, sibling: LexicalNode): boolean {
            // something
        },
    },
    normalize(node: TableRowNode, error: LexicalSchemaError, editor: LexicalEditor) {
        if (error.type === 'invalid_parent') {
            // try to repair
        }
    }
})
abelsj60 commented 1 year ago

Tyler, really nice, succinct discussion here. You all are much better than I am at this, but I have a couple questions. Overall, this seems like it's very helpful and clean. I'm thinking about it from an unusual vantage point, though.

Let me give you a little background:

I. A nested example

I've done a lot of work putting a LinedCodeNode together. One of the lessons is that building nested text layouts in Lexical is very hard. The Override API made this a palatable endeavor — I had the node working before the Override API was available and you would have hated it. It required a lot of command overrides.

Nevertheless, after a lot of work, I can still have trouble conceptualizing nested Lexical layouts.

Take import/exportJSON.

Say you have a top-level node of code. Its immediate children are divs, which are lines, and its grandchildren are spans (text/code). Which node should control import export?

I settled on having the top-level node — the shadowRoot or code — control all. I really liked the notion of each nested node controlling its own destiny in an abstract way, but, unsurprisingly, in practice, this caused one of my eyes to pulse.

I bet you can image some of my questions now:

II. Who's destiny?

What kind of node gets a schema — ElementNode, TextNode, or both? and which node wins if the developer has created a nested structure? In some ways, your example is a good example of how I, myself, can become instantly confused.

Your example is of a TableRowNode — what about the TableNode, though? What do I do with that? I see one of the cited "relations" is a parent — so am I checking the schema of the whole Table from the TableRow?

Or, are you saying each node checks a schema of parents and children because they all have to mind their own destiny? That's great, I guess...but, then, what do I do with normalization? Do I handle it on parent or child...or sibling?

And what happens when two normalization functions produce results that conflict with each other in a nested structure?

III. A relative dash

When do the schemas run? On creation? Before or after .append, .splice., etc?

This seems like a dangerously abstract question — so let me work with an example, instead.

When creating the LinedCodeNode, I kick everything off with a $createLinedCodeNode() function. Then, later, I create lines via a $createLinedCodeLineNode() function.

They may be appended or spliced into place, depending. That's all fine.

In the context of a schema, though, I'm not sure what happens when I run either of these functions.

In the case of the top-level node (LinedCodeNode), the lines don't exist yet. Will the schema method run after I've assembled enough of the parts to make it happy? Or will it run too soon and object, even though I'm not done yet?

Same question with the lines. They start out on their own, without a parent. Then they're appended or spliced into place. Will the schema method run after they've been assembled? Or will it run before that happens — and complain?

The answer may be plainly obvious and everything will be great. Probably best to ask now, though.

IV. Let's drive...

So my last thought is a rumination.

When I first heard you all mention schemas, I thought it might be a way to more easily assemble nested structures.

In this fantasy land, I imagined the schema would be somewhat akin to importJSON or importDOM. Basically, I thought it might contain a "node layout" — a "map" — representing the final nested structure. You'd pass it the data needed to assemble said structure. And it would then follow your map and build your nodes and update the selection.

Ish. I'm not half as good a coder or designer of such things as you all are, but I do sorta wonder if the schema could be more of an active participant in the creation of nodes than an enforcer of rules.

If the former, developers could leverage it to make more predictable and complex nodes. If the later, it's mostly for development and we can always console.log our way through it? Maybe you all have other plans for this.

Again, I figured now was a good time to ask.

-j

GermanJablo commented 1 year ago

I love it! ❤️

First of all, I would like to highlight the Prosemirror implementation, which I find more powerful, elegant and flexible than Slatejs. It allows to do in a trivial way very crazy and complex things like the one I mentioned in another thread.

Leaving that aside, there are a few problems I find in the proposed implementation:

1. A node should only define the schema on its descendants, not on its siblings or its parent.

Otherwise, you're going to find yourself checking schemas redundantly, and they may also be incompatible (note that parent-child and sibling relationships are two-way).

2. Centralized schema?

Don't you think it's better to define the schema in a centralized place instead of having the schemas scattered across many files, which in turn can be rewritten with $extendSchema? Following the Prosemirror notation, the default Lexical schema might look something like this:

nodes: {
     text: {},
     root: {content: "(paragraph | heading | code | quote | listNode | table | decorator)*" },
     listNode: {content: "listItem*"},
     listItem: { content: "list* | (text | link)*" },
     table: {content: "tableRow*"},
     tableRow: {content: "tableCell*"},
     tableCell: {content: "(paragraph | heading | code | quote | listNode | table | decorator)*" },
     code: { "codeHighlightNode*" },
     codeHighlightNode: {},
     decorator: {},
     link: {content: "text{1}"},
     elmentNode: { content: "(text | link)*" }, // rest of nodes not defined above?
   }

This could be put in the same "nodes" parameter that receives the initialConfig. Beautiful.

3. Should the new editorState be normalized, or just rejected?

I find Schema to be an amazing feature for (1) preventing bugs, and (2) modifying the behavior of the editor in an incredibly flexible, easy and powerful way (for example if I want the document to start with an H1 (i.e. root: {content: "heading{1} (paragraph | heading | code | quote | listNode | table | decorator)*" }, any operation that violates this it just wouldn't run.

In my opinion, this is achieved simply if, when an invalid editorState is generated, the transform is undone and reverts to the previous editorState.

Perhaps the logic in detecting an invalid editorState could be:

If anyone thinks that this generic solution is not enough, can you give me an example of a more complex normalization that you would use in real life? I can't think of any.


@abelsj60 I will try to answer some of your questions here:

acywatson commented 1 year ago

Don't you think it's better to define the schema in a centralized place instead of having the schemas scattered across many files, which in turn can be rewritten with $extendSchema?

I agree with this and came here to say something similar. I made the argument for doing this with import/exportDOM, as well, and I originally implemented it that way. The centralization has some trade-offs, but ultimately, I think it's a better DX because you can easily grok what's happening without jumping around and trying to understand priority interactions in different node classes. On the other hand, centralizing this would certainly be a departure from how we have approached structuring such APIs in the past, which was an intentionally-made decision.

I also am not sure how the current proposal will support use cases like starting the document with an HeadingNode always, or perhaps having an editor that only allows a single top-level ParagraphNode. Would you use $extendSchema(RootNode) for that? Maybe it would be good to enumerate those use cases and see if the community can come up with any we haven't thought of.

@tylerjbainbridge

abelsj60 commented 1 year ago

Thanks @EgonBolton. I hear what you're saying. This sounds OK and like everyone has it in hand.

One thing I've learned, though, is that, if you see a rough edge where theory and practice could collide, and say nothing, you often end up with heartache and a lot of people staring at each other silently during some unanticipated meeting later.

Anyway, I look forward to the exciting conclusion.

acywatson commented 1 year ago

From Discord:

Perhaps our desire for schema normalization comes from limitations of older versions of Slate and the schema being the only way to salvage state that would otherwise become invalid sometimes

But new versions still have the concept of normalizing - just no longer called a schema https://docs.slatejs.org/concepts/11-normalizing

Defining what's allowed in a normalizer would prevent that content from being entered into an invalid area, or prevent valid content from becoming invalid due to editor operations.

For example, let's say a user pastes copied HTML content into a codeblock which can't contain a heading. Now when the user tries to paste into the codeblock, the editor will allow that.

What is the "Lexical" way of doing that?

In older versions of Slate we'd define that codeblocks can only contain nodes of a type we'd call CODE_LINE and anything else would be discarded automatically during normalization.

It also defined things like:

abelsj60 commented 1 year ago

From Discord:

Perhaps our desire for schema normalization comes from limitations of older versions of Slate and the schema being the only way to salvage state that would otherwise become invalid sometimes

But new versions still have the concept of normalizing - just no longer called a schema https://docs.slatejs.org/concepts/11-normalizing

Defining what's allowed in a normalizer would prevent that content from being entered into an invalid area, or prevent valid content from becoming invalid due to editor operations.

For example, let's say a user pastes copied HTML content into a codeblock which can't contain a heading. Now when the user tries to paste into the codeblock, the editor will allow that.

What is the "Lexical" way of doing that?

In older versions of Slate we'd define that codeblocks can only contain nodes of a type we'd call CODE_LINE and anything else would be discarded automatically during normalization.

It also defined things like:

  • CODE_BLOCK must contain a minimum of one CODE_LINE (I found this sort of thing especially helpful when dealing with Android, which can accidentally delete the sole text nodes within a block)
  • Defined that a document must begin or end with nodes of a certain type, eg HEADING or PARAGRAPH - because if a user put certain elements as the first (eg lists) it would make inserting new elements above it difficult
  • Define what types of elements are allowed inside of UNORDERED_LIST - in our case CHECKBOX_BLOCK and PARAGRAPH are allowed, but nothing else - eg no headings, no blockquotes

@acywatson I'm sorry, can I just ask, I guess don't understand something. What is the scenario you're warding against here? Is it pasting from external sources? Is regular typing or the use of a UI button that generates, say, a headline, a concern, why?

I get the code example — in my lined node, I mandate it's created with one empty line and subsequent appends check to see if there is only one single empty line at the top, and if there is, the first line is appended into the old one to avoid extra lines. Maybe this is really just a substitute for normalization, though? I wonder if I checked H1 pasting, though. Ha!

I'm just trying to understand as a passerby what you're aiming at with "normalization?"

I tend to think of it in terms of going from Node A to Node B, or assembling a nested node structure, but maybe I've got the wrong idea?

acywatson commented 1 year ago

@abelsj60 these aren't my words - just documenting some use cases set forth by someone on Discord. Generally, yea I guess there are contexts where you can direct Lexical to insert some node types into a context where they don't make sense, either in terms of the individual surfaces business logic or just from a common sense standpoint. Our strategy up to this point has been something to the effect of "just don't put those nodes there" but that doesn't scale super well when there are so many ways to insert content into the editor. There are a lot of cases you need to check for to make sure you don't get into a bad state. At least, this is how I understand the specific problem you're asking about.

abelsj60 commented 1 year ago

@abelsj60 these aren't my words - just documenting some use cases set forth by someone on Discord. Generally, yea I guess there are contexts where you can direct Lexical to insert some node types into a context where they don't make sense, either in terms of the individual surfaces business logic or just from a common sense standpoint. Our strategy up to this point has been something to the effect of "just don't put those nodes there" but that doesn't scale super well when there are so many ways to insert content into the editor. There are a lot of cases you need to check for to make sure you don't get into a bad state. At least, this is how I understand the specific problem you're asking about.

Makes sense, thanks for taking a minute.

I guess the comment you added had a lot of specificity and I realized that there were so many thoughts in my head that had I lost the plot. I think I got that expression from UK tele on Netflix.

acywatson commented 1 year ago

Just some research on Slates history with Schemas - they were completely replaced with Normalization years ago:

https://docs.slatejs.org/general/changelog#0.53-december-10-2019

The reasoning is because you can do anything with normalization (which is basically Lexical Transforms) that you can do with schemas - which is true. I think this will also be true with the Lexical implementation. I am still researching ProseMirrors implementation, but right now I'm thinking of schemas as just a convenient abstraction over a specific type of node transform. There's potentially value in this, for sure, as I see some of these use cases as pretty common, but node-based transforms sort of nudge me towards node-based schemas, as originally described.

acywatson commented 1 year ago

First of all, I would like to highlight the Prosemirror implementation, which I find more powerful, elegant and flexible than Slatejs.

@EgonBolton Wow - just got done reading over the Prosemirror documentation for Schemas. Certainly flexible and powerful, but perhaps a bit too flexible and powerful. I'd like to think we can create something significantly simpler than that and still address the vast majority of use cases.

Awjin commented 1 year ago

Here's another use-case. Say that we cut 3 paragraphs, then paste them inside a list item:

            insertion point
             |
             *
     - TARGET LIST ITEM

 +-* paragraph1
 |   paragraph2
 |   paragraph3 *-- selection focus
 |
selection anchor

The paste should be "context-aware", normalizing the paragraphs into list items:

- TARGET paragraph1
- paragraph2
- paragraph3 LIST ITEM

(Correct me if I'm wrong but) this is currently difficult to express in a "Lexical way" and would be better captured by a schema/normalization.

This also applies more generally to pasting into any element, e.g. pasting paragraphs into a code block.

acywatson commented 1 year ago

Here's another use-case. Say that we cut 3 paragraphs, then paste them inside a list item:

            insertion point
             |
             *
     - TARGET LIST ITEM

 +-* paragraph1
 |   paragraph2
 |   paragraph3 *-- selection focus
 |
selection anchor

The paste should be "context-aware", normalizing the paragraphs into list items:

- TARGET paragraph1
- paragraph2
- paragraph3 LIST ITEM

(Correct me if I'm wrong but) this is currently difficult to express in a "Lexical way" and would be better captured by a schema/normalization.

This also applies more generally to pasting into any element, e.g. pasting paragraphs into a code block.

Yea, that's right - you'd have to make all the importDOMs context-aware and do whatever transformations you want in there. Possible currently, but I wouldn't say it's easy. I think this use case is also why we can't just reject a change that violates the schema (instead of remediating) as @EgonBolton suggested. That would mean the paste would just fail, I suppose, which I don't think is acceptable.

GermanJablo commented 1 year ago

Tentative algorithm:

LexicalNode in each cycle marks the nodes that changed in 3 ways: created, destroyed and updated. So...

When a node's schema is checked, it is marked in some way, so as not to double-check it if, for example, two sibling nodes are created/destroyed. With this algorithm, the schema does not need to traverse the entire editorState from root to its leaves with each change.

When checking the schema of the parent of a mutated node, you could make a string concatenating the children, and then see if it complies with the regex (see the content property in my comment above).

This PR may be of interest: https://github.com/ProseMirror/prosemirror/issues/220#issuecomment-218591966

Normalization

Regarding normalization, maybe I don't explain myself.

image

I think both options make some sense.

Reasons why I think using node methods* and transformations might be better:

The reason why it might be better to have a lifecycle stage in charge of normalization:

*With methods I mean things like insertAfter, insertBefore, canBeEmpty, isShadowRoot, etc. They are in fact a way of normalizing.

Awjin commented 1 year ago

Here's another interesting use-case:

1. one
2. two
  *-- what if we delete this line?
1. three

OR

1. one
2. two
  *-- what if we paste  {{ 1. three }}  ?

I think the result should be:

1. one
2. two
3. three

Sibling-relationship normalization seems like the cleanest way to implement something like this.

(Although in the first case—deletion—would the normalization not get triggered, since neither list is dirty?)

acywatson commented 1 year ago

*With methods I mean things like insertAfter, insertBefore, canBeEmpty, isShadowRoot, etc. They are in fact a way of normalizing.

Sort of - these aren't all the same. The first two are simple list manipulations. The second two are behavioral heuristics - something closer to normalization, but still not it.

Easier to detect unwanted behaviors in advance. It would not be necessary to have to anticipate all the possible ways to arrive at an invalid editor.

I don't understand why this would be the case. Can you elaborate on what you have in mind. I'm almost certain I'll implement it in the transform stage, but I want to hear what you're thinking here.

acywatson commented 1 year ago

Here are the broad categories use cases I see so far:

1) Inserting content into the editor at an arbitrary location. You can construct a tree that isn't valid to insert into a specific node, and right now we have to rely on heuristics and special-casing to catch it.

Example: Pasting a an HTML list item (with no wrapping list) into a ParagraphNode.

2) Bugs (basically) - sometimes user events can simply cause the state to become malformed. Schemas could theoretically catch and remediate some of those issues at runtime.

Example: Pressing enter in a table cell inserts a new table cell (hypothetical, I hope).

3) Constrain document structure according to business logic

Example: I want to enforce that the root node always has a HeadingNode as the first child.

The important and interesting thing about 3) is that it requires not only validation of relationships, but also order and maybe quantity?

acywatson commented 1 year ago

(Although in the first case—deletion—would the normalization not get triggered, since neither list is dirty?)

If we rely on node transforms (or the same logic) it seems that it will trigger, thought I'm not quite sure why. It might not be something we can rely on in all cases, but this works:

function QuickTest(): null {
  const [editor] = useLexicalComposerContext();
  editor.registerNodeTransform(ListNode, (node) => {
    const nextSibling = node.getNextSibling();
    if ($isListNode(nextSibling)) {
      node.append(...nextSibling.getChildren());
      nextSibling.remove();
    }
  });
  return null;
}
GermanJablo commented 1 year ago

There are two different problems

I think this thread is discussing two different issues:

  1. What an API could look like to define validity rules on the editorState (ie Schemas).
  2. How to "automatically" fix an invalid editorState' (ie Normalization/Transformation).

Even if these two problems are solved in an integrated way in a single API (as in tylerjbainbridge's initial proposal), they are two problems that are inevitably somewhat isolated.

This is so for two reasons:

I guess the idea is that a user should be able to define a schema without defining a normalization, and should also be able to define normalization without defining a schema. The latter is actually already possible, with the transformations API (which is why I decided to use normalization/transformation as synonyms in this message).

What goes first?

As I said, there are two issues: schema verification, and normalization/transformation. Let's call the lifecycle stages where they are executed Schema verification stage and Normalization/transformation stage, respectively.

The question would be: Which of the two stages should be executed first? The answer depends on whether we consider schema verification a step to PREVENT or to FIX an invalid editorState.

In other words, if an error is found at the schema validation stage, two things can be done:

In the second case, we would be making the schema verification stage not have the objective of 'verifying' a schema to prevent it from being invalid, but rather to correct it. But... this is the responsibility of the normalization/transformation stage, and if we call it again, it could generate infinite loops. image

Clarifications

@EgonBolton Wow - just got done reading over the Prosemirror documentation for Schemas. Certainly flexible and powerful, but perhaps a bit too flexible and powerful. I'd like to think we can create something significantly simpler than that and still address the vast majority of use cases.

What do you think when you see a slightly simplified version like this?

const initialConfig = {
    namespace: "LexicalEditor",
    nodes: [
       text: {},
       root: {content: "(paragraph | heading | code | quote | listNode | table | decorator)*" },
       listNode: {content: "listItem*"},
       listItem: { content: "(listNode | text | link)*" }, // instead of "listNode* | (text | link)*"
       table: {content: "tableRow*"},
       tableRow: {content: "tableCell*"},
       tableCell: {content: "(paragraph | heading | code | quote | listNode | table | decorator)*" },
       code: { "codeHighlightNode*" },
       codeHighlightNode: {},
       decorator: {},
       link: {content: "text*"}, // instead of text{1}
    ],
    onError,
    theme,
  };

In other words, it could initially support defining which types of nodes can be children of each node, and later support operators like {n}, +, ?. (following Prosemirror API) Actually, I think using regex the implementation would be the same, but maybe it looks easier when you think in those terms.

Easier to detect unwanted behaviors in advance. It would not be necessary to have to anticipate all the possible ways to arrive at an invalid editor.

I don't understand why this would be the case. Can you elaborate on what you have in mind. I'm almost certain I'll implement it in the transform stage, but I want to hear what you're thinking here.

I hope it has been clearer with what I explained above. What I was referring to here is that if the schema verification stage could call the normalization stage, it would be "Easier to detect unwanted behaviors in advance. It would not be necessary to have to anticipate all the possible ways to arrive at an invalid editor". But it would bring all the other problems… If instead the schema verification stage only verify/prevents, the user will have to detect when there are blocking operations and write a corresponding transformation that will be executed before, in the Normalization/transformation stage.

acywatson commented 1 year ago

@EgonBolton I think it's a bit clearer, but in implementation those two stages can possibly just be in the transformation stage of the lifecycle. Otherwise, AFAICT, this comment can mostly be distilled down to "conflicting normalizations can create infinite loops". This is, of course, true, but this is a problem with transforms, generally, and not specific to schemas or the normalization use case.

I guess the idea is that a user should be able to define a schema without defining a normalization

It's not clear to me why you would want to do this. The only way it makes sense (to me - show me where I am wrong) is if you have a sort of implicit normalization, as otherwise you can only revert to the previous state, which I don't think is acceptable in use cases like copy + paste. My current perspective is that schemas are best thought of as an abstraction over transformation to serve a specific subset of transform use cases. Perhaps it make sense to fallback to the previous state in the case of an infinite loop.

What do you think when you see a slightly simplified version like this?

Definitely better and simpler, but I would love to avoid dealing with all of this string parsing. The original design makes validation functional so that you have more flexibility without the additional complexity of a separate syntax. I'm currently experimenting with this locally to see if we can avoid the complexity. At the same time, I do see how, once you familiarize with this syntax, it can be pretty straightforward and expressive.

GermanJablo commented 1 year ago

"conflicting normalizations can create infinite loops". This is, of course, true, but this is a problem with transforms

That was a question that arose to me while I wrote this. The transformations are executed again on the transformed editorstate?

I guess the idea is that a user should be able to define a schema without defining a normalization

It's not clear to me why you would want to do this. The only way it makes sense (to me - show me where I am wrong) is if you have a sort of implicit normalization...

There are many cases where blocking the operation is what you want. If I want each document to begin with a title, I could only do: root: {content: "heading{1} element*"} and that would resolve many things without normalization. In Prosemirror it works well.

The problem of schema is a subset of the normalization problem. In both cases you need to know that the editorstate is invalid, but in the schema you do not worry about how to solve it. That is why I visualize a Schema API as something simpler than a normalization/transformation API.

Anyway, don't worry about that. I think that if I wanted to implement my own Schema right now I could discard pendingEditorState in a transformation if it does not meet certain conditions. Reading the comment of @tylerjbainbridge and due to how the term schema is used in other editors I thought you wanted to do something like that.

What has left me something confused is that I thought that the problem of normalization was already solved with the transformation API, so at this point I no longer understand what this PR will offer. Is it a set of default transformations?

acywatson commented 1 year ago

There are many cases where blocking the operation is what you want. If I want each document to begin with a title, I could only do: root: {content: "heading{1} element*"} and that would resolve many things without normalization. In Prosemirror it works well.

So if I paste some HTML into an editor configured this way and let's say it's a single list-item or even an h1, it will just be rejected? I can't see how that makes any sense. You'd want to normalize the content in some way to fit it into the heading node. How will the user know exactly what content they have on their clipboard when every surface handles data transfer slightly differently?

That was a question that arose to me while I wrote this. The transformations are executed again on the transformed editorstate?

Yes, that's right. They continue to be executed until there are no more dirty nodes to execute them on (nodes can be marked dirty during the transforms). This is why we have infinite loop protections in place here, in the core itself.

What has left me something confused is that I thought that the problem of normalization was already solved with the transformation API, so at this point I no longer understand what this PR will offer. Is it a set of default transformations?

I think this is the right question, and what I was trying to get at in my last comment. The only way I can see to think about "schemas" is as an ergonomic abstraction over transforms. Just a way to express what would otherwise be more cumbersome transforms to write. ProseMirror is a bit different, as their schemas are doing a lot of the work that our node heuristics are doing - even down to determining what DOM elements are rendered. For us, this configuration is functional and distributed rather than static and centralized.

I'm going to do some more research, but I still don't see a value add in schemas without normalization. Does ProseMirror truly just reject inserted content that doesn't match the schema? Do you have to manually normalize the content before the schemas validation step or something?

Awjin commented 1 year ago

Interesting. Are the ergonomic benefits of a schema worth the added API surface?

abelsj60 commented 1 year ago

@acywatson You get a lot of bug reports and complaints, so have a nice, wide perspective on these issues.

From my narrow vantage point, I can only say that the turns in this conversation continue to make me think the value of a schema could be in helping users to assemble complicated — often meaning nested — node structures at creation.

I was unable to use transforms in LinedCodeNode because they happened after creation ran.

This made it impossible to use them to properly configure the initial set of nodes. And it spiraled.

I believe I ran into similarly insurmountable problems using mutations for this — not to mention the fact that the most natural way to think of assembling a complicated node is to just assume $createMyComplicatedNode(options) will work.

This is why I moved to using (a) the override API, (b) .append, and (c) a custom internal paste method, to direct the creation of a nested node from the outset of node creation — $createLinedCodeNode(options)

At this point in time, no options are set and the entire structure can be assembled cleanly.

To me, this represents a useful path for a schema — to be able to offer some kind of "map" (I mean this in a plain English way) that directs how nodes are created and combined when $createMyComplicatedNode() runs.

I realize paste, export, and import are the complicating factors. As I said, I handled this via an internal paste function that works nicely because code is always imported as text. I don't have a better proposal for you right now (EDIT: see note below).

But, I do wonder, if you were to think of the problem as I'm suggesting, would you see a solution I can't?

Bottom line, I'm really just saying you may want to step back and think again about the point of the schema itself — enforcer or choreographer? I think the latter may be more useful, but it may also mean calling the "schema" something else.

Anyway, like I said, you have a much broader perspective on this issue, so this may all be silly.


EDIT: I forgot. I should say, I do use conditions in the override API.

So, for instance, when a line of code is pasted inside the LinedCodeNode, I tell the override API to replace the ParagraphNode with a LinedCodeLineNode. Same with the TextNode v. the LinedCodeTextNode.

But, if text is pasted outside the LinedCodeNode, all works as normal.

This may be one way to think of handling import, export, paste.

acywatson commented 1 year ago

@Awjin

Interesting. Are the ergonomic benefits of a schema worth the added API surface?

This is definitely something on my mind. I think the decision ultimately depends on to what extent a schema is able to more simply and reliably express some of the behavioral heuristics we're currently using for essentially this same person (like canInsertAfter or isValidParent, canBeEmpty, etc.). If we can come up with something the is easier to reason about and simplifies the core logic, it might be worth it. Additionally, if we're able to make some use cases like @abelsj60 has easier to accomplish without having to think about configuring transforms, that could be good. Currently, we solve these normalization problems internally with just a plugin that registers a bunch of transforms to deal with different common failure modes, so it'd just be a different way of expressing that.

acywatson commented 1 year ago

Interesting discussion around the original schema API design for ProseMirror:

https://discuss.prosemirror.net/t/schema-api-design/47

Overall, I get the impression so far that ProseMirror schemas (in the latest versions) are centralizing configuration of editor mechanics as a whole, as opposed to just determining how nodes can be nested. Also, I am certain that there's some normalization happening within ProseMirror itself, as otherwise pasting HTML list-items into the examples here simply wouldn't work. I don't see how this normalization behavior is defined in the schemas themselves.

GermanJablo commented 1 year ago

Does ProseMirror truly just reject inserted content that doesn't match the schema? Do you have to manually normalize the content before the schemas validation step or something?

Sorry for not answering this. Sure enough, they seem to use some heuristics for some common use cases, but for another they offer a normalization API. From the documentation:

The order in which your nodes appear in an or-expression is significant. When creating a default instance for a non-optional node, for example to make sure a document still conforms to the schema after a replace step the first type in the expression will be used. If that is a group, the first type in the group (determined by the order in which the group's members appear in your nodes map) is used. If I switched the positions of "paragraph" and "blockquote" in the the example schema, you'd get a stack overflow as soon as the editor tried to create a block node—it'd create a "blockquote" node, whose content requires at least one block, so it'd try to create another "blockquote" as content, and so on.

Not every node-manipulating function in the library checks that it is dealing with valid content—higher level concepts like transforms do, but primitive node-creation methods usually don't and instead put the responsibility for providing sane input on their caller. It is perfectly possible to use, for example NodeType.create, to create a node with invalid content. For nodes that are ‘open’ on the edge of slices, this is even a reasonable thing to do. There is a separate createChecked method, as well as an after-the-fact check method that can be used to assert that a given node's content is valid.

Awjin commented 1 year ago

I think the decision ultimately depends on to what extent a schema is able to more simply and reliably express some of the behavioral heuristics we're currently using for essentially this same purpose (like canInsertAfter or isValidParent, canBeEmpty, etc.). If we can come up with something that is easier to reason about and simplifies the core logic, it might be worth it.

That seems reasonable.

RE: @EgonBolton...

I thought that the problem of normalization was already solved with the transformation API, so at this point I no longer understand what this PR will offer. Is it a set of default transformations?

and @acywatson...

Currently, we solve these normalization problems internally with just a plugin that registers a bunch of transforms to deal with different common failure modes, so it'd just be a different way of expressing that.

What if instead of schemas, we leaned harder into transforms?

For example: extract insertion logic and node heuristics into transforms, and then export those transforms (along with the transforms currently registered internally) to library users. This could make it easier to reason about what Lexical is doing by default, allow users to augment/override the transforms as needed, and make cases like @abelsj60's LinedCodeNode easier to implement.

fantactuka commented 1 year ago

What if instead of schemas, we leaned harder into transforms?

Some time ago, @acywatson and I discussed this topic and transforms were a viable alternative to a new API. As I recall, one of the concerns was that they might be executed more frequently than necessary for normalization, since dirty nodes trigger transforms on their siblings, even when it's unnecessary for this scenario.

Another question is how the feature will be registered: by core or as a plugin. The first option makes it harder (impossible?) to override, while the second option makes editor setup more bloated. That said, some of normalization transforms seem to be mandatory, for example lists and tables where node structure is critical for the core logic so it would make sense to bake into @lexical/list, @lexical/table instead of exposing as separate plugins.

zurfyx commented 1 year ago

Catching up on this thread, thank you for the in-depth exploration work @EgonBolton. I quite like the proposal you're pitching. Sharing some thoughts.

The first and foremost problem we're trying to fix is validation. The fact that pasting content can result in an inconsistent document is very bad.

  1. It often leads to no (immediate) errors
  2. It's very difficult to find the root cause later

For example, we have encountered cases when a table is missing a cell. This can happen when pasting a GDoc table as it doesn't guarantee rectangular selection. Initially, Lexical will render this just fine but the plugin may eventually throw when selecting cells or inserting new rows.

I think this is in a way very similar to the early type-checking React (PropTypes)

Where my opinion differs slightly is with the API. I think Lexical would more naturally take a scoped-like function that is very similar to update and read.

registerValidate(LexicalTableCell, cellNode => {
  const parent = $getParentOrThrow(cellNode);
  return $isTableRow(parent);
});

This would run after transforms and before reconciliation as described in your chart here.

There's an obvious performance concern here, and it's the fact that we can't validate every single Node prior reconciliation. However, we can provide a very limited set of controlled API and leverage scoping to make this possible in a transparent manner for the user.

let validatedNode;
const validationDependants = new Map<LexicalNode, Array<LexicalNode>>();
export {
  $getParentOrThrow: (node: LexicalNode) => {
    invariant(node === validatedNode, 'Validation limited to immediate parent');
    const parent = $getParentOrThrow(node);
    validationDependants.set(parent.__key, [...(validationDependants.get(parent) || []), node]);
    return parent;
  }
}

Given a set of validationDependants and a list of dirtyNodes we can run the validation efficiently before the reconciliation.

If the validation fails it throws, which automatically reverts the pendingEditorState to the last current EditorState.


What about normalization?

I believe we're all on the same page here (@EgonBolton, @abelsj60, @acywatson and @fantactuka). Transforms are very flexible and very robust. In fact, we are already running a normalization algorithm internally that @fanctuma built based on transforms.

However, they can be made better and simpler for most use cases. Just like @trueadm did with registerLexicalTextEntity (a complex but common use case).

First of, I want to highlight that normalization is slightly different from validation. As @EgonBolton pointed out in his diagrams, with a validation mechanism we want to ensure that after all the transforms the result is OK whereas the normalization mechanism just describes some additional operations on the what it is rendered. That is because the combination of all operations (inc. the fact that the same plugin can run multiple transforms) can lead to an acceptable result.

It is also more feasiable to achieve an exhaustive check via validation than via normalization as normalization will naturally have to handle more cases and likely be aware of external nodes. The second means that part of the normalization may have to be delegated to third parties.

One of the pain points that normalization via transforms present is the fact that we have have to replicate the conditions on parent-children relationships as dirty nodes may exclude one of them during the update process.

function fixMissingCells(tableRowNode) {
  // Count column based on a sibling row
  const countColumns = countColumns(tableRowNode.getPreviousSibling() || tableRowNode.getNextSibling) || null;
  let currentColumns = countColumns(tableRowNode);
  while (countColumns !== null && currentColumns++ !== countColumns) {
    tableRowNode.append($createTableCell().append($createParagraphNode()));
  }
}
registerNodeTransform(TableRowNode, tableRowNode => {
  fixMissingCells(tableRowNode);
})
registerNodeTransform(TableCellNode, tableCellNode => {
  fixMissingCells(tableCellNode.getParentOrThrow() as TableRowNode);
})

Perhaps we can do something similar as the above model when it tracks relationship within the transform and tries to operate on them accordingly.

This would also correct a weakness of the transform model. Dirtiness only works on immediate parent-child relations and insert-delete operations. Deeper relationships are not tracked.

ANode -> BNode -> CNode

A transform (except for RootNode transform) can't guarantee that ANode will always have a BNode followed by CNode as their child hierarchy. CNode can't be tracked.

To do this we would leverage the RootNode transform that triggers every time, and we can give this wrapper a name (i.e. registerNormalize).


In either case, both the validation step and normalization come with performance implications (particulary the normalization). It can be a lot of added work on top of existing transforms. Currently, the most heavyweight operation seems to be render (even in the heaviest internal apps) so it's hard to predict if this will have a noticeable impact.

Additionally, the (scoped) functions would be very limited. This already happens inside the read clausure but they would be even more limited in this case as we can't possibly generalize this.

Validation can also optionally be turned off in production, just like React PropTypes.

acywatson commented 1 year ago

Thanks @zurfyx - good insights.

I have started work on this, but my API design looks quite a bit different. I think we can try to reconcile this offline.

Specifically, I'm not sure I understand the need for separate validation and normalization APIs. At the end of the day, this really moves away from the concept of a declarative schema that can model these relationships and puts the onus back on the user to define all that logic themselves inside a validation. Definitely more flexible, but arguably significantly less useful (in terms of simplifying how you implement behavior like this).

If the validation fails it throws, which automatically reverts the pendingEditorState to the last current EditorState.

This part I still don't fully understand. It seems like a non-starter to have a system that rejects the entire update operation rather than attempting some sort of remediation, for reasons I listed above. I suppose we can have separate APIs and let the user decide whether or not they want normalization or just outright failure, but I'm not quite convinced that this is the right approach.

whereas the normalization mechanism just describes some additional operations on the what it is rendered

Continuing on the above point, this doesn't have to be the case. Of course these are additional operations, but why do they have to be conceptually separate from validation? The point is to get to a valid end state, which can happen when the transform exit condition occurs (node is no longer marked dirty) OR when we detect a cycle.

GermanJablo commented 1 year ago

RE: @zurfyx Thanks for the insights. You seem to share many thoughts with my past self 😂.

Commenting on a few points:

For example, we have encountered cases when a table is missing a cell. This can happen when pasting a GDoc table as it doesn't guarantee rectangular selection.

Do you have that problem also with the experimental table? Since tableNode is a decoratorNode that defines methods like insertColumnAt instead of inheriting things like append, it seems to be robust by default.

One of the pain points that normalization via transforms present is the fact that we have to replicate the conditions on parent-children relationships as dirty nodes may exclude one of them during the update process.

If I'm not mistaken, that's only true when the schema involves a child that is neither first nor last. The example you give of the table with fixMissingCells is quite a particular case, but I think that, again, a table à la decoratorNode can solve this problem.

Dirtiness only works on immediate parent-child relations and insert-delete operations. Deeper relationships are not tracked.

ANode -> BNode -> CNode A transform (except for RootNode transform) can't guarantee that ANode will always have a BNode followed by CNode as their child hierarchy. CNode can't be tracked.

To do this we would leverage the RootNode transform that triggers every time, and we can give this wrapper a name (i.e. registerNormalize).

I think it's better to think in terms of A <-> B on the one hand, and B <-> C on the other. Otherwise, you'd be checking the entire editorState from top to bottom on every change.


I think my current position on this is:

CanRau commented 8 months ago

I'd like to add, that I think it might be nice to be able to reject an update if it doesn't adhere to the "schema", normalize/transform it or accept with the ability to let's say highlight in red and maybe even adding an info icon which explains the error and possible solutions, the last just via APIs you'd then have to react to and add/highlight whatever you like.

hope that makes sense, just skimmed the comments, haven't read everything in-depth.

GermanJablo commented 7 months ago

Hey, I want to share with you the solution that I have found to this problem, and with which I am quite satisfied:

import { useLexicalComposerContext } from "@lexical/react/LexicalComposerContext";
import { useEffect } from "react";
import { mergeRegister } from "@lexical/utils";
import { $isBlock, BulletListItem, CheckListItem, Heading, Paragraph } from "./MyNodes";
import { $isRootNode, TextNode, type LexicalNode, RootNode, type ElementNode } from "lexical";
import { $isLinkNode, AutoLinkNode, LinkNode } from "@lexical/link";
import { ImageNode } from "./ImagesPlugin/ImageNode";
import { $isListItem, type BulletListItem } from "./ListPlugin/ListNodes";

const validateParent = <T extends LexicalNode>(
  checkParent: (parent: ElementNode) => boolean,
  normalize?: (node: T) => void
) => {
  return (node: T) => {
    if (normalize) normalize(node);
    const parent = node.getParentOrThrow();
    if (!checkParent(parent))
      throw new Error(`${parent.getType()} cannot contain ${node.getType()}`);
  };
};

function normalizeListItem(node: BulletListItem) {
  const parent = node.getParentOrThrow();
  if ($isListItem(parent)) {
    parent.insertAfter(node);
    node.setIndent(parent.getIndent() + 1);
  }
}

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

  useEffect(() => {
    // prettier-ignore
    return mergeRegister(
      editor.registerNodeTransform(Paragraph, validateParent($isRootNode)),
      editor.registerNodeTransform(Heading, validateParent($isRootNode)),
      editor.registerNodeTransform(BulletListItem, validateParent($isRootNode, normalizeListItem)),
      editor.registerNodeTransform(CheckListItem, validateParent($isRootNode, normalizeListItem)),
      editor.registerNodeTransform(LinkNode, validateParent($isBlock)),
      editor.registerNodeTransform(AutoLinkNode, validateParent($isBlock)),
      editor.registerNodeTransform(ImageNode, validateParent($isRootNode)),
      editor.registerNodeTransform(TextNode, validateParent((parent) => $isBlock(parent) || $isLinkNode(parent))),
      editor.registerNodeTransform(RootNode, (root) => {
        const lastBlock = root.getLastChild();
        if (lastBlock instanceof Paragraph && !lastBlock?.__first) return;
        root.append(new Paragraph());
      })
    );
  }, [editor]);

  return null;
}

As you can see, the validateParent utility allows me in a very simple way to block any operation in which a node does not have a valid parent, as I define it.

If I need to normalize for more complex structures, as is the case with listItem, I can send a second parameter that is responsible for normalizing before validating.


But this can be improved. Currently initialConfig.nodes has the following interface:

     nodes?: ReadonlyArray<Klass<LexicalNode> | {
         replace: Klass<LexicalNode>;
         with: <T extends {
             new (...args: any): any;
         }>(node: InstanceType<T>) => LexicalNode;
     }>;

I suggest the following:

     nodes?: ReadonlyArray<Klass<LexicalNode> | {
         node: Klass<LexicalNode>;
         replace?: <T extends {  // or maybe "replacementOf:"
             new (...args: any): any;
         }>(node: InstanceType<T>) => LexicalNode;
        transform?: <T extends LexicalNode>(node: T) => void
     }>;

I put transform because it is more flexible. But everyone would be free to use syntactic sugar like my validateParent utility is.

I know there was a PR a while ago that allows transforms to be defined as a static method of the node class.

I think initialConfig is also a place where it makes sense to allow this, since then a schema could be defined in a centralized place, and not in each class as observed earlier in this thread.

And why not use the SchemaPlugin like I'm doing now? Again, to keep things centralized.

  1. For example, if I add a new node, it is very likely that I will forget to add its schema in SchemaPlugin.
  2. If I want to test a version with fewer nodes, I could simply modify the initialConfig. However right now, I also have to modify the schemaPlugin.
GermanJablo commented 6 months ago

UPDATE: I have improved validateParent to provide generic normalization as a fallback instead of throwing an error:

const validateParent = <T extends LexicalNode>(
  checkParent: (parent: ElementNode) => boolean,
  normalize?: (node: T) => void
) => {
  return (node: T) => {
    if (normalize) normalize(node);
    const parent = node.getParentOrThrow();
    if (!checkParent(parent)) {
      if ($isElementNode(node)) {
        const index = node.getIndexWithinParent();
        parent.splice(index, 1, node.getChildren());
      } else {
        node.remove();
      }
      // NORMALIZING: `${parent.getType()} cannot contain ${node.getType()}, so ${node.getType()} has been removed (or replaced by its children if it had them).`
    }
  };
};

Basically, if a node has an invalid parent and no specific normalization has been defined, that node is removed but replaced by its children instead.

If their children violate the schema again, the process is repeated for them.

It's working wonders for me ☺️

kevinansfield commented 6 months ago

I don't have any ideas to share right now for how schemas could be implemented in a generic fashion but I can share what we've had to implement to fix/normalize nodes.

In our editor we are often seeing valid block element nodes being invalidly nested inside a parent that doesn't support block elements so we wanted to make sure we weren't throwing anything away as that would cause user content or formatting to be lost. To do that we pulled our transforms out into a separate package so we can import and register them for both our client-side editor and our stripped-down server-side editor that we use for converting HTML to lexical.

The core "fix" was ensuring any invalidly nested content is pulled back up to the root node by registering a transform for each of the block nodes that we support.

Here we register the transfer and pass in a function that creates a new node "clone" of the invalidly nested node that we can move it's children to and re-insert into the top-level node list.

// fix invalid nesting of nodes
registerDenestTransform(editor, ParagraphNode, () => ($createParagraphNode())),
registerDenestTransform(editor, HeadingNode, node => ($createHeadingNode(node.getTag()))),
registerDenestTransform(editor, ExtendedHeadingNode, node => ($createHeadingNode(node.getTag()))),
registerDenestTransform(editor, QuoteNode, () => ($createQuoteNode())),
registerDenestTransform(editor, ListNode, node => ($createListNode(node.getListType(), node.getStart()))),
registerDenestTransform(editor, ListItemNode, () => ($createListItemNode())),

And here's the actual denest implementation:

// With this transform we attempt to detect and fix invalid nesting by pulling out any non-inline
// children from the passed-in node and inserting them after the node's top-level parent. We need
// to move the nested nodes rather than remove them so we don't lose any pasted/imported content.

export function denestTransform<T extends ElementNode>(node: T, createNode: CreateNodeFn<T>) {
    const children = node.getChildren();

    const hasInvalidChild = children.some((child: LexicalNode) => {
        return child.isInline && !child.isInline() && !$isListNode(child) && !$isListItemNode(child);
    });

    if (!hasInvalidChild) {
        return;
    }

    // we need a temporary detached node to hold any moved nodes otherwise
    // we can trigger an infinite loop with the transform continually
    // re-running on each child move
    const tempParagraph = $createParagraphNode();

    // we need a new node of the current node type to collect inline
    // children so we can maintain order when moving the non-inline children
    // out. Will be appended to the temp paragraph and the var replaced with a
    // new node each time we find a non-inline child
    let currentElementNode = createNode(node);

    // pull any non-inline children out into the temp paragraph
    children.forEach((child: LexicalNode) => {
        if (!$isListNode(child) && !$isListItemNode(child) && child.isInline && !child.isInline()) {
            if (currentElementNode.getChildrenSize() > 0) {
                tempParagraph.append(currentElementNode);
                currentElementNode = createNode(node);
            }
            tempParagraph.append(child);
        } else {
            currentElementNode.append(child);
        }
    });

    // append any remaining nodes from the current element node holder
    if (currentElementNode.getChildrenSize() > 0) {
        tempParagraph.append(currentElementNode);
    }

    // find the top-level parent to insert nodes after in case of deeper nesting,
    // e.g. images inside lists
    let parent = node;
    while (parent.getParent() && parent.getParent() !== $getRoot()) {
        parent = parent.getParentOrThrow();
    }

    // reverse order because we can only insertAfter the parent node
    // meaning first child needs to be inserted last to maintain order.
    tempParagraph.getChildren().reverse().forEach((child) => {
        parent.insertAfter(child);
    });

    // remove the original node - it's now empty
    node.remove();

    // clean up the temporary detached paragraph immediately as we know it's no longer needed
    tempParagraph.remove();
}

export function registerDenestTransform<T extends ElementNode>(editor: LexicalEditor, klass: Klass<T>, createNode: CreateNodeFn<T>): () => void {
    if (editor.hasNodes([klass])) {
        return editor.registerNodeTransform(klass, (node) => {
            denestTransform(node, createNode);
        });
    }

    return () => {};
}

Hopefully this can either help someone having the same issue or at least help inform any proposals for generic schema validation. For other "schema-like" transforms we ensure element alignment doesn't get set and adjacent list nodes of the same type get merged together.

We're assuming that the invalid nesting is only coming from copy/paste and server-side use of generateNodesFromDOM() as we haven't been able to replicate the issue from direct editor use.