From d18d7f341f3671f7fe47afbc769faf28e350d092 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Sat, 7 Mar 2026 23:52:12 +0100 Subject: [PATCH] feat: preserve comments when updating pnpm-workspace.yaml (#10402) Cherry-pick of 2b14c742e from main, adapted for v10: - Use CJS module type for @pnpm/yaml.document-sync - Use ramda/src/equals import style (v10 CJS) - Remove GLOBAL_CONFIG_YAML_FILENAME (v11-only) - Replace write-yaml-file with yaml + write-file-atomic + patchDocument Co-Authored-By: Brandon Cheng Co-Authored-By: Claude Opus 4.6 --- .changeset/sharp-insects-joke.md | 5 ++ .changeset/wet-rabbits-reply.md | 6 ++ .../plugin-commands-installation/test/link.ts | 1 + pnpm-lock.yaml | 16 +++++- workspace/manifest-writer/package.json | 8 ++- workspace/manifest-writer/src/index.ts | 57 +++++++++++++------ .../test/updateWorkspaceManifest.test.ts | 39 +++++++++++++ workspace/manifest-writer/tsconfig.json | 3 + workspace/read-manifest/src/index.ts | 2 +- yaml/document-sync/package.json | 8 +-- yaml/document-sync/test/tsconfig.json | 5 +- 11 files changed, 122 insertions(+), 28 deletions(-) create mode 100644 .changeset/sharp-insects-joke.md create mode 100644 .changeset/wet-rabbits-reply.md diff --git a/.changeset/sharp-insects-joke.md b/.changeset/sharp-insects-joke.md new file mode 100644 index 0000000000..faec89db01 --- /dev/null +++ b/.changeset/sharp-insects-joke.md @@ -0,0 +1,5 @@ +--- +"@pnpm/workspace.read-manifest": minor +--- + +The `validateWorkspaceManifest` function is now exported and can be used to validate whether a workspace manifest object's schema is correct. diff --git a/.changeset/wet-rabbits-reply.md b/.changeset/wet-rabbits-reply.md new file mode 100644 index 0000000000..29ccd0ca8a --- /dev/null +++ b/.changeset/wet-rabbits-reply.md @@ -0,0 +1,6 @@ +--- +"@pnpm/workspace.manifest-writer": minor +pnpm: minor +--- + +When pnpm updates the `pnpm-workspace.yaml`, comments, string formatting, and whitespace will be preserved. diff --git a/pkg-manager/plugin-commands-installation/test/link.ts b/pkg-manager/plugin-commands-installation/test/link.ts index 7c3fb17ce4..f62bbb8046 100644 --- a/pkg-manager/plugin-commands-installation/test/link.ts +++ b/pkg-manager/plugin-commands-installation/test/link.ts @@ -289,6 +289,7 @@ test('logger warns about peer dependencies when linking', async () => { ...DEFAULT_OPTS, dir: process.cwd(), globalPkgDir: globalDir, + rootProjectManifestDir: globalDir, }, ['linked-with-peer-deps']) expect(warnMock).toHaveBeenCalledWith(expect.objectContaining({ diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index edf314e9e2..d428f56b7d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -8913,12 +8913,18 @@ importers: '@pnpm/workspace.read-manifest': specifier: workspace:* version: link:../read-manifest + '@pnpm/yaml.document-sync': + specifier: workspace:* + version: link:../../yaml/document-sync ramda: specifier: 'catalog:' version: '@pnpm/ramda@0.28.1' - write-yaml-file: + write-file-atomic: specifier: 'catalog:' - version: 5.0.0 + version: 5.0.1 + yaml: + specifier: 'catalog:' + version: 2.8.1 devDependencies: '@pnpm/fs.find-packages': specifier: workspace:* @@ -8935,9 +8941,15 @@ importers: '@types/ramda': specifier: 'catalog:' version: 0.29.12 + '@types/write-file-atomic': + specifier: 'catalog:' + version: 4.0.3 read-yaml-file: specifier: 'catalog:' version: 2.1.0 + write-yaml-file: + specifier: 'catalog:' + version: 5.0.0 workspace/pkgs-graph: dependencies: diff --git a/workspace/manifest-writer/package.json b/workspace/manifest-writer/package.json index df38969624..d8267463b3 100644 --- a/workspace/manifest-writer/package.json +++ b/workspace/manifest-writer/package.json @@ -37,8 +37,10 @@ "@pnpm/object.key-sorting": "workspace:*", "@pnpm/types": "workspace:*", "@pnpm/workspace.read-manifest": "workspace:*", + "@pnpm/yaml.document-sync": "workspace:*", "ramda": "catalog:", - "write-yaml-file": "catalog:" + "write-file-atomic": "catalog:", + "yaml": "catalog:" }, "devDependencies": { "@pnpm/fs.find-packages": "workspace:*", @@ -46,7 +48,9 @@ "@pnpm/prepare-temp-dir": "workspace:*", "@pnpm/workspace.manifest-writer": "workspace:*", "@types/ramda": "catalog:", - "read-yaml-file": "catalog:" + "@types/write-file-atomic": "catalog:", + "read-yaml-file": "catalog:", + "write-yaml-file": "catalog:" }, "engines": { "node": ">=18.12" diff --git a/workspace/manifest-writer/src/index.ts b/workspace/manifest-writer/src/index.ts index ed633fceee..343f7d67aa 100644 --- a/workspace/manifest-writer/src/index.ts +++ b/workspace/manifest-writer/src/index.ts @@ -1,28 +1,37 @@ import fs from 'fs' import path from 'path' +import util from 'util' import { type Catalogs } from '@pnpm/catalogs.types' import { type ResolvedCatalogEntry } from '@pnpm/lockfile.types' -import { readWorkspaceManifest, type WorkspaceManifest } from '@pnpm/workspace.read-manifest' +import { validateWorkspaceManifest, type WorkspaceManifest } from '@pnpm/workspace.read-manifest' import { WORKSPACE_MANIFEST_FILENAME } from '@pnpm/constants' -import writeYamlFile from 'write-yaml-file' +import { patchDocument } from '@pnpm/yaml.document-sync' import equals from 'ramda/src/equals' +import yaml from 'yaml' +import writeFileAtomic from 'write-file-atomic' import { sortKeysByPriority } from '@pnpm/object.key-sorting' import { type Project, } from '@pnpm/types' -async function writeManifestFile (dir: string, manifest: Partial): Promise { - manifest = sortKeysByPriority({ - priority: { packages: 0 }, - deep: true, - }, manifest) - return writeYamlFile(path.join(dir, WORKSPACE_MANIFEST_FILENAME), manifest, { - lineWidth: -1, // This is setting line width to never wrap - blankLines: true, - noCompatMode: true, - noRefs: true, - sortKeys: false, +async function writeManifestFile (dir: string, manifest: yaml.Document): Promise { + const manifestStr = manifest.toString({ + lineWidth: 0, // This is setting line width to never wrap + singleQuote: true, // Prefer single quotes over double quotes }) + await fs.promises.mkdir(dir, { recursive: true }) + await writeFileAtomic(path.join(dir, WORKSPACE_MANIFEST_FILENAME), manifestStr) +} + +async function readManifestRaw (file: string): Promise { + try { + return (await fs.promises.readFile(file)).toString() + } catch (err) { + if (util.types.isNativeError(err) && 'code' in err && err.code === 'ENOENT') { + return undefined + } + throw err + } } export async function updateWorkspaceManifest (dir: string, opts: { @@ -32,7 +41,16 @@ export async function updateWorkspaceManifest (dir: string, opts: { cleanupUnusedCatalogs?: boolean allProjects?: Project[] }): Promise { - const manifest = await readWorkspaceManifest(dir) ?? {} as WorkspaceManifest + const workspaceManifestStr = await readManifestRaw(path.join(dir, WORKSPACE_MANIFEST_FILENAME)) + + const document = workspaceManifestStr != null + ? yaml.parseDocument(workspaceManifestStr) + : new yaml.Document() + + let manifest = document.toJSON() + validateWorkspaceManifest(manifest) + manifest ??= {} + let shouldBeUpdated = opts.updatedCatalogs != null && addCatalogs(manifest, opts.updatedCatalogs) if (opts.cleanupUnusedCatalogs) { shouldBeUpdated = removePackagesFromWorkspaceCatalog(manifest, opts.allProjects ?? []) || shouldBeUpdated @@ -71,7 +89,6 @@ export async function updateWorkspaceManifest (dir: string, opts: { if (value == null) { delete manifest[key as keyof WorkspaceManifest] } else { - // @ts-expect-error manifest[key as keyof WorkspaceManifest] = value } } @@ -92,7 +109,15 @@ export async function updateWorkspaceManifest (dir: string, opts: { await fs.promises.rm(path.join(dir, WORKSPACE_MANIFEST_FILENAME)) return } - await writeManifestFile(dir, manifest) + + manifest = sortKeysByPriority({ + priority: { packages: 0 }, + deep: true, + }, manifest) + + patchDocument(document, manifest) + + await writeManifestFile(dir, document) } export interface NewCatalogs { diff --git a/workspace/manifest-writer/test/updateWorkspaceManifest.test.ts b/workspace/manifest-writer/test/updateWorkspaceManifest.test.ts index 84288a004c..8f410d44b1 100644 --- a/workspace/manifest-writer/test/updateWorkspaceManifest.test.ts +++ b/workspace/manifest-writer/test/updateWorkspaceManifest.test.ts @@ -44,6 +44,45 @@ test('updateWorkspaceManifest updates an existing setting', async () => { }) }) +// This test is intentionally minimal and doesn't exhaustively cover every case +// of comment preservation in pnpm-workspace.yaml. +// +// The tests in @pnpm/yaml.document-sync should cover more cases and be +// sufficient. It's likely not necessary to duplicate the tests in that package. +test('updateWorkspaceManifest preserves comments', async () => { + const dir = tempDir(false) + const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME) + + const manifest = `\ +packages: + - '*' + +overrides: + bar: '2' + # This comment on foo should be preserved + foo: '3' +` + + const expected = `\ +packages: + - '*' + +overrides: + bar: '3' + baz: '1' + # This comment on foo should be preserved + foo: '2' +` + + fs.writeFileSync(filePath, manifest) + + await updateWorkspaceManifest(dir, { + updatedFields: { overrides: { foo: '2', bar: '3', baz: '1' } }, + }) + + expect(fs.readFileSync(filePath).toString()).toStrictEqual(expected) +}) + test('updateWorkspaceManifest updates allowBuilds', async () => { const dir = tempDir(false) const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME) diff --git a/workspace/manifest-writer/tsconfig.json b/workspace/manifest-writer/tsconfig.json index 7c4cd74084..3b2f3e2035 100644 --- a/workspace/manifest-writer/tsconfig.json +++ b/workspace/manifest-writer/tsconfig.json @@ -33,6 +33,9 @@ { "path": "../../packages/types" }, + { + "path": "../../yaml/document-sync" + }, { "path": "../read-manifest" } diff --git a/workspace/read-manifest/src/index.ts b/workspace/read-manifest/src/index.ts index df0913fcf5..d0ea5782e9 100644 --- a/workspace/read-manifest/src/index.ts +++ b/workspace/read-manifest/src/index.ts @@ -48,7 +48,7 @@ async function readManifestRaw (dir: string): Promise { } } -function validateWorkspaceManifest (manifest: unknown): asserts manifest is WorkspaceManifest | undefined { +export function validateWorkspaceManifest (manifest: unknown): asserts manifest is WorkspaceManifest | undefined { if (manifest === undefined || manifest === null) { // Empty or null manifest is ok return diff --git a/yaml/document-sync/package.json b/yaml/document-sync/package.json index 081646f656..35ba5aac5d 100644 --- a/yaml/document-sync/package.json +++ b/yaml/document-sync/package.json @@ -4,7 +4,7 @@ "description": "Update a YAML document to match the contents of an in-memory object.", "keywords": [ "pnpm", - "pnpm11", + "pnpm10", "patch", "yaml" ], @@ -15,7 +15,7 @@ "bugs": { "url": "https://github.com/pnpm/pnpm/issues" }, - "type": "module", + "type": "commonjs", "main": "lib/index.js", "types": "lib/index.d.ts", "exports": { @@ -30,7 +30,7 @@ "compile": "tsc --build && pnpm run lint --fix", "lint": "eslint \"src/**/*.ts\" \"test/**/*.ts\"", "prepublishOnly": "pnpm run compile", - "_test": "cross-env NODE_OPTIONS=\"$NODE_OPTIONS --experimental-vm-modules\" jest" + "_test": "jest" }, "dependencies": { "yaml": "catalog:" @@ -39,7 +39,7 @@ "@pnpm/yaml.document-sync": "workspace:*" }, "engines": { - "node": ">=20.19" + "node": ">=18.12" }, "jest": { "preset": "@pnpm/jest-config" diff --git a/yaml/document-sync/test/tsconfig.json b/yaml/document-sync/test/tsconfig.json index 67ce5e1d0e..74036126c6 100644 --- a/yaml/document-sync/test/tsconfig.json +++ b/yaml/document-sync/test/tsconfig.json @@ -2,9 +2,8 @@ "extends": "../tsconfig.json", "compilerOptions": { "noEmit": false, - "outDir": "../node_modules/.test.lib", - "rootDir": "..", - "isolatedModules": true + "outDir": "../test.lib", + "rootDir": "." }, "include": [ "**/*.ts",