fix(manifest): preserve package.yaml comments and key order (#15148)

Patch the existing package.yaml document using the shared YAML document
synchronizer and write it atomically. Preserve existing key order and scalar
aliases, and reject malformed YAML without overwriting it.

Allow the synchronizer to retain null values and empty maps for package
metadata while keeping its configuration-pruning defaults. Match scalar YAML
keys to their JSON property names so custom metadata keeps numeric and
boolean keys. Keep reader-specific null-key conversion explicit and retain
comments when replacing scalar metadata such as timestamps and binary values. Reconcile shorter
sequences and replaced collection aliases without retaining stale values.

Validate through both project-manifest reader entry points, direct writer
regressions, synchronizer tests, and the existing workspace writer suite.
The new writer regression tests fail against the original implementation.

Closes pnpm/pnpm#2008.

.changeset/preserve-package-yaml.md

@@ -0,0 +1,7 @@
+---
+"@pnpm/workspace.project-manifest-writer": patch
+"@pnpm/yaml.document-sync": patch
+"pnpm": patch
+---
+
+pnpm now preserves comments and existing key order when updating `package.yaml`. New keys are appended to their mapping [pnpm/pnpm#2008](https://github.com/pnpm/pnpm/issues/2008).

pnpm-lock.yaml

@@ -10176,21 +10176,27 @@ importers:
 
   pnpm11/workspace/project-manifest-writer:
     dependencies:
+      '@pnpm/error':
+        specifier: workspace:*
+        version: link:../../core/error
       '@pnpm/text.comments-parser':
         specifier: workspace:*
         version: link:../../text/comments-parser
       '@pnpm/types':
         specifier: workspace:*
         version: link:../../core/types
+      '@pnpm/yaml.document-sync':
+        specifier: workspace:*
+        version: link:../../yaml/document-sync
       json5:
         specifier: 'catalog:'
         version: 2.2.3
       write-file-atomic:
         specifier: 'catalog:'
         version: 7.0.1
-      write-yaml-file:
+      yaml:
         specifier: 'catalog:'
-        version: 6.0.0
+        version: 2.9.1
     devDependencies:
       '@jest/globals':
         specifier: 'catalog:'

pnpm11/workspace/project-manifest-reader/test/index.ts

@@ -5,7 +5,7 @@ import path from 'node:path'
 import { expect, test } from '@jest/globals'
 import { fixtures } from '@pnpm/test-fixtures'
 import type { ProjectManifest } from '@pnpm/types'
-import { readProjectManifest, tryReadProjectManifest } from '@pnpm/workspace.project-manifest-reader'
+import { readExactProjectManifest, readProjectManifest, tryReadProjectManifest } from '@pnpm/workspace.project-manifest-reader'
 import { temporaryDirectory } from 'tempy'
 
 const f = fixtures(import.meta.dirname)
@@ -487,3 +487,44 @@ test('canceling changes to a manifest', async () => {
   await writeProjectManifest({ name: 'foo' })
   expect(fs.readFileSync('package.json', 'utf8')).toBe(JSON.stringify({ name: 'foo' }))
 })
+
+test.each(['directory', 'exact'])('preserves package.yaml comments and dependency order through the %s reader', async (reader) => {
+  const dir = temporaryDirectory()
+  const file = path.join(dir, 'package.yaml')
+  const original = `name: example
+dependencies:
+  # runtime dependencies
+  zebra: 1.0.0 # pinned
+  alpha: 1.0.0
+`
+  await fs.promises.writeFile(file, original)
+  const { manifest, writeProjectManifest } = reader === 'exact'
+    ? await readExactProjectManifest(file)
+    : await readProjectManifest(dir)
+  manifest.dependencies!.zebra = '2.0.0'
+  manifest.dependencies!.beta = '1.0.0'
+  await writeProjectManifest(manifest)
+  expect(await fs.promises.readFile(file, 'utf8')).toBe(original.replace('zebra: 1.0.0', 'zebra: 2.0.0') + '  beta: 1.0.0\n')
+  delete manifest.dependencies!.alpha
+  await writeProjectManifest(manifest)
+  expect(await fs.promises.readFile(file, 'utf8')).toBe(original.replace('zebra: 1.0.0', 'zebra: 2.0.0').replace('  alpha: 1.0.0\n', '') + '  beta: 1.0.0\n')
+})
+
+test('preserves comments on legacy YAML scalar keys and metadata values', async () => {
+  const dir = temporaryDirectory()
+  const file = path.join(dir, 'package.yaml')
+  const original = `name: example
+metadata:
+  null: empty # null key
+  date: 2020-01-01 # timestamp value
+  data: !!binary SGVsbG8= # binary value
+`
+  await fs.promises.writeFile(file, original)
+  const { manifest, writeProjectManifest } = await readProjectManifest(dir)
+  await writeProjectManifest({ ...manifest, version: '1.0.0' })
+  const result = await fs.promises.readFile(file, 'utf8')
+  expect(result).toContain('null: empty # null key')
+  expect(result).toContain('# timestamp value')
+  expect(result).toContain('# binary value')
+  expect((await readProjectManifest(dir)).manifest).toStrictEqual({ ...manifest, version: '1.0.0' })
+})

pnpm11/workspace/project-manifest-writer/package.json

@@ -31,11 +31,13 @@
     ".test": "cross-env NODE_OPTIONS=\"$NODE_OPTIONS --experimental-vm-modules --disable-warning=ExperimentalWarning --disable-warning=DEP0169\" jest"
   },
   "dependencies": {
+    "@pnpm/error": "workspace:*",
     "@pnpm/text.comments-parser": "workspace:*",
     "@pnpm/types": "workspace:*",
+    "@pnpm/yaml.document-sync": "workspace:*",
     "json5": "catalog:",
     "write-file-atomic": "catalog:",
-    "write-yaml-file": "catalog:"
+    "yaml": "catalog:"
   },
   "devDependencies": {
     "@jest/globals": "catalog:",

pnpm11/workspace/project-manifest-writer/src/index.ts

@@ -1,16 +1,14 @@
 import { promises as fs } from 'node:fs'
 import path from 'node:path'
+import util from 'node:util'
 
+import { PnpmError } from '@pnpm/error'
 import { type CommentSpecifier, insertComments } from '@pnpm/text.comments-parser'
 import type { ProjectManifest } from '@pnpm/types'
+import { patchDocument } from '@pnpm/yaml.document-sync'
 import JSON5 from 'json5'
 import writeFileAtomic from 'write-file-atomic'
-import { writeYamlFile } from 'write-yaml-file'
-
-const YAML_FORMAT = {
-  noCompatMode: true,
-  noRefs: true,
-}
+import yaml from 'yaml'
 
 export async function writeProjectManifest (
   filePath: string,
@@ -23,7 +21,7 @@ export async function writeProjectManifest (
 ): Promise<void> {
   const fileType = filePath.slice(filePath.lastIndexOf('.') + 1).toLowerCase()
   if (fileType === 'yaml') {
-    return writeYamlFile(filePath, manifest, YAML_FORMAT)
+    return writePackageYaml(filePath, manifest)
   }
 
   await fs.mkdir(path.dirname(filePath), { recursive: true })
@@ -46,3 +44,24 @@ function stringifyJson5 (obj: object, indent: string | number, comments?: Commen
   }
   return json5
 }
+
+async function writePackageYaml (filePath: string, manifest: ProjectManifest): Promise<void> {
+  let text: string | undefined
+  try {
+    text = await fs.readFile(filePath, 'utf8')
+  } catch (err) {
+    if (!util.types.isNativeError(err) || !('code' in err) || err.code !== 'ENOENT') throw err
+  }
+  const document = text == null ? new yaml.Document() : yaml.parseDocument(text)
+  if (document.errors.length > 0) {
+    throw new PnpmError('YAML_PARSE', `${document.errors[0].message}\nin ${filePath}`)
+  }
+  patchDocument(document, manifest, {
+    stringifyKey: String,
+    preserveKeyOrder: true,
+    preserveScalarAliases: true,
+    pruneEmptyValues: false,
+  })
+  await fs.mkdir(path.dirname(filePath), { recursive: true })
+  await writeFileAtomic(filePath, document.toString({ lineWidth: 0 }))
+}

pnpm11/workspace/project-manifest-writer/test/index.ts

@@ -6,6 +6,7 @@ import { promisify } from 'node:util'
 import { expect, test } from '@jest/globals'
 import { writeProjectManifest } from '@pnpm/workspace.project-manifest-writer'
 import { temporaryDirectory } from 'tempy'
+import yaml from 'yaml'
 
 const readFile = promisify(fs.readFile)
 
@@ -21,3 +22,86 @@ test('writeProjectManifest()', async () => {
   await writeProjectManifest(path.join(dir, 'package.yaml'), { name: 'foo', version: '1.0.0' })
   expect(await readFile(path.join(dir, 'package.yaml'), 'utf8')).toBe('name: foo\nversion: 1.0.0\n')
 })
+
+test('preserves YAML comments and key order across updates, additions, and removals', async () => {
+  const file = path.join(temporaryDirectory(), 'package.yaml')
+  await fs.promises.writeFile(file, `# project
+name: example
+
+dependencies:
+  # runtime dependencies
+  zebra: '1.0.0' # pinned
+  alpha: 1.0.0
+  removed: 1.0.0
+
+scripts:
+  test: echo test # test command
+`)
+  await writeProjectManifest(file, {
+    dependencies: { alpha: '1.0.0', beta: '2.0.0', zebra: '2.0.0' },
+    name: 'example',
+    scripts: { test: 'echo test' },
+  })
+  const expected = `# project
+name: example
+
+dependencies:
+  # runtime dependencies
+  zebra: '2.0.0' # pinned
+  alpha: 1.0.0
+  beta: 2.0.0
+
+scripts:
+  test: echo test # test command
+`
+  expect(await readFile(file, 'utf8')).toBe(expected)
+  await writeProjectManifest(file, {
+    name: 'example',
+    dependencies: { alpha: '1.0.0', beta: '2.0.0', zebra: '3.0.0' },
+    scripts: { test: 'echo test' },
+  })
+  expect(await readFile(file, 'utf8')).toBe(expected.replace("'2.0.0'", "'3.0.0'"))
+})
+
+test('preserves YAML metadata with null values, empty collections, and aliases', async () => {
+  const file = path.join(temporaryDirectory(), 'package.yaml')
+  const original = `name: example
+metadata:
+  empty: {}
+  missing: null
+  list: [null, {}, []]
+  labels:
+    1: first # numeric key
+    true: enabled
+    null: empty
+dependencies:
+  zebra: &version 1.0.0 # shared version
+  alpha: *version
+`
+  await fs.promises.writeFile(file, original)
+  const manifest = yaml.parse(original)
+  manifest.version = '2.0.0'
+  await writeProjectManifest(file, manifest)
+  const result = await readFile(file, 'utf8')
+  expect(yaml.parse(result)).toStrictEqual(manifest)
+  expect(result).toContain('zebra: &version 1.0.0 # shared version')
+  expect(result).toContain('alpha: *version')
+  expect(result).toContain('1: first # numeric key')
+})
+
+test('does not overwrite invalid YAML', async () => {
+  const file = path.join(temporaryDirectory(), 'package.yaml')
+  const original = 'dependencies: [\n'
+  await fs.promises.writeFile(file, original)
+  await expect(writeProjectManifest(file, { name: 'example' })).rejects.toMatchObject({
+    code: 'ERR_PNPM_YAML_PARSE',
+    message: expect.stringContaining(file),
+  })
+  expect(await readFile(file, 'utf8')).toBe(original)
+})
+
+test('creates a YAML manifest in a missing directory', async () => {
+  const file = path.join(temporaryDirectory(), 'nested', 'package.yaml')
+  await writeProjectManifest(file, { name: 'example' })
+  expect(await readFile(file, 'utf8')).toBe('name: example\n')
+})

pnpm11/workspace/project-manifest-writer/tsconfig.json

@@ -9,11 +9,17 @@
     "../../__typings__/**/*.d.ts"
   ],
   "references": [
+    {
+      "path": "../../core/error"
+    },
     {
       "path": "../../core/types"
     },
     {
       "path": "../../text/comments-parser"
+    },
+    {
+      "path": "../../yaml/document-sync"
     }
   ]
 }

pnpm11/yaml/document-sync/README.md

@@ -60,6 +60,20 @@ qux:
   - 3
 ```
 
+For package manifests, retain null values and empty maps and keep existing keys in their original order:
+
+```ts
+patchDocument(document, target, {
+  preserveKeyOrder: true,
+  preserveScalarAliases: true,
+  pruneEmptyValues: false,
+})
+```
+
+New keys are appended to their mapping. By default, keys follow the target object's order, and null values and empty maps are pruned from existing nodes.
+
+Consumers that use a different conversion for scalar mapping keys can pass `stringifyKey`. For example, `stringifyKey: String` matches `js-yaml`, which exposes a null mapping key as the property name `"null"`. The default follows `yaml` and uses an empty string.
+
 ## Purpose
 
 This package is useful when your codebase:

pnpm11/yaml/document-sync/src/patchDocument.ts

@@ -3,6 +3,12 @@ import yaml from 'yaml'
 import { preserveScalarAliases } from './preserveScalarAliases.js'
 
 export interface PatchDocumentOptions {
+  /** Convert scalar keys to target property names. Defaults to YAML's null-to-empty-string conversion. */
+  readonly stringifyKey?: (key: unknown) => string
+  /** Keep existing map keys in their original order and append new keys. */
+  readonly preserveKeyOrder?: boolean
+  /** Remove null values and empty maps. Defaults to true for configuration files. */
+  readonly pruneEmptyValues?: boolean
   /**
    * Updating aliases is inherently ambiguous since they're not a concept in
    * JSON. The default is to unwrap and remove aliases since that's the most
@@ -44,6 +50,7 @@ export function patchDocument (document: yaml.Document, target: unknown, options
 
   const restoreAliases = options?.preserveScalarAliases ? preserveScalarAliases(document) : undefined
   document.contents = patchNode(document.contents, target, {
+    ...options,
     document,
     aliases: options?.aliases ?? 'unwrap',
   })
@@ -56,7 +63,8 @@ function patchNode (node: yaml.Node | null | undefined, target: unknown, ctx: Pa
   }
 
   if (target == null) {
-    return null
+    if (ctx.pruneEmptyValues !== false) return null
+    return yaml.isScalar(node) && node.value === target ? node : ctx.document.createNode(target)
   }
 
   if (yaml.isAlias(node)) {
@@ -100,8 +108,7 @@ function patchAlias (alias: yaml.Alias, target: unknown, ctx: PatchContext): yam
     case 'unwrap': {
       const copy = resolved.clone() as typeof resolved
       copy.anchor = undefined
-      patchNode(copy, target, ctx)
-      return copy
+      return patchNode(copy, target, ctx)
     }
   }
 }
@@ -111,12 +118,14 @@ function patchScalar (scalar: yaml.Scalar, target: unknown, ctx: PatchContext):
     return scalar
   }
 
-  if (typeof target === 'boolean' || typeof target === 'string' || typeof target === 'number') {
-    scalar.value = target
+  const replacement = ctx.document.createNode(target)
+  if (yaml.isScalar(replacement)) {
+    scalar.value = replacement.value
+    scalar.tag = replacement.tag
     return scalar
   }
 
-  return ctx.document.createNode(target)
+  return replacement
 }
 
 function patchMap (map: yaml.YAMLMap, target: unknown, ctx: PatchContext): yaml.Node | null {
@@ -124,9 +133,7 @@ function patchMap (map: yaml.YAMLMap, target: unknown, ctx: PatchContext): yaml.
     return ctx.document.createNode(target)
   }
 
-  // Intentionally return null on empty maps as well. This recursively clears
-  // empty maps in the final document.
-  if (target == null || Object.keys(target).length === 0) {
+  if (ctx.pruneEmptyValues !== false && Object.keys(target).length === 0) {
     return null
   }
 
@@ -135,22 +142,28 @@ function patchMap (map: yaml.YAMLMap, target: unknown, ctx: PatchContext): yaml.
   for (const pair of map.items) {
     // We can't update non-node types. Pairs should only contain values that are
     // non-nodes if the yaml document was modified manually after parsing.
-    if (!yaml.isScalar(pair.key) || typeof pair.key.value !== 'string') {
+    if (!yaml.isScalar(pair.key)) {
       throw new Error('Encountered unexpected non-node value: ' + String(pair.key))
     }
 
-    mapKeyToExistingPair.set(pair.key.value, pair)
+    mapKeyToExistingPair.set(ctx.stringifyKey?.(pair.key.value) ?? String(pair.key.value ?? ''), pair)
   }
 
-  map.items = Object.entries(target)
-    .map(([key, value]) => {
+  const keys = ctx.preserveKeyOrder
+    ? [...mapKeyToExistingPair.keys()].filter(key => Object.hasOwn(target, key))
+      .concat(Object.keys(target).filter(key => !mapKeyToExistingPair.has(key)))
+    : Object.keys(target)
+
+  map.items = keys
+    .map(key => {
+      const value = target[key]
       const existingPair = mapKeyToExistingPair.get(key)
 
       if (existingPair == null) {
         return ctx.document.createPair(key, value)
       }
 
-      if (!yaml.isNode(existingPair.value)) {
+      if (existingPair.value != null && !yaml.isNode(existingPair.value)) {
         throw new Error('Encountered unexpected non-node value: ' + String(existingPair.value))
       }
 
@@ -177,15 +190,15 @@ function patchSeq (seq: yaml.YAMLSeq, target: unknown, ctx: PatchContext): yaml.
   // problem becomes important in the future, it may be worth making callers to
   // pass in a getKeyForNode() function.
   return isPrimitiveList(target)
-    ? patchSeqPrimitive(seq, target)
+    ? patchSeqPrimitive(seq, target, ctx)
     : patchSeqComplex(seq, target, ctx)
 }
 
-function patchSeqPrimitive (seq: yaml.YAMLSeq, target: Array<boolean | number | string | null | undefined>): yaml.Node {
+function patchSeqPrimitive (seq: yaml.YAMLSeq, target: Array<boolean | number | string | null | undefined>, ctx: PatchContext): yaml.Node {
   // Keep track of existing nodes to reuse when building up the final list from
   // the target list. These nodes will have comments attached to them, so it's
   // important to reuse them when possible.
-  const valueToNodesMap = new Map<boolean | number | string, yaml.Scalar[]>()
+  const valueToNodesMap = new Map<boolean | number | string | null | undefined, yaml.Scalar[]>()
 
   for (const item of seq.items) {
     if (item != null && !yaml.isNode(item)) {
@@ -195,7 +208,7 @@ function patchSeqPrimitive (seq: yaml.YAMLSeq, target: Array<boolean | number |
     // We know all items in the target list are scalars. If there's a non-scalar
     // in the source list, it needs to be removed. Skip over this item so it's
     // not added to the final list.
-    if (!yaml.isScalar(item) || !isPrimitive(item.value) || item.value == null) {
+    if (!yaml.isScalar(item) || !isPrimitive(item.value) || (item.value == null && ctx.pruneEmptyValues !== false)) {
       continue
     }
 
@@ -205,7 +218,7 @@ function patchSeqPrimitive (seq: yaml.YAMLSeq, target: Array<boolean | number |
     valueToNodesMap.set(item.value, nodeList)
   }
 
-  seq.items = target.filter(item => item != null).map((item): yaml.Scalar => {
+  seq.items = target.filter(item => item != null || ctx.pruneEmptyValues === false).map((item): yaml.Scalar => {
     const existingNodesList = valueToNodesMap.get(item)
     const firstExistingItem = existingNodesList?.shift()
 
@@ -224,7 +237,7 @@ function patchSeqPrimitive (seq: yaml.YAMLSeq, target: Array<boolean | number |
 function patchSeqComplex (seq: yaml.YAMLSeq, target: unknown[], ctx: PatchContext): yaml.Node {
   const nextItems: yaml.Node[] = []
 
-  for (let i = 0; i < Math.max(seq.items.length, target.length); i++) {
+  for (let i = 0; i < target.length; i++) {
     const existingItem = seq.items[i]
     const targetItem = target[i]
 

pnpm11/yaml/document-sync/test/patchDocument.test.ts

@@ -824,3 +824,46 @@ it('preserves aliases nested beneath an alias mapping key', () => {
   expect(document.toString()).toContain('version: *version')
   expect(document.toString()).toContain('? *key\n')
 })
+
+describe('manifest preservation options', () => {
+  it('preserves existing key order recursively and appends new keys', () => {
+    const document = yaml.parseDocument('z: { z: 1, a: 2 }\na: 3\nremoved: 4\n')
+    patchDocument(document, { a: 4, b: 5, z: { a: 3, b: 4, z: 2 } }, { preserveKeyOrder: true })
+    expect(document.toString()).toBe('z: { z: 2, a: 3, b: 4 }\na: 4\nb: 5\n')
+  })
+
+  it.each([
+    { empty: {}, nil: null, values: [null, {}, [], 1] },
+    {},
+    null,
+  ])('retains empty and null values when pruning is disabled: %j', (target) => {
+    const document = yaml.parseDocument('empty: { old: value }\nnil: value\nvalues: [1, 2, 3]\n')
+    patchDocument(document, target, { pruneEmptyValues: false })
+    expect(yaml.parse(document.toString())).toStrictEqual(target)
+  })
+})
+
+it('removes trailing sequence items without inserting null when pruning is disabled', () => {
+  const document = yaml.parseDocument('values: [{ old: 1 }, { old: 2 }]\n')
+  patchDocument(document, { values: [{}] }, { pruneEmptyValues: false })
+  expect(document.toJSON()).toStrictEqual({ values: [{}] })
+})
+
+it('replaces a collection alias when its value changes type', () => {
+  const document = yaml.parseDocument('metadata: &metadata { value: 1 }\ncopy: *metadata\n')
+  patchDocument(document, { metadata: { value: 1 }, copy: 'changed' }, { pruneEmptyValues: false })
+  expect(document.toJSON()).toStrictEqual({ metadata: { value: 1 }, copy: 'changed' })
+})
+
+it('matches scalar mapping keys to their JSON property names', () => {
+  const document = yaml.parseDocument('1: one # numeric\ntrue: enabled\nnull: empty\n')
+  patchDocument(document, { 1: 'first', true: 'yes', '': 'blank' }, { preserveKeyOrder: true })
+  expect(document.toJSON()).toStrictEqual({ 1: 'first', true: 'yes', '': 'blank' })
+  expect(document.toString()).toBe('1: first # numeric\ntrue: yes\nnull: blank\n')
+})
+
+it('supports consumers that stringify null mapping keys', () => {
+  const document = yaml.parseDocument('null: empty # null key\n')
+  patchDocument(document, { null: 'updated' }, { stringifyKey: String })
+  expect(document.toString()).toBe('null: updated # null key\n')
+})