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

[ChristianMurphy]

Would solution 2 be acceptable to you for customizing hast?

😬 that makes me more uneasy than xast.
hast does have custom types in the wild, most notably mdx-hast.
And the mdx community does have (moderately) broad ts adoption, with adopters at different comfort levels with ts.

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

convenient for who? 🙂
It is a way to make types extensible.
My closest experience to this, is working on/with https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-table
which is another extensible type interface.
It was not a pleasant or convenient experience, either to help maintain or adopt.

the same idea is applied to koa#DefaultContext, because Koa middleware may patch the context, which is then passed to the next middleware

Does koa/this approach in general allow patching multiple times?
For example toml nodes and directive nodes added to mdast?

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

I've been recently thinking about that.
I appreciate it as an excellent starting point, but I'm not quite happy with the extensiblility.
What I've been thinking about, adding several generics across the types, one for each of content model types
https://github.com/syntax-tree/mdast#content-model
Having each generic default to plain mdast types, and a requirement that it extends the base type, but new node types could be added to the union.

Something like

import { Root, BaseFlowContent, BaseContent, BaseListContent, BasePhrasingContent, BaseStaticPhrasingContent } from 'mdast' 
import { ListItemGFM } from 'remark-gfm'

type plainMdast = Root
type mdastWithGFMList = Root<BaseFlowContent, BaseContent, BaseListContent | ListItemGFM, BasePhrasingContent, BaseStaticPhrasingContent>

[wooorm]

...sooooo, what should we do about this? 🤷‍♂️

[ChristianMurphy]

I'm still leaning a bit towards @remcohaszing's solution 1 from #8 (comment)

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[].

---

I'm open to solution 2 (#8 (comment)) but my limited experience with type module patching has not been pleasant.
Though xast may be different from react-table, in that patching would happen at a package level, rather than needing to be done by individual adopters.
Perhaps @remcohaszing could offer more insights on how that would work at a package level, and how it would handle adding multiple node types.

---

Looking back at

What I've been thinking about, adding several generics across the types, one for each of content model types
https://github.com/syntax-tree/mdast#content-model

import { Root, BaseFlowContent, BaseContent, BaseListContent, BasePhrasingContent, BaseStaticPhrasingContent } from 'mdast' 
import { ListItemGFM } from 'remark-gfm'

type plainMdast = Root
type mdastWithGFMList = Root<BaseFlowContent, BaseContent, BaseListContent | ListItemGFM, BasePhrasingContent, BaseStaticPhrasingContent>

it avoids type patching, but it's not a great developer experience for adding custom node types either.
Needing to remember which generic position is for which content type, and adding unions for each custom node type.

---

Which brings me back to solution 1 #8 (comment) as potentially a better alternative

[remcohaszing]

I don’t have any experience working with react-table, but my experience working with koa types has been very pleasent. I think the difference is typically you’ll only want to use one Koa server, but one may want to use multiple instanced of react-table. I can imagine those types arent great to work with. Also module augmentation can be considered global state, which is usually a bad thing.

I think in Koa it works out great though:

import Koa from 'koa'
import bodyParser from 'koa-bodyparser'

const app = new Koa()
app.use(bodyParser)
app.use(async (ctx) => {
  // Defined by module augmentation in koa-bodyparser
  // $ts-expect-type any
  ctx.request.body;

  // Defined by module augmentation in koa-bodyparser
  // $ts-expect-type string
  ctx.request.rawBody;
})

I think unified ASTs have some similarities to Koa context in this regard. They sort of share a global variable where no conflicts may occur (the type property on all node types). I.e. a plugin could insert a node shaped as {"type": "root", "children": undefined}, but I’m pretty sure this will break multiple plugins, so I wouldn’t mind if AST types would define some sort of node type registry that can be augmented by plugins.

I’ll add an example based on mdast, because it actually has plugins that define custom nodes:

import * as unist from 'unist'

// @types/mdast
interface MdastNodeMap {
  root: Root
  paragraph: Paragraph
}

export type Node = MdastNodeMap[keyof MdastNodeMap]

export interface Parent extends unist.Parent {
  children: Node[]
}

export interface Root extends Parent {
  type: 'root'
}

export interface Paragraph extends Parent {
  type: 'paragraph'
}

// remark-frontmatter
import { Node } from 'unist'

export interface Yaml extends Node {
  type: 'yaml'
  value: string
}

declare module 'mdast' {
  interface MdastNodeMap {
    yaml: Yaml
  }
}

// end user code
import remark from 'remark'
import frontmatter from 'remark-frontmatter'

const mdast = remark.use(frontmatter).parse(markdown)
for (const child of children) {
  if(child.type === 'yaml') {
    console.log(
      // $ExpectType string
      child.value
    )
  }
}

// @ts-expect-error missing value property
const customYamlNode: Yaml = {
  type: 'yaml'
}

---

As for xastscript types: Whatever isn’t specifically processed, is added to the node children. This should be reflected by types, whether this means the xastscript return type (Element) is loosened or the input type is strictened.

I personally don’t really see a use case for extending xast types, so I’m leaning towards strictening the input.

[wooorm]

Especially for mdast extending node types is a thing. But also for hast (and potentially xast), raw nodes representing a string of unparsed HTML/XML are sometimes useful.

[wooorm]

@remcohaszing could you clarify what would be needed in @types/hast and mdast-util-to-hast to add raw nodes as potential children of root, element?

[wooorm]

@remcohaszing Is this now solved by the PR in DT?

[wooorm]

Closing as I think this is solved, but let us know if this needs to be reopened