From 23458f85144e4108cd2f2f0a43495de2b5bdcec8 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sun, 20 Sep 2026 11:05:16 +0200 Subject: [PATCH] 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 | 7 ++ pnpm-lock.yaml | 10 ++- .../project-manifest-reader/test/index.ts | 43 +++++++++- .../project-manifest-writer/package.json | 4 +- .../project-manifest-writer/src/index.ts | 33 ++++++-- .../project-manifest-writer/test/index.ts | 84 +++++++++++++++++++ .../project-manifest-writer/tsconfig.json | 6 ++ pnpm11/yaml/document-sync/README.md | 14 ++++ .../yaml/document-sync/src/patchDocument.ts | 53 +++++++----- .../document-sync/test/patchDocument.test.ts | 43 ++++++++++ 10 files changed, 266 insertions(+), 31 deletions(-) create mode 100644 .changeset/preserve-package-yaml.md diff --git a/.changeset/preserve-package-yaml.md b/.changeset/preserve-package-yaml.md new file mode 100644 index 0000000000..4cbf3271bd --- /dev/null +++ b/.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). diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ec956d5f8d..e65af16e9d 100644 --- a/pnpm-lock.yaml +++ b/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:' diff --git a/pnpm11/workspace/project-manifest-reader/test/index.ts b/pnpm11/workspace/project-manifest-reader/test/index.ts index e709952829..377bf759bf 100644 --- a/pnpm11/workspace/project-manifest-reader/test/index.ts +++ b/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' }) +}) diff --git a/pnpm11/workspace/project-manifest-writer/package.json b/pnpm11/workspace/project-manifest-writer/package.json index 2ab1d1df4b..e39b50a51a 100644 --- a/pnpm11/workspace/project-manifest-writer/package.json +++ b/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:", diff --git a/pnpm11/workspace/project-manifest-writer/src/index.ts b/pnpm11/workspace/project-manifest-writer/src/index.ts index e87e1c02b7..801009877c 100644 --- a/pnpm11/workspace/project-manifest-writer/src/index.ts +++ b/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 { 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 { + 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 })) +} diff --git a/pnpm11/workspace/project-manifest-writer/test/index.ts b/pnpm11/workspace/project-manifest-writer/test/index.ts index b7a1446fb6..5ac96ff50a 100644 --- a/pnpm11/workspace/project-manifest-writer/test/index.ts +++ b/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') +}) diff --git a/pnpm11/workspace/project-manifest-writer/tsconfig.json b/pnpm11/workspace/project-manifest-writer/tsconfig.json index 88c76c5185..e8c1e8b695 100644 --- a/pnpm11/workspace/project-manifest-writer/tsconfig.json +++ b/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" } ] } diff --git a/pnpm11/yaml/document-sync/README.md b/pnpm11/yaml/document-sync/README.md index d840dc989d..a6969be428 100644 --- a/pnpm11/yaml/document-sync/README.md +++ b/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: diff --git a/pnpm11/yaml/document-sync/src/patchDocument.ts b/pnpm11/yaml/document-sync/src/patchDocument.ts index bbcd6c2909..0c13244ffc 100644 --- a/pnpm11/yaml/document-sync/src/patchDocument.ts +++ b/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): yaml.Node { +function patchSeqPrimitive (seq: yaml.YAMLSeq, target: Array, 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() + const valueToNodesMap = new Map() for (const item of seq.items) { if (item != null && !yaml.isNode(item)) { @@ -195,7 +208,7 @@ function patchSeqPrimitive (seq: yaml.YAMLSeq, target: Array 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 { 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') +})