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?

[cameracker]

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 notifications@github.com 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 notifications@github.com 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

[mikestopcontinues]

I'd also like to add that the current form of declaration merging is messing up typescript's ability to differentiate the merged Element type from the Element export. When you merge, TS seems to become blind to the shadowed value.

Example:

import {Element as SlateElement} from 'slate';

declare module 'slate' {
  interface Element {
    type: 'p' | 'blockquote' | 'h1' | 'h2' | 'h3' | 'ol' | 'ul' | 'li';
  }
}

SlateElement.isElement(null); // 'SlateElement' only refers to a type, but is being used as a value here. ts(2693)

I suspect this problem might go away if you could export Element without renaming it, but of course, then typescript thinks you're referring to DOM Element:

import {Element} from 'slate';

Element.isElement(null); // Property 'isElement' does not exist on type '{ new (): Element; prototype: Element; }'. ts(2339)

[thesunny]

@mikestopcontinues Can you make sure your TypeScript version is the most recent? I have code that uses Element.isElement and it seems to work.

Unfortunately, because we are using a lot of cutting edge TypeScript, these new types are pretty sensitive to having to be used with recent versions. Ran across a number of situations which were solved by upgrading.

Let me know either way (if it works or if it continues to fail).

[mikestopcontinues]

@thesunny Sorry for the delay. I was finally able to get back to my slate code. It seems using the CustomX interface got things working for me. Thanks!

[ianstormtaylor]

I believe this was solved by @thesunny? But feel free to comment and we can re-open if not.