TypeScript definition should use unions type over base type

[joshuaavalon]

Describe the bug
After you parse an document, you cannot narrow down the return type but check type.

To Reproduce

const doc = yaml.parseDocument(str);
const { type } = doc.contents || {};
if (doc.contents?.type === Type.MAP) {
    // doc.contents is detected as Node
}

Expected behaviour

const doc = yaml.parseDocument(str);
const { type } = doc.contents || {};
if (doc.contents?.type === Type.MAP) {
    // doc.contents is detected as BlockMap
}

Versions (please complete the following information):

• Environment: Node.js 14
• yaml: 1.10.0 or 2.0.0-1

Additional context
This can be fixed by replacing type of Parsed.contents to union type.

type AllNodeType = AST.FlowMap | AST.BlockMap; // Union of all possible types
interface Parsed extends Document {
  contents: AllNodeType | null
  /** The schema used with the document. */
  schema: Schema
 }

This is work as long as type is not duplicated. This is pretty common in TypeScript. You can read more at the official document.

[eemeli]

A PR for this would be welcome. The same issue is probably repeated in other parts of the library as well. With #203 I'm starting to introduce more TS code into the source of yaml, but there isn't a schedule for if/when that might extend to replacing all of the current JS codebase, which necessitates these hand-crafted d.ts files.

[eemeli]

@joshuaavalon I specified the parsed content type a bit, to Scalar | YAMLMap | YAMLSeq | null. Is that enough for your purposes, or were you doing something even more specific?

[joshuaavalon]

@eemeli If you use the base type (i.e. YAMLMap) instead of the implementation type (i.e. FlowMap), TypeScript can only detected it as YAMLMap. It is better to create a type that contain all implementation types.

type Scalars = BlockFolded | BlockLiteral  | PlainValue | QuoteDouble | QuoteSingle;
type Maps = FlowMap | BlockMap;
type Seqs = FlowSeq | BlockSeq;

type AllNodeTypes = Scalars | Maps | Seqs;

From TypeScript compiler's view, if you don't state the implementation types, it does not know what is the possible implementation types.

If you want to support custom nodes better, you can even use generics:

interface Parsed<T extends AllNodeTypes = AllNodeTypes> extends Document {
  contents: T | null
  schema: Schema
 }

This will allows users to add their custom types.

[eemeli]

Ah, but there's the rub; at this level, Scalar, YAMLMap and YAMLSeq are the implementation types. The last of those, for instance, is internally constructed by calling new YAMLSeq() for both flow & block sequences. It's also perfectly fine to switch the type of a node within its supported range, so e.g. this works:

const doc = YAML.parseDocument('- one\n- two\n')
doc.contents.type = 'FLOW_SEQ'
String(doc) === '[ one, two ]\n'

[joshuaavalon]

I still think using implementation type is better. Since that use case is not documented, there is no need to expose internal api. If others want to use internal api, they can cast the type themselves.

[eemeli]

The perhaps the documentation ought to be improved to make it even clearer, as the behaviour of the stringifier is documented e.g. here: https://eemeli.org/yaml/#collections

And, as I mentioned before, in the AST, Scalar, YAMLMap and YAMLSeq are the actual implementations of the nodes. In the source JS code there are not different Node descendants for e.g. flow & block sequences, just the one YAMLSeq. This is what the TS typings are intended to convey.

[eemeli]

The docs now mention type modifications more explicitly: https://eemeli.org/yaml/#modifying-nodes