Code review feedback - test and code cleanup

Author: Ben Keen

libraries/rush-lib/config/heft.json

@@ -30,7 +30,7 @@
                 {
                   "sourcePath": "src/cli/test",
                   "destinationFolders": ["lib-intermediate-commonjs/cli/test"],
-                  "fileExtensions": [".js"]
+                  "fileExtensions": [".js", ".yaml"]
                 },
                 {
                   "sourcePath": "src/logic/pnpm/test",

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

@@ -6,58 +6,29 @@ import { FileSystem, JsonFile } from '@rushstack/node-core-library';
 import { TestUtilities } from '@rushstack/heft-config-file';
 import { RushConfiguration } from '../../api/RushConfiguration';
 
+const MONOREPO_ROOT: string = path.dirname(
+  RushConfiguration.tryFindRushJsonLocation({ startingFolder: __dirname })!
+);
+const CATALOG_SYNC_REPO_PATH: string = `${__dirname}/catalogSyncTestRepo`;
+
 describe('RushPnpmCommandLineParser', () => {
   describe('catalog syncing', () => {
-    let testRepoPath: string;
-    let pnpmConfigPath: string;
-    let pnpmWorkspacePath: string;
-
-    beforeEach(() => {
-      testRepoPath = path.join(__dirname, 'temp', 'catalog-sync-test-repo');
-      FileSystem.ensureFolder(testRepoPath);
-
-      const rushJsonPath: string = `${testRepoPath/rush.json`;
-      const rushJson = {
-        $schema: 'https://developer.microsoft.com/json-schemas/rush/v5/rush.schema.json',
-        rushVersion: '5.166.0',
-        pnpmVersion: '10.28.1',
-        nodeSupportedVersionRange: '>=18.0.0',
-        projects: []
-      };
-      JsonFile.save(rushJson, rushJsonPath, { ensureFolderExists: true });
-
-      const configDir: string = path.join(testRepoPath, 'common', 'config', 'rush');
-      FileSystem.ensureFolder(configDir);
-
-      pnpmConfigPath = path.join(configDir, 'pnpm-config.json');
-      const pnpmConfig = {
-        globalCatalogs: {
-          default: {
-            react: '^18.0.0',
-            'react-dom': '^18.0.0'
-          }
-        }
-      };
-      JsonFile.save(pnpmConfig, pnpmConfigPath);
+    const testRepoPath: string = `${MONOREPO_ROOT}/temp/catalog-sync-test-repo`;
+    const pnpmConfigPath: string = `${testRepoPath}/common/config/rush/pnpm-config.json`;
+    const pnpmWorkspacePath: string = `${testRepoPath}/common/temp/pnpm-workspace.yaml`;
 
-      const tempDir: string = path.join(testRepoPath, 'common', 'temp');
-      FileSystem.ensureFolder(tempDir);
+    beforeEach(async () => {
+      await FileSystem.copyFilesAsync({ sourcePath: CATALOG_SYNC_REPO_PATH, destinationPath: testRepoPath });
 
-      pnpmWorkspacePath = path.join(tempDir, 'pnpm-workspace.yaml');
-      const workspaceYaml = `packages:
-  - '../../apps/*'
-catalogs:
-  default:
-    react: ^18.0.0
-    react-dom: ^18.0.0
-`;
-      FileSystem.writeFile(pnpmWorkspacePath, workspaceYaml);
+      // common/temp is gitignored so it is not part of the static repo; copy the initial workspace file here
+      await FileSystem.copyFilesAsync({
+        sourcePath: `${CATALOG_SYNC_REPO_PATH}/pnpm-workspace.yaml`,
+        destinationPath: pnpmWorkspacePath
+      });
     });
 
-    afterEach(() => {
-      if (FileSystem.exists(testRepoPath)) {
-        FileSystem.deleteFolder(testRepoPath);
-      }
+    afterEach(async () => {
+      await FileSystem.deleteFolderAsync(testRepoPath);
     });
 
     it('syncs updated catalogs from pnpm-workspace.yaml to pnpm-config.json', async () => {
@@ -71,10 +42,10 @@ catalogs:
   frontend:
     vue: ^3.4.0
 `;
-      FileSystem.writeFile(pnpmWorkspacePath, updatedWorkspaceYaml);
+      await FileSystem.writeFileAsync(pnpmWorkspacePath, updatedWorkspaceYaml);
 
       const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
-        path.join(testRepoPath, 'rush.json')
+        `${testRepoPath}/rush.json`
       );
 
       const subspace = rushConfiguration.getSubspace('default');
@@ -93,7 +64,7 @@ catalogs:
       pnpmOptions?.updateGlobalCatalogs(newCatalogs);
 
       const updatedRushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
-        path.join(testRepoPath, 'rush.json')
+        `${testRepoPath}/rush.json`
       );
       const updatedSubspace = updatedRushConfiguration.getSubspace('default');
       const updatedPnpmOptions = updatedSubspace.getPnpmOptions();
@@ -115,7 +86,7 @@ catalogs:
       const newCatalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(pnpmWorkspacePath);
 
       const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
-        path.join(testRepoPath, 'rush.json')
+        `${testRepoPath}/rush.json`
       );
       const subspace = rushConfiguration.getSubspace('default');
       const pnpmOptions = subspace.getPnpmOptions();
@@ -135,23 +106,23 @@ catalogs:
       const workspaceWithoutCatalogs = `packages:
   - '../../apps/*'
 `;
-      FileSystem.writeFile(pnpmWorkspacePath, workspaceWithoutCatalogs);
+      await FileSystem.writeFileAsync(pnpmWorkspacePath, workspaceWithoutCatalogs);
 
       const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile');
       const newCatalogs = await PnpmWorkspaceFile.loadCatalogsFromFileAsync(pnpmWorkspacePath);
 
       expect(newCatalogs).toBeUndefined();
 
       const rushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
-        path.join(testRepoPath, 'rush.json')
+        `${testRepoPath}/rush.json`
       );
       const subspace = rushConfiguration.getSubspace('default');
       const pnpmOptions = subspace.getPnpmOptions();
 
       pnpmOptions?.updateGlobalCatalogs(newCatalogs);
 
       const updatedRushConfiguration: RushConfiguration = RushConfiguration.loadFromConfigurationFile(
-        path.join(testRepoPath, 'rush.json')
+        `${testRepoPath}/rush.json`
       );
       const updatedSubspace = updatedRushConfiguration.getSubspace('default');
       const updatedPnpmOptions = updatedSubspace.getPnpmOptions();

libraries/rush-lib/src/cli/test/catalogSyncTestRepo/common/config/rush/pnpm-config.json

@@ -0,0 +1,8 @@
+{
+  "globalCatalogs": {
+    "default": {
+      "react": "^18.0.0",
+      "react-dom": "^18.0.0"
+    }
+  }
+}

libraries/rush-lib/src/cli/test/catalogSyncTestRepo/pnpm-workspace.yaml

@@ -0,0 +1,6 @@
+packages:
+  - '../../apps/*'
+catalogs:
+  default:
+    react: ^18.0.0
+    react-dom: ^18.0.0

libraries/rush-lib/src/cli/test/catalogSyncTestRepo/rush.json

@@ -0,0 +1,7 @@
+{
+  "$schema": "https://developer.microsoft.com/json-schemas/rush/v5/rush.schema.json",
+  "rushVersion": "5.166.0",
+  "pnpmVersion": "10.28.1",
+  "nodeSupportedVersionRange": ">=18.0.0",
+  "projects": []
+}

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

@@ -535,9 +535,7 @@ export class PnpmOptionsConfiguration extends PackageManagerOptionsConfiguration
   public updateGlobalPatchedDependencies(patchedDependencies: Record<string, string> | undefined): void {
     this._globalPatchedDependencies = patchedDependencies;
     this._json.globalPatchedDependencies = patchedDependencies;
-    if (this.jsonFilename) {
-      JsonFile.save(this._json, this.jsonFilename, { updateExistingFile: true });
-    }
+    JsonFile.save(this._json, this.jsonFilename as string, { updateExistingFile: true });
   }
 
   /**
@@ -547,18 +545,20 @@ export class PnpmOptionsConfiguration extends PackageManagerOptionsConfiguration
     onlyBuiltDependencies: string[] | undefined
   ): Promise<void> {
     this._json.globalOnlyBuiltDependencies = onlyBuiltDependencies;
-    if (this.jsonFilename) {
-      await JsonFile.saveAsync(this._json, this.jsonFilename, { updateExistingFile: true, ignoreUndefinedValues: true });
-    }
+    await JsonFile.saveAsync(this._json, this.jsonFilename as string, {
+      updateExistingFile: true,
+      ignoreUndefinedValues: true
+    });
   }
 
   /**
    * Updates globalCatalogs field of the PNPM options in the common/config/rush/pnpm-config.json file.
    */
   public updateGlobalCatalogs(catalogs: Record<string, Record<string, string>> | undefined): void {
     this._json.globalCatalogs = catalogs;
-    if (this.jsonFilename) {
-      JsonFile.save(this._json, this.jsonFilename, { updateExistingFile: true, ignoreUndefinedValues: true });
-    }
+    JsonFile.save(this._json, this.jsonFilename as string, {
+      updateExistingFile: true,
+      ignoreUndefinedValues: true
+    });
   }
 }

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

@@ -77,7 +77,7 @@ export class PnpmWorkspaceFile extends BaseWorkspaceFile {
         throw error;
       }
     }
-    
+
     const parsed: IPnpmWorkspaceYaml | undefined = yamlModule.load(content) as IPnpmWorkspaceYaml | undefined;
 
     return parsed?.catalogs;