The type of Children is invalid.

[remcohaszing]

Subject of the issue

The type of Children is defined as:

type Children = string | xast.Node | number | Children[]

However, it should match the type of xast.Element.children, strings, numbers, and xast.Root.

type Children = string | number | xast.Element.children[number] | xast.Root | Children[]

Invalid child types are unprocessed, yielding an invalid xast element node, as can be seen here

Your environment

• OS: N/A
• Packages: N/A
• Env: N/A

Steps to reproduce

N/A

Expected behavior

Xastscript types only allow valid children and nodes that are treated in a special way.

Actual behavior

Xastscript types allow any unist.Node type as a child. This is unhandled and produces an invalid xast.Element.

[remcohaszing]

I decided not to create a PR yet, because this would conflict with #3.

[wooorm]

What about other nodes: comments, cdata, doctype, text?

Leaving this vague is also nice for custom nodes. E.g., raw is used in hast/HTML and might be useful here too. Markdown also often makes use of custom nodes. Of course, custom nodes need to be handled in other places and might result in crashes if not.

[remcohaszing]

What about other nodes: comments, cdata, doctype, text?

These are currently valid children: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/xast/index.d.ts#L54

Leaving this vague is also nice for custom nodes. E.g., raw is used in hast/HTML and might be useful here too. Markdown also often makes use of custom nodes. Of course, custom nodes need to be handled in other places and might result in crashes if not.

You’re talking about the xast format, which is currently as strict as I described above.

I’m talking about the fact that the types of hastscript are now incorrect. If custom types are passed as children, this also means a node is return which doesn’t match the xast.Element interface.

I think this code snippet should explain the issue. The $ExpectType comments assert the TypeScript types, but the console.log() statement shows the type is wrong.

const element = h('foo', null, { type: 'custom' })  // $ExpectType Element

let foo = element.children[0].type  // $ ExpectType 'element' | 'comment' | 'text' | 'instruction' | 'cdata';
console.log(foo)
// Logs 'custom'

xastscript.Children should match the type of xast.Element.children for unprocessed entries. This can be solved either by applying the change I proposed in the issue description, or by loosening the types of xast.Element.children to Node[].

Personally I favor the approach using stricter types. :)

[wooorm]

/cc @ChristianMurphy you may b better suited for this one

[ChristianMurphy]

Taking a step back, the question comes down to, does XAST support custom node types or not?
MDAST and HAST do, which is why they use unist.Node (for example in MDAST a yaml frontmatter node is custom, is not part of core, but is allowed).
XAST currently does not have any, nor does the parser used, sax, which does not support extensions from what I can tell.
So this may be a safe assumption?

---

as for

xast.Element.children[number]

I like the idea, reusing the type from XAST, it does make me wonder if Child or Children should be exported on the XAST side to avoid needing to reach into a type here (and elsewhere in the XAST utilities).

[wooorm]

@remcohaszing Are you proposing for your code example to crash here?:

const element = h('foo', null, { type: 'custom' })  // $ExpectType Element CRASHHH

let foo = element.children[0].type  // $ ExpectType 'element' | 'comment' | 'text' | 'instruction' | 'cdata';
console.log(foo)
// Logs 'custom'

Because IMO that’s fine usage: you’re explicitly injecting a custom node (which is a pretty useful feature: I haven’t seen it in xast yet but hast has raw nodes and hast/mdast both often get extended in plugins).

As for allowing any object in xast.Element.children: that seems like defeating the purpose of types.

Can you instead cast something on your side? (e.g., L3 in the example)

[remcohaszing]

First of all the goal of this issue: The types of children accepted by xastscript are passed directly as xast.Element.children or xast.Root.children (I forgot about root before). This means the type accepted by xastscript children should align with those defined by @types/xast.

---

Can you instead cast something on your side? (e.g., L3 in the example)

Someone could cast this to a less strict type and then cast it to the actual type. This is a common trick mostly needed to avoid type errors if types are incorrect or something else is going on cause TypeScript to infer the type improperly. Typically this is undesirable.

let foo = element.children[0] as any as CustomNode
let foo = element.children[0] as Node as CustomNode

---

Taking a step back, the question comes down to, does XAST support custom node types or not?

I think this is a proper question to start with. If not, let’s disallow custom nodes altogether.

---

I think the answer is yes. This means the type of xast.Element.children is incorrect. There are several ways to solve this.

Solution 1: Loosen children types

This is the simplest solution. Simply remove the types of xast.Element.children and xast.Root.children. Since these types inherit from unist.Parent, their children types will be Node[].

Solution 2: Allow registering custom xast element / root children

Module augmentation can be used to extend modules, namespaces, and interfaces. This is typically not something recommended, but it’s ideal for this use case.

@types/xast will add a new interface named ElementChildren. This interface will map a key to an element. This is very similar to HTMLElementTagNameMap in the official dom types. Element will use the values of this interface. The code will look like this:

export interface ElementChildren {
  element: Element;
  comment: Comment;
  text: Text;
  instruction: Instruction;
  cdata: Cdata;
}

export interface Element {
  type: 'element';

  children: ElementChildren[keyof ElementChildren][]
}

Now if a library introduces a custom node, they can register their node as a custom child by augmenting the ElementChildren interface.

export interface CustomNode {
  type: 'custom'
}

declare module 'xast' {
  interface ElementChildren {
    custom: CustomNode;
  }
}

If one would now use this library and xastscript, which allows xast.Element.children[number], they can write the following code, which is now fully type safe:

import * as x from 'xastscript';

x('foo', null, { type: 'custom' });
x('foo', null, { type: 'nonexistent' });  // $ExpectError

Of course the same should be use for Root as well. In fact, this principle could be used for any unist.Parent interface.

---

Expanding on this thought, unist.Parent could support this:

export interface Parent<ChildrenMap = Record<string, Node>> extends Node {
  children: ChildrenMap[typeof ChildrenMap][];
}

Now xast.Element could be defined as:

export interface Element extends Parent<ElementChildren> {
  type: 'element';
}

[wooorm]

I feel like I don’t know enough about TS to have a meaningful opinion here btw, and it seems like something that’s not just related to this project, but all types for unist.
Sorry! 😬

[wooorm]

I kinda like Solution 2, reading this again. But that’s quite a bit of work to all type packages.
Would be nice to get that in with syntax-tree/unist#32.

Is that something you could work on in the coming month? (due to unifiedjs/unified#121)

[ChristianMurphy]

I'd lean a bit toward solution 1, whenever possible I prefer for people to not have to modify/extend the types (except through generics).
However given how uncommon and unlikely adding custom node types to XAST is I'd be okay with the second option as well.

[wooorm]

@ChristianMurphy I am assuming we’d also want hast to have the same solution we’d pick here.
Would solution 2 be acceptable to you for customizing hast?

[remcohaszing]

I agree solution 2 feels hacky, but on the other hand it feels very convenient as well. I think this fits in very well with an extensible interface such as most unist formats. I.e. the same idea is applied to koa#DefaultContext, because Koa middleware may patch the context, which is then passed to the next middleware. This sounds fairly similar to how unified plugins work.

The same principle could be used for example to declare toml and yaml nodes as children for for remark-frontmatter.

https://github.com/remarkjs/remark-frontmatter/blob/main/types/index.d.ts#L50-L59

Currently yaml nodes are already in the mdast type definitions, whereas toml nodes are not supported at all.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/mdast/index.d.ts#L133-L135