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') +})