Retain existing nodes when using set() in mappings & sequences (Fixes #…

…185)

BREAKING CHANGE: When overwriting a `Scalar` value with a scalar using
set(), the original node is retained.

docs/05_content_nodes.md

@@ -70,27 +70,24 @@ All of the collections provide the following accessor methods:
 | delete(key)                 | `boolean` | Removes a value from the collection. Returns `true` if the item was found and removed.                                                                                                            |
 | get(key, [keepScalar]) | `any`     | Returns item at `key`, or `undefined` if not found. By default unwraps scalar values from their surrounding node; to disable set `keepScalar` to `true` (collections are always returned intact). |
 | has(key)                    | `boolean` | Checks if the collection includes a value with the key `key`.                                                                                                                                     |
-| set(key, value)             | `any`     | Sets a value in this collection. For `!!set`, `value` needs to be a boolean to add/remove the item from the set.                                                                                  |
+| set(key, value)             | `any`     | Sets a value in this collection. For `!!set`, `value` needs to be a boolean to add/remove the item from the set. When overwriting a `Scalar` value with a scalar, the original node is retained.  |
 
 <!-- prettier-ignore -->
 ```js
-const doc = new YAML.Document()
-const map = doc.createNode({ a: 1, b: [2, 3] })
-map.add({ key: 'c', value: 4 })
-  // => map.get('c') === 4 && map.has('c') === true
-map.addIn(['b'], 5) // -> map.getIn(['b', 2]) === 5
-map.delete('c') // true
-map.deleteIn(['c', 'f']) // false
-map.get('a') // 1
-map.get(doc.createNode('a'), true) // Scalar { value: 1 }
-map.getIn(['b', 1]) // 3
-map.has('c') // false
-map.hasIn(['b', '0']) // true
-map.set('c', null)
-  // => map.get('c') === null && map.has('c') === true
-map.setIn(['c', 'x'])
-  // throws Error:
-  // Expected YAML collection at c. Remaining path: x
+const doc = new YAML.Document({ a: 1, b: [2, 3] }) // { a: 1, b: [ 2, 3 ] }
+doc.add({ key: 'c', value: 4 }) // { a: 1, b: [ 2, 3 ], c: 4 }
+doc.addIn(['b'], 5)             // { a: 1, b: [ 2, 3, 5 ], c: 4 }
+doc.set('c', 42)                // { a: 1, b: [ 2, 3, 5 ], c: 42 }
+doc.setIn(['c', 'x']) // Error: Expected YAML collection at c. Remaining path: x
+doc.delete('c')                 // { a: 1, b: [ 2, 3, 5 ] }
+doc.deleteIn(['b', 1])          // { a: 1, b: [ 2, 5 ] }
+
+doc.get('a') // 1
+doc.get('a', true) // Scalar { value: 1 }
+doc.getIn(['b', 1]) // 5
+doc.has(doc.createNode('a')) // true
+doc.has('c') // false
+doc.hasIn(['b', '0']) // true
 ```
 
 For all of these methods, the keys may be nodes or their wrapped scalar values (i.e. `42` will match `Scalar { value: 42 }`) . Keys for `!!seq` should be positive integers, or their string representations. `add()` and `set()` do not automatically call `doc.createNode()` to wrap the value.

src/ast/Scalar.js

@@ -1,6 +1,9 @@
 import { Node } from './Node.js'
 import { toJSON } from './toJSON.js'
 
+export const isScalarValue = value =>
+  !value || (typeof value !== 'function' && typeof value !== 'object')
+
 export class Scalar extends Node {
   constructor(value) {
     super()

src/ast/YAMLMap.js

@@ -1,6 +1,6 @@
 import { Collection } from './Collection.js'
 import { Pair } from './Pair.js'
-import { Scalar } from './Scalar.js'
+import { Scalar, isScalarValue } from './Scalar.js'
 
 export function findPair(items, key) {
   const k = key instanceof Scalar ? key.value : key
@@ -21,8 +21,11 @@ export class YAMLMap extends Collection {
     const prev = findPair(this.items, pair.key)
     const sortEntries = this.schema && this.schema.sortMapEntries
     if (prev) {
-      if (overwrite) prev.value = pair.value
-      else throw new Error(`Key ${pair.key} already set`)
+      if (!overwrite) throw new Error(`Key ${pair.key} already set`)
+      // For scalars, keep the old node & its comments and anchors
+      if (prev.value instanceof Scalar && isScalarValue(pair.value))
+        prev.value.value = pair.value
+      else prev.value = pair.value
     } else if (sortEntries) {
       const i = this.items.findIndex(item => sortEntries(pair, item) < 0)
       if (i === -1) this.items.push(pair)

src/ast/YAMLSeq.js

@@ -1,5 +1,5 @@
 import { Collection } from './Collection.js'
-import { Scalar } from './Scalar.js'
+import { Scalar, isScalarValue } from './Scalar.js'
 import { toJSON } from './toJSON.js'
 
 function asItemIndex(key) {
@@ -36,7 +36,9 @@ export class YAMLSeq extends Collection {
     const idx = asItemIndex(key)
     if (typeof idx !== 'number')
       throw new Error(`Expected a valid index, not ${key}.`)
-    this.items[idx] = value
+    const prev = this.items[idx]
+    if (prev instanceof Scalar && isScalarValue(value)) prev.value = value
+    else this.items[idx] = value
   }
 
   toJSON(_, ctx) {

tests/doc/collection-access.js

@@ -4,8 +4,8 @@ import { Pair } from '../../types.js'
 describe('Map', () => {
   let doc, map
   beforeEach(() => {
-    doc = new YAML.Document()
-    map = doc.createNode({ a: 1, b: { c: 3, d: 4 } })
+    doc = new YAML.Document({ a: 1, b: { c: 3, d: 4 } })
+    map = doc.contents
     expect(map.items).toMatchObject([
       { key: { value: 'a' }, value: { value: 1 } },
       {
@@ -68,7 +68,7 @@ describe('Map', () => {
   test('set with value', () => {
     map.set('a', 2)
     expect(map.get('a')).toBe(2)
-    expect(map.get('a', true)).toBe(2)
+    expect(map.get('a', true)).toMatchObject({ value: 2 })
     map.set('b', 5)
     expect(map.get('b')).toBe(5)
     map.set('c', 6)
@@ -79,20 +79,27 @@ describe('Map', () => {
   test('set with node', () => {
     map.set(doc.createNode('a'), 2)
     expect(map.get('a')).toBe(2)
-    expect(map.get('a', true)).toBe(2)
+    expect(map.get('a', true)).toMatchObject({ value: 2 })
     map.set(doc.createNode('b'), 5)
     expect(map.get('b')).toBe(5)
     map.set(doc.createNode('c'), 6)
     expect(map.get('c')).toBe(6)
     expect(map.items).toHaveLength(3)
   })
+
+  test('set scalar node with anchor', () => {
+    doc = YAML.parseDocument('a: &A value\nb: *A\n')
+    doc.set('a', 'foo')
+    expect(doc.get('a',true)).toMatchObject({ value: 'foo' })
+    expect(String(doc)).toBe('a: &A foo\nb: *A\n')
+  })
 })
 
 describe('Seq', () => {
   let doc, seq
   beforeEach(() => {
-    doc = new YAML.Document()
-    seq = doc.createNode([1, [2, 3]])
+    doc = new YAML.Document([1, [2, 3]])
+    seq = doc.contents
     expect(seq.items).toMatchObject([
       { value: 1 },
       { items: [{ value: 2 }, { value: 3 }] }
@@ -150,7 +157,7 @@ describe('Seq', () => {
   test('set with value', () => {
     seq.set(0, 2)
     expect(seq.get(0)).toBe(2)
-    expect(seq.get(0, true)).toBe(2)
+    expect(seq.get(0, true)).toMatchObject({ value: 2 })
     seq.set('1', 5)
     expect(seq.get(1)).toBe(5)
     seq.set(2, 6)
@@ -161,7 +168,7 @@ describe('Seq', () => {
   test('set with node', () => {
     seq.set(doc.createNode(0), 2)
     expect(seq.get(0)).toBe(2)
-    expect(seq.get(0, true)).toBe(2)
+    expect(seq.get(0, true)).toMatchObject({ value: 2 })
     seq.set(doc.createNode('1'), 5)
     expect(seq.get(1)).toBe(5)
     seq.set(doc.createNode(2), 6)
@@ -171,14 +178,11 @@ describe('Seq', () => {
 })
 
 describe('Set', () => {
-  let doc
-  beforeAll(() => {
-    doc = new YAML.Document(null, { version: '1.1' })
-  })
-
-  let set
+  let doc, set
   beforeEach(() => {
+    doc = new YAML.Document(null, { version: '1.1' })
     set = doc.createNode([1, 2, 3], { tag: '!!set' })
+    doc.contents = set
     expect(set.items).toMatchObject([
       { key: { value: 1 }, value: { value: null } },
       { key: { value: 2 }, value: { value: null } },
@@ -222,14 +226,11 @@ describe('Set', () => {
 })
 
 describe('OMap', () => {
-  let doc
-  beforeAll(() => {
-    doc = new YAML.Document(null, { version: '1.1' })
-  })
-
-  let omap
+  let doc, omap
   beforeEach(() => {
+    doc = new YAML.Document(null, { version: '1.1' })
     omap = doc.createNode([{ a: 1 }, { b: { c: 3, d: 4 } }], { tag: '!!omap' })
+    doc.contents = omap
     expect(omap.items).toMatchObject([
       { key: { value: 'a' }, value: { value: 1 } },
       {
@@ -278,7 +279,7 @@ describe('OMap', () => {
   test('set', () => {
     omap.set('a', 2)
     expect(omap.get('a')).toBe(2)
-    expect(omap.get('a', true)).toBe(2)
+    expect(omap.get('a', true)).toMatchObject({ value: 2 })
     omap.set('b', 5)
     expect(omap.get('b')).toBe(5)
     omap.set('c', 6)
@@ -290,8 +291,8 @@ describe('OMap', () => {
 describe('Collection', () => {
   let doc, map
   beforeEach(() => {
-    doc = new YAML.Document()
-    map = doc.createNode({ a: 1, b: [2, 3] })
+    doc = new YAML.Document({ a: 1, b: [2, 3] })
+    map = doc.contents
   })
 
   test('addIn', () => {
@@ -340,7 +341,7 @@ describe('Collection', () => {
   test('setIn', () => {
     map.setIn(['a'], 2)
     expect(map.get('a')).toBe(2)
-    expect(map.get('a', true)).toBe(2)
+    expect(map.get('a', true)).toMatchObject({ value: 2 })
     map.setIn(['b', 1], 5)
     expect(map.getIn(['b', 1])).toBe(5)
     map.setIn([1], 6)
@@ -456,7 +457,7 @@ describe('Document', () => {
   test('set', () => {
     doc.set('a', 2)
     expect(doc.get('a')).toBe(2)
-    expect(doc.get('a', true)).toBe(2)
+    expect(doc.get('a', true)).toMatchObject({ value: 2 })
     doc.set('c', 6)
     expect(doc.get('c')).toBe(6)
     expect(doc.contents.items).toHaveLength(3)
@@ -468,7 +469,7 @@ describe('Document', () => {
   test('setIn', () => {
     doc.setIn(['a'], 2)
     expect(doc.getIn(['a'])).toBe(2)
-    expect(doc.getIn(['a'], true)).toBe(2)
+    expect(doc.getIn(['a'], true)).toMatchObject({ value: 2 })
     doc.setIn(['b', 1], 5)
     expect(doc.getIn(['b', 1])).toBe(5)
     doc.setIn(['c'], 6)