Remove catch-all keys from Node to improve typechecking

[GuiltyDolphin]

PR: DefinitelyTyped/DefinitelyTyped#54181

A while ago, an index signature was added to Node in @types/unist (see here and here for some discussion). This causes issues however - because it appears so early on in the chain, anything extending Node will no longer be protected from type errors that would otherwise be caught. For example, if you have interface Doc extends Node { contents: string }, then you would likely expect this to error if you tried to do x.foo on x : Doc, but because of the index signature in Node, type script won't complain.

This makes it really hard to work with unified in a type safe manner - I wonder what implications there would be for switching back to the old definition of Node? I guess one deeper problem here is that by defining Node with [k: string]: string, we are saying that we expect any subtype to be able to handle any additional keys, but this doesn't make sense in a lot of cases, and is actually more restrictive than not having this clause.

Additionally, in the spec, Node isn't defined to have a catch-all index signature, it states "Specifications implementing unist are encouraged to define more fields" - but this can just be handled by making additional interfaces.

Edit: just to make it a bit more concrete as to why this is an issue - when I switched to using a version of Node without the index parameter (over at orgajs), I found that there were places that were making use of keys that would never be present - these would cause runtime errors, which is exactly what we are trying to avoid by using typescript. In removing the index signature, typescript is able to tell us that we are making invalid assumptions, making code safer and less prone to runtime errors. The index signature allows programs to silently fail... That is, until we try and use the values (then bad stuff happens).

@Rokt33r @wooorm Pinging you because you were involved in the original discussion. Any thoughts?

[wooorm]

Also /cc @ChristianMurphy and @remcohaszing

[remcohaszing]

I totally agree with @GuiltyDolphin. Custom node types can be created by inheriting unist.Node. Although removal could be considered a breaking change.

[GuiltyDolphin]

@remcohaszing Yep! In the majority of cases this "breaks", it would either indicate that a good sub-interface could be made that would clarify usage (e.g., if I had been depending on a foo attribute, then I could make a Foo extends Node interface that would make the usage clearer), OR that actually there is a bug (e.g., the foo attribute might not even be properly defined in all cases, in which case the typechecker is now going to help find that out).

[ChristianMurphy]

we are saying that we expect any subtype to be able to handle any additional keys

Right, some context here, some folks extend node types with custom attributes, which aren't on official mdast, hast, or xast node types.
The index catch all index type was added to account for that.

@remcohaszing outlined a potential alternative in syntax-tree/xastscript#8, using overrides to extend node types.

then you would likely expect this to error if you tried to do x.foo on x : Doc, but because of the index signature in Node, type script won't complain

It will, in many cases, still error because the indexed value is unknown which is incompatible with most typings.

---

that said, I am open to removing the index type from Node, overrides may provide a better path forward for plugins adding custom attributes to Node types.
And as you note, there are some limited cases where it can cause potentially type check detectable errors to be missed.

[GuiltyDolphin]

I agree that the signature being unknown catches some errors, but still lets some slip through (especially if users make use of as to assert types).

Another thing to bear in mind is that the index signature prevents the checker from narrowing properly. Consider the following:

import { Node } from 'unist';

interface Attr extends Node {
  attr: string[];
}

function attrs(x: Node | Attr): string[] {
  if ('attr' in x) {
    // error (doesn't know this is an array - wasn't able to narrow the type of `x`)
    return x.attr;
  }
  return [];
}

The checker can't narrow to x : Attr in the true branch because Node also allows an attr key. But if you use Node without the index signature:

import { Data, Position } from 'unist';

interface Node {
  type: string;
  data?: Data;
  position?: Position;
}

interface Attr extends Node {
  attr: string[];
}

function attrs(x: Node | Attr): string[] {
  if ('attr' in x) {
    // checker knows that x : Attr here
    return x.attr;
  }
  return [];
}

Then all is well.

[ChristianMurphy]

I'd recommend staying way from using in type narrowing on Node, with or without the index type being there.
the Node.type attribute is a discriminated union https://www.typescriptlang.org/docs/handbook/2/narrowing.html#discriminated-unions, works nicely with https://github.com/syntax-tree/unist-util-is, and avoids many of the quirks/pitfalls of in narrowing.

[GuiltyDolphin]

Yep - best avoided. My main worry is just with the checker not being able to warn when trying to use a key that isn't present.

What's the best way to move forwards with potentially removing the index signature? If there's agreement with moving forwards, I'm happy to help update any code that needs updating.

Thanks!

[ChristianMurphy]

I'd still be interest to hear @Rokt33r thoughts on the topic.

Given the current feedback it may also be worth opening a tentative PR updating the code at definitely typed https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/unist/index.d.ts

[GuiltyDolphin]

Sounds good, and will do!