feat!: Make Pair not extend NodeBase; drop its prop forwarding

BREAKING CHANGE: While the inheritance of the class is just an
implementation detail, dropping the prop forwarding means that a user
that previously set the `commentBefore`, `comment` or `spaceBefore`
property of a pair will now need to set it for the appropriate key or
value.

While adjusting the test suite for the changes, the somewhat surprising
behaviour of multi-line comments on scalar values getting handled as
`commentBefore` instead of `comment` is also dropped.

docs/05_content_nodes.md

@@ -46,7 +46,7 @@ On the other hand, `!!int` and `!!float` stringifiers will take `format` into ac
 ## Collections
 
 ```js
-class Pair<K = unknown, V = unknown> extends NodeBase {
+class Pair<K = unknown, V = unknown> {
   key: K    // When parsed, key and value are always
   value: V  // Node or null, but can be set to anything
 }

src/compose/composer.ts

@@ -1,7 +1,7 @@
 import { Directives } from '../doc/directives.js'
 import { Document } from '../doc/Document.js'
 import { ErrorCode, YAMLParseError, YAMLWarning } from '../errors.js'
-import { isCollection } from '../nodes/Node.js'
+import { isCollection, isPair } from '../nodes/Node.js'
 import {
   defaultOptions,
   DocumentOptions,
@@ -99,7 +99,8 @@ export class Composer {
       } else if (afterEmptyLine || doc.directives.marker || !dc) {
         doc.commentBefore = comment
       } else if (isCollection(dc) && !dc.flow && dc.items.length > 0) {
-        const it = dc.items[0]
+        let it = dc.items[0]
+        if (isPair(it)) it = it.key
         const cb = it.commentBefore
         it.commentBefore = cb ? `${comment}\n${cb}` : comment
       } else {

src/nodes/Pair.ts

@@ -2,8 +2,7 @@ import { createNode, CreateNodeContext } from '../doc/createNode.js'
 import { StringifyContext } from '../stringify/stringify.js'
 import { stringifyPair } from '../stringify/stringifyPair.js'
 import { addPairToJSMap } from './addPairToJSMap.js'
-import { isNode, NodeBase, PAIR } from './Node.js'
-import { Scalar } from './Scalar.js'
+import { NODE_TYPE, PAIR } from './Node.js'
 import { ToJSContext } from './toJS.js'
 
 export function createPair(
@@ -16,46 +15,19 @@ export function createPair(
   return new Pair(k, v)
 }
 
-export class Pair<K = unknown, V = unknown> extends NodeBase {
+export class Pair<K = unknown, V = unknown> {
+  readonly [NODE_TYPE]: symbol
+
   /** Always Node or null when parsed, but can be set to anything. */
   key: K
 
   /** Always Node or null when parsed, but can be set to anything. */
   value: V | null
 
   constructor(key: K, value: V | null = null) {
-    super(PAIR)
+    Object.defineProperty(this, NODE_TYPE, { value: PAIR })
     this.key = key
     this.value = value
-
-    // TS doesn't allow for accessors to override properties
-    // https://github.com/microsoft/TypeScript/pull/33509
-    Object.defineProperties(this, {
-      commentBefore: {
-        get: () => (isNode(this.key) ? this.key.commentBefore : undefined),
-        set: (cb: string | null) => {
-          if (this.key == null) this.key = (new Scalar(null) as unknown) as K
-          if (isNode(this.key)) this.key.commentBefore = cb
-          else {
-            const msg =
-              'Pair.commentBefore is an alias for Pair.key.commentBefore. To set it, the key must be a Node.'
-            throw new Error(msg)
-          }
-        }
-      },
-      spaceBefore: {
-        get: () => (isNode(this.key) ? this.key.spaceBefore : undefined),
-        set: (sb: boolean) => {
-          if (this.key == null) this.key = (new Scalar(null) as unknown) as K
-          if (isNode(this.key)) this.key.spaceBefore = sb
-          else {
-            const msg =
-              'Pair.spaceBefore is an alias for Pair.key.spaceBefore. To set it, the key must be a Node.'
-            throw new Error(msg)
-          }
-        }
-      }
-    })
   }
 
   toJSON(_?: unknown, ctx?: ToJSContext): ReturnType<typeof addPairToJSMap> {

src/schema/types.ts

@@ -100,7 +100,7 @@ export interface CollectionTag extends TagBase {
    * If returning a non-`Node` value, the output will be wrapped as a `Scalar`.
    */
   resolve(
-    value: YAMLMap | YAMLSeq,
+    value: YAMLMap.Parsed | YAMLSeq.Parsed,
     onError: (message: string) => void,
     options: ParseOptions
   ): unknown

src/schema/yaml-1.1/pairs.ts

@@ -1,13 +1,16 @@
 import type { CreateNodeContext } from '../../doc/createNode.js'
-import type { Schema } from '../../schema/Schema.js'
-import { isMap, isPair, isSeq } from '../../nodes/Node.js'
+import { isMap, isPair, isSeq, ParsedNode } from '../../nodes/Node.js'
 import { createPair, Pair } from '../../nodes/Pair.js'
+import { Scalar } from '../../nodes/Scalar.js'
 import { YAMLMap } from '../../nodes/YAMLMap.js'
 import { YAMLSeq } from '../../nodes/YAMLSeq.js'
+import type { Schema } from '../../schema/Schema.js'
 import type { CollectionTag } from '../types.js'
 
 export function resolvePairs(
-  seq: YAMLSeq | YAMLMap,
+  seq:
+    | YAMLSeq.Parsed<ParsedNode | Pair<ParsedNode, ParsedNode | null>>
+    | YAMLMap.Parsed,
   onError: (message: string) => void
 ) {
   if (isSeq(seq)) {
@@ -17,21 +20,24 @@ export function resolvePairs(
       else if (isMap(item)) {
         if (item.items.length > 1)
           onError('Each pair must have its own sequence indicator')
-        const pair = item.items[0] || new Pair(null)
+        const pair =
+          item.items[0] || new Pair(new Scalar(null) as Scalar.Parsed)
         if (item.commentBefore)
-          pair.commentBefore = pair.commentBefore
-            ? `${item.commentBefore}\n${pair.commentBefore}`
+          pair.key.commentBefore = pair.key.commentBefore
+            ? `${item.commentBefore}\n${pair.key.commentBefore}`
             : item.commentBefore
-        if (item.comment)
-          pair.comment = pair.comment
-            ? `${item.comment}\n${pair.comment}`
+        if (item.comment) {
+          const cn = pair.value || pair.key
+          cn.comment = cn.comment
+            ? `${item.comment}\n${cn.comment}`
             : item.comment
+        }
         item = pair
       }
       seq.items[i] = isPair(item) ? item : new Pair(item)
     }
   } else onError('Expected a sequence for this tag')
-  return seq as YAMLSeq<Pair>
+  return seq as YAMLSeq.Parsed<Pair<ParsedNode, ParsedNode | null>>
 }
 
 export function createPairs(

src/stringify/stringifyCollection.ts

@@ -28,29 +28,43 @@ export function stringifyCollection(
   const inFlow = flow || ctx.inFlow
   if (inFlow) itemIndent += indentStep
   ctx = Object.assign({}, ctx, { indent: itemIndent, inFlow, type: null })
-  let chompKeep = false
+  let chompKeep = false // flag for the preceding node's status
   let hasItemWithNewLine = false
   const nodes = items.reduce((nodes: StringifyNode[], item, i) => {
     let comment: string | null = null
-    if (isNode(item) || isPair(item)) {
-      if (!chompKeep && item.spaceBefore) nodes.push({ comment: true, str: '' })
-
+    if (isNode(item)) {
+      if (!chompKeep && item.spaceBefore) {
+        nodes.push({ comment: true, str: '' })
+        if (inFlow) hasItemWithNewLine = true
+      }
       if (item.commentBefore) {
         // This match will always succeed on a non-empty string
         for (const line of item.commentBefore.match(/^.*$/gm) as string[])
           nodes.push({ comment: true, str: `#${line}` })
+        if (inFlow) hasItemWithNewLine = true
+      }
+      if (item.comment) {
+        comment = item.comment
+        if (inFlow) hasItemWithNewLine = true
+      }
+    } else if (isPair(item)) {
+      if (isNode(item.key)) {
+        if (!chompKeep && item.key.spaceBefore) {
+          nodes.push({ comment: true, str: '' })
+          if (inFlow) hasItemWithNewLine = true
+        }
+        if (item.key.commentBefore) {
+          // This match will always succeed on a non-empty string
+          for (const line of item.key.commentBefore.match(/^.*$/gm) as string[])
+            nodes.push({ comment: true, str: `#${line}` })
+          if (inFlow) hasItemWithNewLine = true
+        }
+        if (inFlow && item.key.comment) hasItemWithNewLine = true
       }
-
-      if (item.comment) comment = item.comment
-
-      const pair = item as any // Apply guards manually in the following
       if (
         inFlow &&
-        ((!chompKeep && item.spaceBefore) ||
-          item.commentBefore ||
-          item.comment ||
-          (pair.key && (pair.key.commentBefore || pair.key.comment)) ||
-          (pair.value && (pair.value.commentBefore || pair.value.comment)))
+        isNode(item.value) &&
+        (item.value.commentBefore || item.value.comment)
       )
         hasItemWithNewLine = true
     }

src/stringify/stringifyPair.ts

@@ -5,9 +5,9 @@ import { addComment } from './addComment.js'
 import { stringify, StringifyContext } from './stringify.js'
 
 export function stringifyPair(
-  { comment, key, value }: Readonly<Pair>,
+  { key, value }: Readonly<Pair>,
   ctx: StringifyContext,
-  onComment?: () => void,
+  _onComment?: () => void,
   onChompKeep?: () => void
 ) {
   const {
@@ -30,7 +30,7 @@ export function stringifyPair(
   let explicitKey =
     !simpleKeys &&
     (!key ||
-      (keyComment && value == null) ||
+      (keyComment && value == null && !ctx.inFlow) ||
       isCollection(key) ||
       (isScalar(key)
         ? key.type === Scalar.BLOCK_FOLDED || key.type === Scalar.BLOCK_LITERAL
@@ -62,24 +62,13 @@ export function stringifyPair(
     (value == null && (explicitKey || ctx.inFlow))
   ) {
     str = addComment(str, ctx.indent, keyComment)
-    if (comment) {
-      if (keyComment && !comment.includes('\n'))
-        str += `\n${ctx.indent || ''}#${comment}`
-      else str = addComment(str, ctx.indent, comment)
-      if (onComment) onComment()
-    } else if (chompKeep && !keyComment && onChompKeep) onChompKeep()
+    if (chompKeep && !keyComment && onChompKeep) onChompKeep()
     return ctx.inFlow && !explicitKey ? str : `? ${str}`
   }
 
   str = explicitKey
     ? `? ${addComment(str, ctx.indent, keyComment)}\n${indent}:`
     : addComment(`${str}:`, ctx.indent, keyComment)
-  if (comment) {
-    if (keyComment && !explicitKey && !comment.includes('\n'))
-      str += `\n${ctx.indent || ''}#${comment}`
-    else str = addComment(str, ctx.indent, comment)
-    if (onComment) onComment()
-  }
 
   let vcb = ''
   let valueComment = null
@@ -94,7 +83,7 @@ export function stringifyPair(
     value = doc.createNode(value)
   }
   ctx.implicitKey = false
-  if (!explicitKey && !keyComment && !comment && isScalar(value))
+  if (!explicitKey && !keyComment && isScalar(value))
     ctx.indentAtStart = str.length + 1
   chompKeep = false
   if (
@@ -117,7 +106,7 @@ export function stringifyPair(
     () => (chompKeep = true)
   )
   let ws = ' '
-  if (vcb || keyComment || comment) {
+  if (vcb || keyComment) {
     ws = `${vcb}\n${ctx.indent}`
   } else if (!explicitKey && isCollection(value)) {
     const flow = valueStr[0] === '[' || valueStr[0] === '{'

src/stringify/stringifyString.ts

@@ -1,5 +1,4 @@
 import { Scalar } from '../nodes/Scalar.js'
-import { addCommentBefore } from './addComment.js'
 import {
   foldFlowLines,
   FoldOptions,
@@ -227,7 +226,7 @@ function plainString(
   onComment?: () => void,
   onChompKeep?: () => void
 ) {
-  const { comment, type, value } = item
+  const { type, value } = item
   const { actualString, implicitKey, indent, inFlow } = ctx
   if (
     (implicitKey && /[\n[\]{},]/.test(value)) ||
@@ -290,18 +289,9 @@ function plainString(
         return doubleQuotedString(value, ctx)
     }
   }
-  const body = implicitKey
+  return implicitKey
     ? str
     : foldFlowLines(str, indent, FOLD_FLOW, getFoldOptions(ctx))
-  if (
-    comment &&
-    !inFlow &&
-    (body.indexOf('\n') !== -1 || comment.indexOf('\n') !== -1)
-  ) {
-    if (onComment) onComment()
-    return addCommentBefore(body, indent, comment)
-  }
-  return body
 }
 
 export function stringifyString(

src/test-events.ts

@@ -93,7 +93,7 @@ function addEvents(
     }
     props = ` &${anchor}`
   }
-  if (node.tag) props += ` <${node.tag}>`
+  if (isNode(node) && node.tag) props += ` <${node.tag}>`
 
   if (isMap(node)) {
     events.push(`+MAP${props}`)