Slate: Please DON'T use `[key: string]: unknown`

[exports]

Reference merge request: https://github.com/ianstormtaylor/slate/pull/3566

Setting key type to unknown is a disaster for typescript users, now I have to write type cast (node as any) or node.type as any everywhere when accessing arbitrary properties, otherwise it will gives me an error and cannot be turned off by any typescript configurations. It makes me miss the old days when we are at 0.57.

Slate itself is a modular and extensive framework, so many users will assign self-defined properties to editor objects. an unknown type will leads to many annoying and redundant type casts and type checks. Write type check code everywhere with if-else block or (as any) for simply access type or bold is silly.

unknown type is evil, may be it fixes some trivial problems, but it will break the develop experience of typescript users, I have developed in typescript for three years, most of the typescript typings for open source projects don't use unknown type. I think it's better if we have [key: string]: any or other solutions.

[CameronAckermanSEL]

Edit: Thanks for the feedback, we appreciate you bringing these concerns up

The goal behind this change is captured nicely here in this stack overflow answer https://stackoverflow.com/a/51441168

This is useful for APIs that want to signal “this can be any value, so you must perform some type of checking before you use it”. This forces users to safely introspect returned values.

I have trouble believing that the intention of the API's original use of any is to deprive the user of any typechecking when they bind arbitrary values to the Element or Text objects. I hear you that the type checks are inconvenient, but type checking is part of the value typescript provides.

If there's something that can be done to make this easier to use while still providing stronger type checking than _no type checking_, that'd be cool. It's certainly not a goal to make the API harder to consume. But the most helpful thing would be to provide some alternate suggestions on how to achieve the intended goal.

[lcswillems]

What @exports seems to say is that currently, it has to replace all his node by node as any (and a lot of other cases) which is very painful. From my point of view, it makes Slate harder to use. How should we handle these unknown types?

[CameronAckermanSEL]

Yep, I totally understand what the argument exports is making.

I should first point out that responding to unknown by casting to any is in my opinion a really bad idea. If you're going to have to make a type assertion, users should assert that the property they're trying to unpack is the correct type.

There are also a number of ways to do the type assertion in one liners rather than having huge chains of "if else blocks" or the like:

// you can encapsulate your asserts in easy to read functions
function getId(id: unknown): string {
  return id as string;
}

const id = getId(node.id);

// you can inline your asserts 
const id = node.id as string;

To be clear the intention is that the keys of extra user defined properties being bound to the Element and Text objects are the ones requiring type assertions. Properties generated by slate should not be requiring these type assertions. If that is the case, that can be fixed. @exports

(Some additional feedback, it's a little hard to cut through the hyperbole and know for sure if there's a bug)

[majelbstoat]

I'm generally in favour of stricter types (and am going to have a go at generic prop types when i get the chance). this change is technically correct. but, as a real-world example due to this change, this is real code i’m having to deal with:

let fragment = Node.fragment(editor, selection)
const firstText: Text = fragment[0].children[0]

It can only ever run inside a block, so children[0] is always Text, but despite that, the code now looks like this:

let fragment = Node.fragment(editor, selection)
if (!Element.isElement(fragment[0])) return []
const firstText = fragment[0].children[0]
if (!Text.isText(firstText)) return []

I was surprised to have to do the Text one, but of course, children[0] could - per the types - be an Element or an Editor. Except, of course it can’t. And I think that's what causes the issues for me - the type system is not perfectly aligned with slate's own normalisation rules and document structure.

I don't have a strong proposal for now, but i'd like to see this impedance mismatch reduced somehow.

The minimal solution today is:

const firstText = (fragment[0] as Element).children[0] as Text

but of course, if you do that, you don't actually have type-safety - you're just subverting it.

And even if we went to full generics, and removed {[key: string]: unknown}, the example above would still require those coercions because it could technically be an Element. It probably only happened to work before because {[key: string]: any} muffled the difference.

[BrentFarese]

Here is a PR that @timbuckley began work on to incorporate generics into Slate core and alleviate potentially some of your issues. https://github.com/ianstormtaylor/slate/issues/3416. Want to pick it up and help with the conversion? We could collaborate...

As you mention generics might not solve your specific issue but then again you probably should not be relying on [key: string]: any to remove type errors that would otherwise arise if Slate had a bit better types.

[majelbstoat]

yep, down to collaborate. (and yep, know i was just getting away with it while it was any 😂) I created a #typescript channel in Slack. we can chat logistic there?

[rohmanhm]

any works better for me rather than the unknown. I need to ignore everything now that requires object access to make it works.

[thesunny]

@CameronAckermanSEL

Slate using [key: string]: unknown makes it a safer type which is the right goal; however, I wonder if it should be any until Slate enables proper typing through declaration merging or generics.

The current effect of using unknown is that developers need to use as statements as a workaround for property access.

unknown is informing us, "This isn't typed and there is no way to type it properly so please use a workaround."

I think there will be no push back when it instead communicates "This isn't typed so please type it properly."

[lcswillems]

I agree with @thesunny . Currently, having to cast all the output of functions is very painful, and may prevent new users to adopt Slate. It would be better if it was [key: string]: any or [key: string]: unknown but with declaration merging.

[CameronAckermanSEL]

A PR for moving the types to generics is in the works.

I have to say though, having merged this, there has not been that much observable push back outside of this thread. If you guys would like to see the unknown types get deprecated, I'm sure @timbuckley would appreciate a hand getting movement on the generic types PR.

[lcswillems]

How can you observe push back?

[CameronAckermanSEL]

The number of complaints we get here and on the slack channel.

[lcswillems]

And I am not sure that generics are the good way to do it too because, from my understanding, we will still have to provide generics at every call. Declaration merging might be easier to implement and easier to use.

[CameronAckermanSEL]

Sure, that's a reasonable opinion. Three responses:

1. There's a #typescript channel in slack where there's an ongoing effort for this.
2. This is being hashed out here: https://github.com/ianstormtaylor/slate/issues/3680
3. Ian is behind the generics direction. I'm unsure what his opinion is on the declaration merging.

My opinion is that typescript users need to be validating the arbitrary properties they put on the types. If people are running into this unknown constraint and still putting on any, then I have to question why they're using typescript at all. There are a number of ways to bypass the unknown type for arbitrary keys, such as casting the Element or Text types to a type that formalizes types for the properties (remember, unknown is castable to anything)

[CameronAckermanSEL]

Sort of leaning towards closing this issue in favor of the other one

[BrentFarese]

And I am not sure that generics are the good way to do it too because, from my understanding, we will still have to provide generics at every call. Declaration merging might be easier to implement and easier to use.

I think you can provide defaults to generics so that it's not a breaking change. For instance, *texts of the Node interface can have the following signature:

*texts<T extends Text = Text>(
    root: Node, 
    options: {
        from?: Path
        to?: Path
        reverse?: boolean
        pass?: (node: NodeEntry) => boolean
    } = {}
  ): Iterable<NodeEntry<T>>

This would be a non-breaking change which wouldn't force TS users to provide a generic option, only if they wanted better types (which we do!)

On declaration merging, I think Slate should support that too and it seems like an easy lift. Honestly, I would be in favor of supporting declaration merging first, removing [key: string]: unknown entirely from the codebase, and then providing documentation on how to extend types using declaration merging? Then the generics branch can proceed in the background and be released when it's ready...

What are people's thoughts on that? Declaration merging/module augmentation is not difficult at all and is a clean way to extend Slate types. It also should theoretically give the same benefit as generics because now Slate thinks that a Text node is the specific type of Text node a TS user has implemented and provided a type for using module augmentation.

[majelbstoat]

It would be worth exploring whether we can do for example createEditor<T = Text, E = Element>() and have all the methods on the editor use that custom type, so it doesn’t have to be specified on every call. Harder for the Editor/Node interfaces because they’re static methods called individually, but worth looking into.

[BrentFarese]

Update: @thesunny's comment here is a good illustration (I think) why declaration merging may not be a panacea and generics everywhere might likely be the long-term solution.

Help with the generics branch!

[thesunny]

@brendancarney

Here's a solution that solves the interface problem and also doesn't use generics everywhere. Declare only once and use everywhere:

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

[CameronAckermanSEL]

I'm going to close this. Please move further discussion into one of the other issues linked above instead.

Thanks!