Slate: Support TypeScript declaration merging

[ccorcos]

Slate: 0.58.1

I'd really like to refine the internal editor types. Typescript supposedly has support for this:

https://www.typescriptlang.org/docs/handbook/declaration-merging.html#module-augmentation

But I think its not working currently, because for example text.d.ts exports a interface Text and a const Text so I think Typescript is having trouble with that.

Ideally, I can just write this code:

declare module "slate" {
    interface Text {
        bold?: string
    }
}

And now the text type everywhere in my codebase has a bold property.

[ccorcos]

I can confirm that renaming interface Text → interface IText solves this problem.

[ccorcos]

Just to give you a better sense of what I'm going after, this code is typesafe and lets me modularlize features.

declare module "slate" {
    interface IText {
        bold?: string
    }
    interface IEditor {
        bold: ReturnType<typeof createBoldInterface>
    }
}

function createBoldInterface(editor: IEditor) {
    return {
        isActive() {
            const [match] = Editor.nodes(editor, {
                match: (n) => n.bold === true,
                universal: true,
            })
            return !!match
        },
        toggle() {
            const isActive = editor.bold.isActive()
            Transforms.setNodes(
                editor,
                { bold: isActive ? null : true },
                { match: (n) => Text.isText(n), split: true }
            )
        },
    }
}

function withBold(editor: IEditor) {
    editor.bold = createBoldInterface(editor)
    return editor
}

That said, something I'm not super clear about yet -- what's the point of having methods on the editor instance and separating out all the static Editor helper functions that accept the editor as the first argument?

[CameronAckermanSEL]

This would seem like a good improvement to me, personally

[ccorcos]

I'll make a PR

[ccorcos]

Hmm. It won't let me make a PR...

Here's what I did though: https://github.com/ianstormtaylor/slate/compare/master...ccorcos:master

I've tried to only make a minimal change (hence the last two reverts). But I think in general, it's a bad pattern to shadow variable names with types.

[TheSpyder]

Side note

it's a bad pattern to shadow variable names with types

I find this very useful, although yes it does hinder extending types. However instead of extending the built-in types, Slate methods could just support <T extends Text> etc as discussed in #3416

[ccorcos]

Yeah, I think generics would be a better solution. But it’s going to
require significantly more refactoring. Happy to help if you’d like.

On Wed, May 13, 2020 at 16:03 Andrew Herron [e-mail]
wrote:

Side note

it's a bad pattern to shadow variable names with types

I find this very useful, although yes it does hinder extending types.
However instead of extending the built-in types, Slate methods could just
support etc as discussed in #3416
https://github.com/ianstormtaylor/slate/issues/3416

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ianstormtaylor/slate/issues/3680#issuecomment-628290203,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AANWDX4MI6DIJWYGBEXLLCTRRMRMPANCNFSM4M7PTAIQ
.

[kxalex]

I can confirm that renaming interface Text → interface IText solves this problem.

The prefix I may be confusing and the whole idea of an interface of hiding implementation + not telling anyone that you are using an interface. The practice of having I in front of interfaces leads that you describe parameters, data structures with I which is wrong. It's a data structure, not interface.

If I for interfaces, then why not should use n for numbers, b for booleans and c for classes.

https://stackoverflow.com/questions/31876947/confused-about-the-interface-and-class-coding-guidelines-for-typescript

I, personally, like that slate does not use I prefix.

PS: Microsoft and Facebook do not use it for a reason.

[ccorcos]

Yeah, I don't like that pattern either (and don't do it myself). I was just
trying to come up with a small and pragmatic solution for you all.

On Wed, May 13, 2020 at 11:59 PM Oleksii Shurubura [e-mail]
wrote:

I can confirm that renaming interface Text → interface IText solves this
problem.

The prefix I may be confusing and the whole idea of an interface of
hiding implementation + not telling anyone that you are using an interface.
The practice of having I in front of interfaces leads that you describe
parameters, data structures with I which is wrong. It's a data structure,
not interface.

https://stackoverflow.com/questions/31876947/confused-about-the-interface-and-class-coding-guidelines-for-typescript

I, personally, like that slate does not use I prefix.

PS: Microsoft and Facebook do not use it for a reason.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/ianstormtaylor/slate/issues/3680#issuecomment-628430772,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AANWDXYPRH7RQQFZ6XNEL4LRROJGTANCNFSM4M7PTAIQ
.

[ccorcos]

Something I'm not super clear about yet -- what's the point of having methods on the editor instance and separating out all the static Editor helper functions that accept the editor as the first argument?

Any thoughts on this? @CameronAckermanSEL @ianstormtaylor

Looking through the slate-history package and I'm not sure I understand why undo() and redo() are editor prototype methods while isSaving(editor) is not...

[TheSpyder]

v0.50 moved towards a more compositional architecture, having less _things_ on the editor by default.

I'm not sure in general why some methods were left on the editor directly, but in the case of undo/redo it's probably so the React editor doesn't explicitly depend on slate-history but can make use of it if it's available.
https://github.com/ianstormtaylor/slate/blob/master/packages/slate-react/src/components/editable.tsx#L739-L746

Even then, though, the implementation could still be in the static method and the instance method just calls it (instead of the other way around as happens today).

[thesunny]

Declaration merging requires an interface; however, an interface doesn't support union types.

Because of this, I wonder if we should close this issue in favour of a generic type. An example is a block that can be a paragraph, heading or list. With a type, we can define it as:

type MyElement = Element &
  (
    | { type: "paragraph" }
    | { type: "heading"; level: number }
    | { type: "list-item"; depth: number }
  )

The benefit is that we can use type discrimination:

if (element.type === 'heading') {
  const level = element.level // works!
  // the following would give a useful TypeScript error
  // const depth = element.depth
}

[ccorcos]

@thesunny I agree -- is there another issue ticket for generic types or should I just modify this one? At the end of the day, I just want better types...

[thesunny]

Did some work on this and found a way that solves extensibility (from interface) and type discrimination (from types).

I think it warrants a separate Issue. Will post today.

[thesunny]

Here's the proposal that supports declaration merging and type unions with type discrimination:

https://github.com/ianstormtaylor/slate/issues/3725