search

ianstormtaylor / slate

A completely customizable framework for building rich text editors. (Currently in beta.)
http://slatejs.org
MIT License
29.39k stars 3.22k forks source link

editor.children returns `Node[]` type but should return `Element[]` type #3744

Open thesunny opened 4 years ago

thesunny commented 4 years ago

Do you want to request a feature or report a bug?

bug

What's the current behavior?

Calling editor.children returns type Node[]

Slate: 0.58.3

What's the expected behavior?

Based on this documentation https://docs.slatejs.org/concepts/10-normalizing

We are told the following:

The top-level editor node can only contain block nodes. If any of the top-level children are inline or text nodes they will be removed. This ensures that there are always block nodes in the editor so that behaviors like "splitting a block in two" work as expected.

Therefore, editor.children should return type Element[] since it cannot be Editor[] or Text[]

mdmjg commented 4 years ago

According to this, element.children should also return Element[] or Text[], right? If that is the case, wouldn't this require an entire transformation of the Node[] interface? Since it would no longer be the child of anything? @timbuckley @BrentFarese

timbuckley commented 4 years ago

I agree that Editor.children should be of type Element[], to preserve the logic described in the docs @thesunny mentioned above.

Also, Element.children should be of type Descendant[].

This might be a quick and easy PR, worth trying. There may be a bunch of type guards in the codebase that will need to be changed to accommodate this - some may be obviated entirely.

mdmjg commented 4 years ago

Update on this: I created the following branch https://github.com/arity-contracts/slate/tree/node-change. I attempted to change the Editor.children to Element, but this led to type issues because some of the Editor functions were supposed to return Element | Text | Boolean. To solve this, I created a type that was either Text | Element | Boolean. I also tested out NodeEntry and it will throw more errors as well. I believe that this change requires a lot of code refactoring but might be worth a try if anyone is interested.