refactor: combine addCatalogs to updateWorkspaceManifest (#9850)

.changeset/good-cycles-fix.md

@@ -0,0 +1,5 @@
+---
+"@pnpm/workspace.manifest-writer": major
+---
+
+Combine the logic of the `addCatalogs` function into the `updateWorkspaceManifest` function.

config/config-writer/src/index.ts

@@ -35,5 +35,7 @@ export async function writeSettings (opts: WriteSettingsOptions): Promise<void>
       return
     }
   }
-  await updateWorkspaceManifest(opts.workspaceDir, opts.updatedSettings)
+  await updateWorkspaceManifest(opts.workspaceDir, {
+    updatedFields: opts.updatedSettings,
+  })
 }

config/plugin-commands-config/src/configSet.ts

@@ -36,7 +36,9 @@ export async function configSet (opts: ConfigCommandOptions, key: string, value:
   }
   key = camelCase(key)
   await updateWorkspaceManifest(opts.workspaceDir ?? opts.dir, {
-    [key]: castField(value, kebabCase(key)),
+    updatedFields: ({
+      [key]: castField(value, kebabCase(key)),
+    }),
   })
 }
 

pkg-manager/plugin-commands-installation/src/installDeps.ts

@@ -22,7 +22,7 @@ import {
 } from '@pnpm/core'
 import { globalInfo, logger } from '@pnpm/logger'
 import { sequenceGraph } from '@pnpm/sort-packages'
-import { addCatalogs } from '@pnpm/workspace.manifest-writer'
+import { updateWorkspaceManifest } from '@pnpm/workspace.manifest-writer'
 import { createPkgGraph } from '@pnpm/workspace.pkgs-graph'
 import { updateWorkspaceState, type WorkspaceStateSettings } from '@pnpm/workspace.state'
 import isSubdir from 'is-subdir'
@@ -319,7 +319,9 @@ when running add/update with the --workspace option')
     if (opts.save !== false) {
       await Promise.all([
         writeProjectManifest(updatedProject.manifest),
-        updatedCatalogs && addCatalogs(opts.workspaceDir ?? opts.dir, updatedCatalogs),
+        updateWorkspaceManifest(opts.workspaceDir ?? opts.dir, {
+          updatedCatalogs,
+        }),
       ])
     }
     if (!opts.lockfileOnly) {
@@ -342,7 +344,9 @@ when running add/update with the --workspace option')
   if (opts.update === true && opts.save !== false) {
     await Promise.all([
       writeProjectManifest(updatedManifest),
-      updatedCatalogs && addCatalogs(opts.workspaceDir ?? opts.dir, updatedCatalogs),
+      updateWorkspaceManifest(opts.workspaceDir ?? opts.dir, {
+        updatedCatalogs,
+      }),
     ])
   }
   if (opts.strictDepBuilds && ignoredBuilds?.length) {

pkg-manager/plugin-commands-installation/src/recursive.ts

@@ -31,7 +31,7 @@ import {
   type ProjectRootDir,
   type ProjectRootDirRealPath,
 } from '@pnpm/types'
-import { addCatalogs } from '@pnpm/workspace.manifest-writer'
+import { updateWorkspaceManifest } from '@pnpm/workspace.manifest-writer'
 import {
   addDependenciesToPackage,
   install,
@@ -291,9 +291,9 @@ export async function recursive (
       const promises: Array<Promise<void>> = mutatedPkgs.map(async ({ originalManifest, manifest, rootDir }) => {
         return manifestsByPath[rootDir].writeProjectManifest(originalManifest ?? manifest)
       })
-      if (updatedCatalogs) {
-        promises.push(addCatalogs(opts.workspaceDir, updatedCatalogs))
-      }
+      promises.push(updateWorkspaceManifest(opts.workspaceDir, {
+        updatedCatalogs,
+      }))
       await Promise.all(promises)
     }
     if (opts.strictDepBuilds && ignoredBuilds?.length) {
@@ -439,9 +439,9 @@ export async function recursive (
     })
   ))
 
-  if (updatedCatalogs) {
-    await addCatalogs(opts.workspaceDir, updatedCatalogs)
-  }
+  await updateWorkspaceManifest(opts.workspaceDir, {
+    updatedCatalogs,
+  })
 
   if (
     !opts.lockfileOnly && !opts.ignoreScripts && (

workspace/manifest-writer/src/index.ts

@@ -22,10 +22,14 @@ async function writeManifestFile (dir: string, manifest: Partial<WorkspaceManife
   })
 }
 
-export async function updateWorkspaceManifest (dir: string, updatedFields: Partial<WorkspaceManifest>): Promise<void> {
+export async function updateWorkspaceManifest (dir: string, opts: {
+  updatedFields?: Partial<WorkspaceManifest>
+  updatedCatalogs?: Catalogs
+}): Promise<void> {
   const manifest = await readWorkspaceManifest(dir) ?? {} as WorkspaceManifest
-  let shouldBeUpdated = false
-  for (const [key, value] of Object.entries(updatedFields)) {
+  let shouldBeUpdated = opts.updatedCatalogs != null && addCatalogs(manifest, opts.updatedCatalogs)
+
+  for (const [key, value] of Object.entries(opts.updatedFields ?? {})) {
     if (!equals(manifest[key as keyof WorkspaceManifest], value)) {
       shouldBeUpdated = true
       if (value == null) {
@@ -52,8 +56,7 @@ export interface NewCatalogs {
   }
 }
 
-export async function addCatalogs (workspaceDir: string, newCatalogs: Catalogs): Promise<void> {
-  const manifest: Partial<WorkspaceManifest> = await readWorkspaceManifest(workspaceDir) ?? {}
+function addCatalogs (manifest: Partial<WorkspaceManifest>, newCatalogs: Catalogs): boolean {
   let shouldBeUpdated = false
 
   for (const catalogName in newCatalogs) {
@@ -85,7 +88,5 @@ export async function addCatalogs (workspaceDir: string, newCatalogs: Catalogs):
     }
   }
 
-  if (shouldBeUpdated) {
-    await writeManifestFile(workspaceDir, manifest)
-  }
+  return shouldBeUpdated
 }

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

@@ -2,22 +2,24 @@ import fs from 'fs'
 import path from 'path'
 import { WORKSPACE_MANIFEST_FILENAME } from '@pnpm/constants'
 import { tempDir } from '@pnpm/prepare-temp-dir'
-import { addCatalogs } from '@pnpm/workspace.manifest-writer'
+import { updateWorkspaceManifest } from '@pnpm/workspace.manifest-writer'
 import { sync as readYamlFile } from 'read-yaml-file'
 import { sync as writeYamlFile } from 'write-yaml-file'
 
 test('addCatalogs does not write new workspace manifest for empty catalogs', async () => {
   const dir = tempDir(false)
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
-  await addCatalogs(dir, {})
+  await updateWorkspaceManifest(dir, {})
   expect(fs.existsSync(filePath)).toBe(false)
 })
 
 test('addCatalogs does not write new workspace manifest for empty default catalogs', async () => {
   const dir = tempDir(false)
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
-  await addCatalogs(dir, {
-    default: {},
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      default: {},
+    },
   })
   expect(fs.existsSync(filePath)).toBe(false)
 })
@@ -25,9 +27,11 @@ test('addCatalogs does not write new workspace manifest for empty default catalo
 test('addCatalogs does not write new workspace manifest for empty any-named catalogs', async () => {
   const dir = tempDir(false)
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
-  await addCatalogs(dir, {
-    foo: {},
-    bar: {},
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      foo: {},
+      bar: {},
+    },
   })
   expect(fs.existsSync(filePath)).toBe(false)
 })
@@ -36,7 +40,9 @@ test('addCatalogs does not add empty catalogs', async () => {
   const dir = tempDir(false)
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
   writeYamlFile(filePath, {})
-  await addCatalogs(dir, {})
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {},
+  })
   expect(readYamlFile(filePath)).toStrictEqual({})
 })
 
@@ -44,8 +50,10 @@ test('addCatalogs does not add empty default catalogs', async () => {
   const dir = tempDir(false)
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
   writeYamlFile(filePath, {})
-  await addCatalogs(dir, {
-    default: {},
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      default: {},
+    },
   })
   expect(readYamlFile(filePath)).toStrictEqual({})
 })
@@ -54,9 +62,11 @@ test('addCatalogs does not add empty any-named catalogs', async () => {
   const dir = tempDir(false)
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
   writeYamlFile(filePath, {})
-  await addCatalogs(dir, {
-    foo: {},
-    bar: {},
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      foo: {},
+      bar: {},
+    },
   })
   expect(readYamlFile(filePath)).toStrictEqual({})
 })
@@ -64,9 +74,11 @@ test('addCatalogs does not add empty any-named catalogs', async () => {
 test('addCatalogs adds `default` catalogs to the `catalog` object by default', async () => {
   const dir = tempDir(false)
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
-  await addCatalogs(dir, {
-    default: {
-      foo: '^0.1.2',
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      default: {
+        foo: '^0.1.2',
+      },
     },
   })
   expect(readYamlFile(filePath)).toStrictEqual({
@@ -84,9 +96,11 @@ test('addCatalogs adds `default` catalogs to the `catalog` object if it exists',
       bar: '3.2.1',
     },
   })
-  await addCatalogs(dir, {
-    default: {
-      foo: '^0.1.2',
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      default: {
+        foo: '^0.1.2',
+      },
     },
   })
   expect(readYamlFile(filePath)).toStrictEqual({
@@ -107,9 +121,11 @@ test('addCatalogs adds `default` catalogs to the `catalogs.default` object if it
       },
     },
   })
-  await addCatalogs(dir, {
-    default: {
-      foo: '^0.1.2',
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      default: {
+        foo: '^0.1.2',
+      },
     },
   })
   expect(readYamlFile(filePath)).toStrictEqual({
@@ -125,12 +141,14 @@ test('addCatalogs adds `default` catalogs to the `catalogs.default` object if it
 test('addCatalogs creates a `catalogs` object for any-named catalogs', async () => {
   const dir = tempDir(false)
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
-  await addCatalogs(dir, {
-    foo: {
-      abc: '0.1.2',
-    },
-    bar: {
-      def: '3.2.1',
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      foo: {
+        abc: '0.1.2',
+      },
+      bar: {
+        def: '3.2.1',
+      },
     },
   })
   expect(readYamlFile(filePath)).toStrictEqual({
@@ -155,12 +173,14 @@ test('addCatalogs add any-named catalogs to the `catalogs` object if it already
       },
     },
   })
-  await addCatalogs(dir, {
-    foo: {
-      abc: '0.1.2',
-    },
-    bar: {
-      def: '3.2.1',
+  await updateWorkspaceManifest(dir, {
+    updatedCatalogs: {
+      foo: {
+        abc: '0.1.2',
+      },
+      bar: {
+        def: '3.2.1',
+      },
     },
   })
   expect(readYamlFile(filePath)).toStrictEqual({

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

@@ -10,7 +10,7 @@ test('updateWorkspaceManifest adds a new setting', async () => {
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
   writeYamlFile(filePath, { packages: ['*'] })
   await updateWorkspaceManifest(dir, {
-    onlyBuiltDependencies: [],
+    updatedFields: { onlyBuiltDependencies: [] },
   })
   expect(readYamlFile(filePath)).toStrictEqual({
     packages: ['*'],
@@ -23,7 +23,7 @@ test('updateWorkspaceManifest removes an existing setting', async () => {
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
   writeYamlFile(filePath, { packages: ['*'], overrides: { foo: '2' } })
   await updateWorkspaceManifest(dir, {
-    overrides: undefined,
+    updatedFields: { overrides: undefined },
   })
   expect(readYamlFile(filePath)).toStrictEqual({
     packages: ['*'],
@@ -35,7 +35,7 @@ test('updateWorkspaceManifest updates an existing setting', async () => {
   const filePath = path.join(dir, WORKSPACE_MANIFEST_FILENAME)
   writeYamlFile(filePath, { packages: ['*'], overrides: { foo: '2' } })
   await updateWorkspaceManifest(dir, {
-    overrides: { bar: '3' },
+    updatedFields: { overrides: { bar: '3' } },
   })
   expect(readYamlFile(filePath)).toStrictEqual({
     packages: ['*'],