Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic typescript #4177

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Generic typescript #4177

wants to merge 4 commits into from

Conversation

ianstormtaylor
Copy link
Owner

Description

This is an attempt at making Slate work with TypeScript generics.

Issue
Fixes: #3725
Fixes: #3416

Example

The best example is looking at the new TypeScript example on this branch. (Other examples have been purposefully simplified to have very minimal type annotations, to keep them easier to understand.)

Context

This works by changing the Editor type to be Editor<V> where V represents the "value" being edited by Slate. In the most generic editor, V would be equivalent to Element[] (since that is what is accepted as children of the editor). But in a custom editor, you might have Editor<Array<Paragraph | Quote>>.

Other Editor-and-Node-related methods have been also made generic, so for example if you use Editor.leaf(editor, path) it knows that the return value is a Text node. But more specifically, it knows that it is the text node of the type you've defined in your custom elements (with any marks you've defined).

This replaces the declaration merging approach, and provides some benefits. One of the drawbacks to declaration merging was that it was impossible to know whether you were dealing with an "unknown" or "known" element, since the underlying type was changed. Similarly, having two editors on the page with different schemas wasn't possible to represent. Hopefully this approach with generics will be able to smoothly replace the declaration merging approach. (While being easy to migrate to, since you can pass those same custom element definitions into Editor still.)

It's still a work in progress.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Apr 7, 2021

🦋 Changeset detected

Latest commit: b207bd8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
slate Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Repository owner deleted a comment from github-actions bot Apr 7, 2021
@ianstormtaylor
Copy link
Owner Author

/release:next

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2021

A new release has been made for this pull request. You can install it with yarn add [email protected].

@@ -24,39 +24,39 @@ export const HistoryEditor = {
* Check if a value is a `HistoryEditor` object.
*/

isHistoryEditor(value: any): value is HistoryEditor {
isHistoryEditor(value: any): value is HistoryEditor<Value> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed a V extends Value type parameter?

Suggested change
isHistoryEditor(value: any): value is HistoryEditor<Value> {
isHistoryEditor<V extends Value>(value: any): value is HistoryEditor<V> {

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I'm not really sure how to handle the checking functions in general (like Element.isElement, or this one). I'm not sure there's a way to accurately infer V in that case without the user passing it in? And even if they pass it in, the function itself isn't verifying that it objects any sort of value?

@ianstormtaylor ianstormtaylor mentioned this pull request Apr 13, 2021
5 tasks
@ChildishForces
Copy link

ChildishForces commented Apr 16, 2021

Excited about this branch, it makes a lot more sense than overwriting type definitions. Is there a rough ETA before a @next release?

@cmditch
Copy link
Contributor

cmditch commented Jun 8, 2021

Also pretty excited about this. Thanks for exploring this!

@LoganDark
Copy link

I'd switch to Slate in a heartbeat if it didn't require using some weird hacky global override thing -- unfortunately it looks like this PR is now broken. :/

image

@dylans dylans marked this pull request as draft August 13, 2021 11:42
@ionTea
Copy link

ionTea commented Oct 27, 2021

We would really need this as well, are there any status updates?

options: {
at?: Location | Span
match?: NodeMatch<T>
match?: NodeMatch<T & NodeOf<Editor<V>>>
Copy link
Contributor

@zbeyens zbeyens May 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
match?: NodeMatch<T & NodeOf<Editor<V>>>
match?: NodeMatch<NodeOf<Editor<V>>>

We often know what type of nodes we want to get, so it makes sense to use T as a return type.
I would remove it from match option as we generally iterate through all types of nodes

Transforms.insertFragment(editor, fragment)
},

insertNode: (node: Node) => {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would create issues like udecode/plate#936

Comment on lines +164 to +169
insertNode: (
node:
| ElementOf<Editor<V>>
| TextOf<Editor<V>>
| Array<ElementOf<Editor<V>> | TextOf<Editor<V>>>
) => {
Copy link
Contributor

@zbeyens zbeyens May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Editor or Value here will introduce errors: circular dependency or value type not matching, I found only this looser typing working:

Suggested change
insertNode: (
node:
| ElementOf<Editor<V>>
| TextOf<Editor<V>>
| Array<ElementOf<Editor<V>> | TextOf<Editor<V>>>
) => {
insertNode: <N extends Descendant>(
node: N | N[]
) => {

@@ -55,246 +58,28 @@ export interface BaseEditor {
deleteBackward: (unit: 'character' | 'word' | 'line' | 'block') => void
deleteForward: (unit: 'character' | 'word' | 'line' | 'block') => void
deleteFragment: (direction?: 'forward' | 'backward') => void
getFragment: () => Descendant[]
getFragment: () => Array<Element | Text>
Copy link
Contributor

@zbeyens zbeyens May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getFragment: () => Array<Element | Text>
getFragment: <N extends Descendant>() => N[];

Comment on lines +63 to +64
insertFragment: (fragment: Array<Element | Text>) => void
insertNode: (node: Element | Text | Array<Element | Text>) => void
Copy link
Contributor

@zbeyens zbeyens May 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
insertFragment: (fragment: Array<Element | Text>) => void
insertNode: (node: Element | Text | Array<Element | Text>) => void
insertFragment: <N extends Descendant>(fragment: N[]) => void;
insertNode: <N extends Descendant>(node: N | N[]) => void;

@zbeyens
Copy link
Contributor

zbeyens commented May 11, 2022

I unfortunately don't have the time to complete that PR, so I've forked that PR into Plate. If you're looking to use these types, see Plate 11.

@maral
Copy link

maral commented Mar 13, 2024

Is there someone kind and brave enough to lift this PR up and continue the work on it? Or is the official approach against this?

@dylans
Copy link
Collaborator

dylans commented Mar 14, 2024

Is there someone kind and brave enough to lift this PR up and continue the work on it? Or is the official approach against this?

The official approach is that we definitely want to finish this, but no one has had time to do so. Plate seems to be doing pretty well with bounties attached to work they've not had time to finish, but I haven't wanted to go that route with Slate.

At this point, a good path would probably be to look at what Plate has done, see if it makes sense to wrap this up, and if so land it. More likely there are some things to improve that were uncovered in that work.

After we land this, a decent test suite for slate-react, and some optimizations for Reach 18 and 19, I'm ready to call it 1.0. (Just in time to start thinking about switching to EditContext API if everyone supports it (currently in Chrome, waiting for indication from others): https://w3c.github.io/edit-context/ (this is a substantial change))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants