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

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.

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.

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)

@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

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.

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?

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.