Apply code review feedback

Author: Ben Keen

common/reviews/api/rush-lib.api.md

@@ -1177,7 +1177,7 @@ export class PnpmOptionsConfiguration extends PackageManagerOptionsConfiguration
     readonly strictPeerDependencies: boolean;
     readonly unsupportedPackageJsonSettings: unknown | undefined;
     updateGlobalCatalogs(catalogs: Record<string, Record<string, string>> | undefined): void;
-    updateGlobalOnlyBuiltDependencies(onlyBuiltDependencies: string[] | undefined): void;
+    updateGlobalOnlyBuiltDependenciesAsync(onlyBuiltDependencies: string[] | undefined): Promise<void>;
     updateGlobalPatchedDependencies(patchedDependencies: Record<string, string> | undefined): void;
     readonly useWorkspaces: boolean;
 }

libraries/rush-lib/src/cli/RushPnpmCommandLineParser.ts

@@ -584,7 +584,7 @@ export class RushPnpmCommandLineParser {
 
         if (!Objects.areDeepEqual(currentGlobalOnlyBuiltDependencies, newGlobalOnlyBuiltDependencies)) {
           // Update onlyBuiltDependencies to pnpm configuration file
-          pnpmOptions?.updateGlobalOnlyBuiltDependencies(newGlobalOnlyBuiltDependencies);
+          await pnpmOptions?.updateGlobalOnlyBuiltDependenciesAsync(newGlobalOnlyBuiltDependencies);
 
           // Rerun installation to update
           await this._doRushUpdateAsync();
@@ -605,7 +605,7 @@ export class RushPnpmCommandLineParser {
 
         const workspaceYamlFilename: string = path.join(subspaceTempFolder, 'pnpm-workspace.yaml');
         const newCatalogs: Record<string, Record<string, string>> | undefined =
-          PnpmWorkspaceFile.loadCatalogsFromFile(workspaceYamlFilename);
+          await PnpmWorkspaceFile.loadCatalogsFromFileAsync(workspaceYamlFilename);
         const currentCatalogs: Record<string, Record<string, string>> | undefined =
           pnpmOptions.globalCatalogs;
 

libraries/rush-lib/src/cli/test/RushPnpmCommandLineParser.test.ts

@@ -60,7 +60,7 @@ catalogs:
       }
     });
 
-    it('syncs updated catalogs from pnpm-workspace.yaml to pnpm-config.json', () => {
+    it('syncs updated catalogs from pnpm-workspace.yaml to pnpm-config.json', async () => {
       const updatedWorkspaceYaml = `packages:
   - '../../apps/*'
 catalogs:
@@ -88,7 +88,7 @@ catalogs:
       });
 
       const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile');
-      const newCatalogs = PnpmWorkspaceFile.loadCatalogsFromFile(pnpmWorkspacePath);
+      const newCatalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(pnpmWorkspacePath);
 
       pnpmOptions?.updateGlobalCatalogs(newCatalogs);
 
@@ -110,9 +110,9 @@ catalogs:
       });
     });
 
-    it('does not update pnpm-config.json when catalogs are unchanged', () => {
+    it('does not update pnpm-config.json when catalogs are unchanged', async () => {
       const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile');
-      const newCatalogs = PnpmWorkspaceFile.loadCatalogsFromFile(pnpmWorkspacePath);
+      const newCatalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(pnpmWorkspacePath);
 
       const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
         path.join(testRepoPath, 'rush.json')
@@ -131,14 +131,14 @@ catalogs:
       });
     });
 
-    it('removes catalogs when pnpm-workspace.yaml has no catalogs', () => {
+    it('removes catalogs when pnpm-workspace.yaml has no catalogs', async () => {
       const workspaceWithoutCatalogs = `packages:
   - '../../apps/*'
 `;
       FileSystem.writeFile(pnpmWorkspacePath, workspaceWithoutCatalogs);
 
       const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile');
-      const newCatalogs = PnpmWorkspaceFile.loadCatalogsFromFile(pnpmWorkspacePath);
+      const newCatalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(pnpmWorkspacePath);
 
       expect(newCatalogs).toBeUndefined();
 

libraries/rush-lib/src/logic/pnpm/PnpmOptionsConfiguration.ts

@@ -552,14 +552,16 @@ export class PnpmOptionsConfiguration extends PackageManagerOptionsConfiguration
   /**
    * Updates globalOnlyBuiltDependencies field of the PNPM options in the common/config/rush/pnpm-config.json file.
    */
-  public updateGlobalOnlyBuiltDependencies(onlyBuiltDependencies: string[] | undefined): void {
+  public async updateGlobalOnlyBuiltDependenciesAsync(
+    onlyBuiltDependencies: string[] | undefined
+  ): Promise<void> {
     if (onlyBuiltDependencies === undefined) {
       delete this._json.globalOnlyBuiltDependencies;
     } else {
       this._json.globalOnlyBuiltDependencies = onlyBuiltDependencies;
     }
     if (this.jsonFilename) {
-      JsonFile.save(this._json, this.jsonFilename, { updateExistingFile: true });
+      await JsonFile.saveAsync(this._json, this.jsonFilename, { updateExistingFile: true });
     }
   }
 

libraries/rush-lib/src/logic/pnpm/PnpmWorkspaceFile.ts

@@ -64,13 +64,13 @@ export class PnpmWorkspaceFile extends BaseWorkspaceFile {
    * @param workspaceYamlFilename - The path to the pnpm-workspace.yaml file
    * @returns The catalogs object, or undefined if the file doesn't exist or has no catalogs
    */
-  public static loadCatalogsFromFile(
+  public static async loadCatalogsFromFileAsync(
     workspaceYamlFilename: string
-  ): Record<string, Record<string, string>> | undefined {
-    if (!FileSystem.exists(workspaceYamlFilename)) {
+  ): Promise<Record<string, Record<string, string>> | undefined> {
+    if (!(await FileSystem.existsAsync(workspaceYamlFilename))) {
       return undefined;
     }
-    const content: string = FileSystem.readFile(workspaceYamlFilename);
+    const content: string = await FileSystem.readFileAsync(workspaceYamlFilename);
     const parsed: IPnpmWorkspaceYaml | undefined = yamlModule.load(content) as IPnpmWorkspaceYaml | undefined;
 
     if (!parsed || !parsed.catalogs) {

libraries/rush-lib/src/logic/pnpm/test/PnpmOptionsConfiguration.test.ts

@@ -294,7 +294,7 @@ describe(PnpmOptionsConfiguration.name, () => {
       }
     });
 
-    it('handles undefined in updateGlobalOnlyBuiltDependencies', () => {
+    it('handles undefined in updateGlobalOnlyBuiltDependenciesAsync', async () => {
       const testConfigPath: string = path.join(__dirname, 'temp', 'pnpm-config-undefined-test.json');
       const tempDir: string = path.dirname(testConfigPath);
       FileSystem.ensureFolder(tempDir);
@@ -316,9 +316,9 @@ describe(PnpmOptionsConfiguration.name, () => {
           'esbuild'
         ]);
 
-        expect(() => {
-          pnpmConfiguration.updateGlobalOnlyBuiltDependencies(undefined);
-        }).not.toThrow();
+        await expect(
+          pnpmConfiguration.updateGlobalOnlyBuiltDependenciesAsync(undefined)
+        ).resolves.not.toThrow();
 
         const reloadedConfig: PnpmOptionsConfiguration = PnpmOptionsConfiguration.loadFromJsonFileOrThrow(
           testConfigPath,

libraries/rush-lib/src/logic/pnpm/test/PnpmWorkspaceFile.test.ts

@@ -12,7 +12,9 @@ describe(PnpmWorkspaceFile.name, () => {
 
   let mockWriteFile: jest.SpyInstance;
   let mockReadFile: jest.SpyInstance;
+  let mockReadFileAsync: jest.SpyInstance;
   let mockExists: jest.SpyInstance;
+  let mockExistsAsync: jest.SpyInstance;
   let writtenContent: string | undefined;
 
   beforeEach(() => {
@@ -34,16 +36,31 @@ describe(PnpmWorkspaceFile.name, () => {
       return writtenContent;
     });
 
+    // Mock async version for loadCatalogsFromFileAsync
+    mockReadFileAsync = jest.spyOn(FileSystem, 'readFileAsync').mockImplementation(async () => {
+      if (writtenContent === undefined) {
+        throw new Error('File not found');
+      }
+      return writtenContent;
+    });
+
     // Mock FileSystem.exists to return true if content was written
     mockExists = jest.spyOn(FileSystem, 'exists').mockImplementation(() => {
       return writtenContent !== undefined;
     });
+
+    // Mock async version for loadCatalogsFromFileAsync
+    mockExistsAsync = jest.spyOn(FileSystem, 'existsAsync').mockImplementation(async () => {
+      return writtenContent !== undefined;
+    });
   });
 
   afterEach(() => {
     mockWriteFile.mockRestore();
     mockReadFile.mockRestore();
+    mockReadFileAsync.mockRestore();
     mockExists.mockRestore();
+    mockExistsAsync.mockRestore();
   });
 
   describe('basic functionality', () => {
@@ -181,23 +198,23 @@ describe(PnpmWorkspaceFile.name, () => {
     });
   });
 
-  describe('loadCatalogsFromFile', () => {
-    it('returns undefined for non-existent file', () => {
+  describe('loadCatalogsFromFileAsync', () => {
+    it('returns undefined for non-existent file', async () => {
       const nonExistentFile: string = path.join(tempDir, 'non-existent.yaml');
-      const catalogs = PnpmWorkspaceFile.loadCatalogsFromFile(nonExistentFile);
+      const catalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(nonExistentFile);
       expect(catalogs).toBeUndefined();
     });
 
-    it('returns undefined when file has no catalogs', () => {
+    it('returns undefined when file has no catalogs', async () => {
       const workspaceFile: PnpmWorkspaceFile = new PnpmWorkspaceFile(workspaceFilePath);
       workspaceFile.addPackage(path.join(projectsDir, 'app1'));
       workspaceFile.save(workspaceFilePath, { onlyIfChanged: true });
 
-      const catalogs = PnpmWorkspaceFile.loadCatalogsFromFile(workspaceFilePath);
+      const catalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(workspaceFilePath);
       expect(catalogs).toBeUndefined();
     });
 
-    it('loads catalogs from existing file', () => {
+    it('loads catalogs from existing file', async () => {
       const workspaceFile: PnpmWorkspaceFile = new PnpmWorkspaceFile(workspaceFilePath);
       workspaceFile.addPackage(path.join(projectsDir, 'app1'));
       workspaceFile.setCatalogs({
@@ -211,7 +228,7 @@ describe(PnpmWorkspaceFile.name, () => {
       });
       workspaceFile.save(workspaceFilePath, { onlyIfChanged: true });
 
-      const catalogs = PnpmWorkspaceFile.loadCatalogsFromFile(workspaceFilePath);
+      const catalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(workspaceFilePath);
       expect(catalogs).toEqual({
         default: {
           react: '^18.0.0',