feat: preserve comments when updating pnpm-workspace.yaml (#10402)

* chore: create empty new `@pnpm/yaml.document-sync` package

* feat: implement @pnpm/yaml.document-sync

* feat: preserve comments when updating `pnpm-workspace.yaml`

* fix: add missing rootProjectManifestDir field to fix test

This was causing a test to fail due to the rootProjectManifestDir being
an empty string.

The main branch doesn't have this problem because `write-yaml-file`
internally does a `path.dirname(...)` call that ends up resolving the
empty string to `.`.

● logger warns about peer dependencies when linking

    ENOENT: no such file or directory, mkdir ''

    25 |     singleQuote: true, // Prefer single quotes over double quotes
    26 |   })
    > 27 |   await fs.promises.mkdir(dir, { recursive: true })
        |   ^
    28 |   await writeFileAtomic(path.join(dir, fileName), manifestStr)
    29 | }
    30 |

    at writeManifestFile (../../workspace/manifest-writer/src/index.ts:27:3)
    at updateWorkspaceManifest (../../workspace/manifest-writer/src/index.ts:83:3)
    at writeSettings (../../config/config-writer/src/index.ts:38:3)
    at addLinkToManifest (src/link.ts:182:3)
    at src/link.ts:161:7
        at async Promise.all (index 0)
    at Module.handler (src/link.ts:159:3)
    at Object.<anonymous> (test/link.ts:300:3)%

* Update workspace/manifest-writer/src/index.ts

.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.

.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.

pkg-manager/plugin-commands-installation/test/link.ts

@@ -301,6 +301,7 @@ test('logger warns about peer dependencies when linking', async () => {
     ...DEFAULT_OPTS,
     dir: process.cwd(),
     globalPkgDir: globalDir,
+    rootProjectManifestDir: globalDir,
   }, ['linked-with-peer-deps'])
 
   expect(logger.warn).toHaveBeenCalledWith(expect.objectContaining({

pnpm-lock.yaml

@@ -9077,12 +9077,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: 6.0.0
+      yaml:
+        specifier: 'catalog:'
+        version: 2.8.1
     devDependencies:
       '@pnpm/fs.find-packages':
         specifier: workspace:*
@@ -9099,9 +9105,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:

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": ">=20.19"

workspace/manifest-writer/src/index.ts

@@ -1,11 +1,14 @@
 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 { type GLOBAL_CONFIG_YAML_FILENAME, WORKSPACE_MANIFEST_FILENAME } from '@pnpm/constants'
-import writeYamlFile from 'write-yaml-file'
+import { patchDocument } from '@pnpm/yaml.document-sync'
 import { equals } from 'ramda'
+import yaml from 'yaml'
+import writeFileAtomic from 'write-file-atomic'
 import { sortKeysByPriority } from '@pnpm/object.key-sorting'
 import {
   type Project,
@@ -17,18 +20,24 @@ export type FileName =
 
 const DEFAULT_FILENAME: FileName = WORKSPACE_MANIFEST_FILENAME
 
-async function writeManifestFile (dir: string, fileName: FileName, manifest: Partial<WorkspaceManifest>): Promise<void> {
-  manifest = sortKeysByPriority({
-    priority: { packages: 0 },
-    deep: true,
-  }, manifest)
-  return writeYamlFile(path.join(dir, 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, fileName: FileName, manifest: yaml.Document): Promise<void> {
+  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, fileName), manifestStr)
+}
+
+async function readManifestRaw (file: string): Promise<string | undefined> {
+  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: {
@@ -39,7 +48,17 @@ export async function updateWorkspaceManifest (dir: string, opts: {
   allProjects?: Project[]
 }): Promise<void> {
   const fileName = opts.fileName ?? DEFAULT_FILENAME
-  const manifest = await readWorkspaceManifest(dir, fileName) ?? {} as WorkspaceManifest
+
+  const workspaceManifestStr = await readManifestRaw(path.join(dir, 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
@@ -53,7 +72,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
       }
     }
@@ -65,7 +83,15 @@ export async function updateWorkspaceManifest (dir: string, opts: {
     await fs.promises.rm(path.join(dir, fileName))
     return
   }
-  await writeManifestFile(dir, fileName, manifest)
+
+  manifest = sortKeysByPriority({
+    priority: { packages: 0 },
+    deep: true,
+  }, manifest)
+
+  patchDocument(document, manifest)
+
+  await writeManifestFile(dir, fileName, document)
 }
 
 export interface NewCatalogs {

workspace/manifest-writer/test/updateWorkspaceManifest.test.ts

@@ -1,3 +1,4 @@
+import fs from 'fs'
 import path from 'path'
 import { WORKSPACE_MANIFEST_FILENAME } from '@pnpm/constants'
 import { tempDir } from '@pnpm/prepare-temp-dir'
@@ -43,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)

workspace/manifest-writer/tsconfig.json

@@ -33,6 +33,9 @@
     {
       "path": "../../packages/types"
     },
+    {
+      "path": "../../yaml/document-sync"
+    },
     {
       "path": "../read-manifest"
     }

workspace/read-manifest/src/index.ts

@@ -52,7 +52,7 @@ async function readManifestRaw (dir: string, cfgFileName: ConfigFileName): Promi
   }
 }
 
-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