Address code review feedback

- afterInstallAsync: switch/case with throw on unsupported pnpm versions
- computeResolverCacheFromLockfileAsync: hoist backslash regex, use
  name ||= parsed.name, destructure lockfile.packages
- helpers: flatten resolveDependencyKey link: branches into ternary,
  simplify package key resolution to single getDescriptionFileRootFromKey call
- pnpm/index.ts: move types and factory to pnpmVersionHelpers.ts,
  index.ts now only re-exports
- pnpm/v8,v9,v10: import from ./pnpmVersionHelpers instead of .
- pnpm/store/v10: add Dirent to import, hoist scope separator regex

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: bmiddha

rush-plugins/rush-resolver-cache-plugin/src/afterInstallAsync.ts

@@ -16,7 +16,11 @@ import {
   computeResolverCacheFromLockfileAsync,
   type IPlatformInfo
 } from './computeResolverCacheFromLockfileAsync';
-import { type PnpmMajorVersion, type IPnpmVersionHelpers, getPnpmVersionHelpersAsync } from './pnpm';
+import {
+  type PnpmMajorVersion,
+  type IPnpmVersionHelpers,
+  getPnpmVersionHelpersAsync
+} from './pnpm/pnpmVersionHelpers';
 import type { IResolverContext } from './types';
 
 /**
@@ -84,10 +88,17 @@ export async function afterInstallAsync(
 
   const pnpmMajorVersion: PnpmMajorVersion = (() => {
     const major: number = parseInt(rushConfiguration.packageManagerToolVersion, 10);
-    if (major >= 10) return 10;
-    if (major >= 9) return 9;
-    return 8;
-  })() as PnpmMajorVersion;
+    switch (major) {
+      case 10:
+        return 10;
+      case 9:
+        return 9;
+      case 8:
+        return 8;
+      default:
+        throw new Error(`Unsupported pnpm major version: ${major}`);
+    }
+  })();
 
   const pnpmHelpers: IPnpmVersionHelpers = await getPnpmVersionHelpersAsync(pnpmMajorVersion);
 

rush-plugins/rush-resolver-cache-plugin/src/computeResolverCacheFromLockfileAsync.ts

@@ -15,7 +15,11 @@ import {
   createContextSerializer,
   extractNameAndVersionFromKey
 } from './helpers';
-import { type PnpmMajorVersion, type IPnpmVersionHelpers, getPnpmVersionHelpersAsync } from './pnpm';
+import {
+  type PnpmMajorVersion,
+  type IPnpmVersionHelpers,
+  getPnpmVersionHelpersAsync
+} from './pnpm/pnpmVersionHelpers';
 import type { IResolverContext } from './types';
 
 /**
@@ -112,7 +116,7 @@ function extractBundledDependencies(
 }
 
 // Re-export for downstream consumers
-export type { PnpmMajorVersion, IPnpmVersionHelpers } from './pnpm';
+export type { PnpmMajorVersion, IPnpmVersionHelpers } from './pnpm/pnpmVersionHelpers';
 
 /**
  * Options for computing the resolver cache from a lockfile.
@@ -157,13 +161,15 @@ export interface IComputeResolverCacheFromLockfileOptions {
   ) => Promise<void>;
 }
 
+const BACKSLASH_REGEX: RegExp = /\\/g;
+
 /**
  * Copied from `@rushstack/node-core-library/src/Path.ts` to avoid expensive dependency
  * @param path - Path using backslashes as path separators
  * @returns The same string using forward slashes as path separators
  */
 function convertToSlashes(path: string): string {
-  return path.replace(/\\/g, '/');
+  return path.replace(BACKSLASH_REGEX, '/');
 }
 
 /**
@@ -183,12 +189,12 @@ export async function computeResolverCacheFromLockfileAsync(
   const contexts: Map<string, IResolverContext> = new Map();
   const missingOptionalDependencies: Set<string> = new Set();
 
-  const pnpmVersion: PnpmMajorVersion = params.pnpmVersion;
+  const helpers: IPnpmVersionHelpers = await getPnpmVersionHelpersAsync(params.pnpmVersion);
 
-  const helpers: IPnpmVersionHelpers = await getPnpmVersionHelpersAsync(pnpmVersion);
+  const { packages } = lockfile;
 
   // Enumerate external dependencies first, to simplify looping over them for store data
-  for (const [key, pack] of lockfile.packages) {
+  for (const [key, pack] of packages) {
     let name: string | undefined = pack.name;
     const descriptionFileRoot: string = getDescriptionFileRootFromKey(
       workspaceRoot,
@@ -208,9 +214,7 @@ export async function computeResolverCacheFromLockfileAsync(
     // Extract name and version from the key if not already provided
     const parsed: { name: string; version: string } | undefined = extractNameAndVersionFromKey(key);
     if (parsed) {
-      if (!name) {
-        name = parsed.name;
-      }
+      name ||= parsed.name;
     }
 
     if (!name) {
@@ -231,10 +235,10 @@ export async function computeResolverCacheFromLockfileAsync(
     contexts.set(descriptionFileRoot, context);
 
     if (pack.dependencies) {
-      resolveDependencies(workspaceRoot, pack.dependencies, context, helpers, lockfile.packages);
+      resolveDependencies(workspaceRoot, pack.dependencies, context, helpers, packages);
     }
     if (pack.optionalDependencies) {
-      resolveDependencies(workspaceRoot, pack.optionalDependencies, context, helpers, lockfile.packages);
+      resolveDependencies(workspaceRoot, pack.optionalDependencies, context, helpers, packages);
     }
   }
 
@@ -275,13 +279,13 @@ export async function computeResolverCacheFromLockfileAsync(
     contexts.set(descriptionFileRoot, context);
 
     if (importer.dependencies) {
-      resolveDependencies(workspaceRoot, importer.dependencies, context, helpers, lockfile.packages);
+      resolveDependencies(workspaceRoot, importer.dependencies, context, helpers, packages);
     }
     if (importer.devDependencies) {
-      resolveDependencies(workspaceRoot, importer.devDependencies, context, helpers, lockfile.packages);
+      resolveDependencies(workspaceRoot, importer.devDependencies, context, helpers, packages);
     }
     if (importer.optionalDependencies) {
-      resolveDependencies(workspaceRoot, importer.optionalDependencies, context, helpers, lockfile.packages);
+      resolveDependencies(workspaceRoot, importer.optionalDependencies, context, helpers, packages);
     }
   }
 

rush-plugins/rush-resolver-cache-plugin/src/helpers.ts

@@ -6,7 +6,7 @@ import * as path from 'node:path';
 import type { ISerializedResolveContext } from '@rushstack/webpack-workspace-resolve-plugin';
 
 import type { IDependencyEntry, IResolverContext } from './types';
-import type { IPnpmVersionHelpers } from './pnpm';
+import type { IPnpmVersionHelpers } from './pnpm/pnpmVersionHelpers';
 
 /**
  * Computes the root folder for a dependency from a reference to it in another package
@@ -26,19 +26,17 @@ export function resolveDependencyKey(
   packageKeys?: { has(key: string): boolean }
 ): string {
   if (specifier.startsWith('link:')) {
-    if (context.isProject) {
-      return path.posix.join(context.descriptionFileRoot, specifier.slice(5));
-    } else {
-      return path.posix.join(lockfileFolder, specifier.slice(5));
-    }
+    return path.posix.join(
+      context.isProject ? context.descriptionFileRoot : lockfileFolder,
+      specifier.slice(5)
+    );
   } else if (specifier.startsWith('file:')) {
     return getDescriptionFileRootFromKey(lockfileFolder, specifier, helpers.depPathToFilename, key);
-  } else if (packageKeys?.has(specifier)) {
-    // The specifier is a full package key (v6: '/pkg@ver', v9: 'pkg@ver')
-    return getDescriptionFileRootFromKey(lockfileFolder, specifier, helpers.depPathToFilename);
   } else {
-    const fullKey: string = helpers.buildDependencyKey(key, specifier);
-    return getDescriptionFileRootFromKey(lockfileFolder, fullKey, helpers.depPathToFilename);
+    const resolvedKey: string = packageKeys?.has(specifier)
+      ? specifier
+      : helpers.buildDependencyKey(key, specifier);
+    return getDescriptionFileRootFromKey(lockfileFolder, resolvedKey, helpers.depPathToFilename);
   }
 }
 

…-resolver-cache-plugin/src/pnpm/index.ts …he-plugin/src/pnpm/pnpmVersionHelpers.tsrush-plugins/rush-resolver-cache-plugin/src/pnpm/index.ts renamed to rush-plugins/rush-resolver-cache-plugin/src/pnpm/pnpmVersionHelpers.ts rush-plugins/rush-resolver-cache-plugin/src/pnpm/index.ts renamed to rush-plugins/rush-resolver-cache-plugin/src/pnpm/pnpmVersionHelpers.ts

@@ -5,17 +5,19 @@
 // {storeDir}/v10/index/{hash[0:2]}/{hash[2:64]}-{name}@{version}.json
 // Falls back to directory scan when the primary path doesn't exist.
 
-import { existsSync, readdirSync } from 'node:fs';
+import { type Dirent, existsSync, readdirSync } from 'node:fs';
 
 import type { IResolverContext } from '../../types';
 
+const SCOPE_SEPARATOR_REGEX: RegExp = /\//g;
+
 export function getStoreIndexPath(pnpmStorePath: string, context: IResolverContext, hash: string): string {
   // pnpm 10 truncates integrity hashes to 32 bytes (64 hex chars) for index paths.
   const truncHash: string = hash.length > 64 ? hash.slice(0, 64) : hash;
   const hashDir: string = truncHash.slice(0, 2);
   const hashRest: string = truncHash.slice(2);
   // pnpm 10 index path format: <hash (0-2)>/<hash (2-64)>-<name>@<version>.json
-  const pkgName: string = (context.name || '').replace(/\//g, '+');
+  const pkgName: string = (context.name || '').replace(SCOPE_SEPARATOR_REGEX, '+');
   const nameVer: string = context.version ? `${pkgName}@${context.version}` : pkgName;
   let indexPath: string = `${pnpmStorePath}/v10/index/${hashDir}/${hashRest}-${nameVer}.json`;
   // For truncated/hashed folder names, nameVer from the key may be wrong.
@@ -24,10 +26,8 @@ export function getStoreIndexPath(pnpmStorePath: string, context: IResolverConte
     const dir: string = `${pnpmStorePath}/v10/index/${hashDir}/`;
     const filePrefix: string = `${hashRest}-`;
     try {
-      const entries: import('node:fs').Dirent[] = readdirSync(dir, { withFileTypes: true });
-      const match: import('node:fs').Dirent | undefined = entries.find(
-        (e) => e.isFile() && e.name.startsWith(filePrefix)
-      );
+      const entries: Dirent[] = readdirSync(dir, { withFileTypes: true });
+      const match: Dirent | undefined = entries.find((e) => e.isFile() && e.name.startsWith(filePrefix));
       if (match) {
         indexPath = dir + match.name;
       }

rush-plugins/rush-resolver-cache-plugin/src/pnpm/store/v10.ts

@@ -3,7 +3,7 @@
 
 // pnpm 10: lockfile v9 (keys have no leading '/'), store v10, SHA-256 hex hash
 
-import type { IPnpmVersionHelpers } from '.';
+import type { IPnpmVersionHelpers } from './pnpmVersionHelpers';
 import { depPathToFilename } from './depPath/v10';
 import { buildDependencyKey } from './keys/v9';
 import { getStoreIndexPath } from './store/v10';

rush-plugins/rush-resolver-cache-plugin/src/pnpm/v10.ts

@@ -3,7 +3,7 @@
 
 // pnpm 8: lockfile v6 (keys start with '/'), store v3, MD5 base32 hash
 
-import type { IPnpmVersionHelpers } from '.';
+import type { IPnpmVersionHelpers } from './pnpmVersionHelpers';
 import { depPathToFilename } from './depPath/v8';
 import { buildDependencyKey } from './keys/v6';
 import { getStoreIndexPath } from './store/v3';

rush-plugins/rush-resolver-cache-plugin/src/pnpm/v8.ts

@@ -3,7 +3,7 @@
 
 // pnpm 9: lockfile v9 (keys have no leading '/'), store v3, MD5 base32 hash
 
-import type { IPnpmVersionHelpers } from '.';
+import type { IPnpmVersionHelpers } from './pnpmVersionHelpers';
 // pnpm 9 uses the same dep-path hashing algorithm as pnpm 8 (MD5 base32)
 // but a different depPathToFilenameUnescaped (indexOf('@') vs lastIndexOf('/'))
 import { depPathToFilename } from './depPath/v9';

rush-plugins/rush-resolver-cache-plugin/src/pnpm/v9.ts

@@ -13,7 +13,7 @@ import {
 import { helpers as v8Helpers } from '../pnpm/v8';
 import { helpers as v9Helpers } from '../pnpm/v9';
 import { helpers as v10Helpers } from '../pnpm/v10';
-import { getPnpmVersionHelpersAsync, type IPnpmVersionHelpers } from '../pnpm';
+import { getPnpmVersionHelpersAsync, type IPnpmVersionHelpers } from '../pnpm/pnpmVersionHelpers';
 import type { IResolverContext } from '../types';
 
 describe(createBase32Hash.name, () => {