From e4f0fafadf83c88da017019990868dd22e0bbdac Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Tue, 3 Feb 2026 17:00:44 -0500 Subject: [PATCH] perf(@angular/cli): optimize package manager discovery with stat-based probing This refactors the package manager discovery logic to use 'stat' instead of 'readdir', significantly improving performance in directories with many files. It also simplifies the internal host abstraction by removing the now unused 'readdir' method. --- .../cli/src/package-managers/discovery.ts | 51 +++++++++---------- .../src/package-managers/discovery_spec.ts | 8 --- .../angular/cli/src/package-managers/host.ts | 8 --- .../src/package-managers/testing/mock-host.ts | 27 +++++----- 4 files changed, 38 insertions(+), 56 deletions(-) diff --git a/packages/angular/cli/src/package-managers/discovery.ts b/packages/angular/cli/src/package-managers/discovery.ts index c96a637e7eb1..e99a4890c6ba 100644 --- a/packages/angular/cli/src/package-managers/discovery.ts +++ b/packages/angular/cli/src/package-managers/discovery.ts @@ -21,18 +21,6 @@ import { SUPPORTED_PACKAGE_MANAGERS, } from './package-manager-descriptor'; -/** - * A map from lockfile names to their corresponding package manager. - * This is a performance optimization to avoid iterating over all possible - * lockfiles in every directory. - */ -const LOCKFILE_TO_PACKAGE_MANAGER = new Map(); -for (const [name, descriptor] of Object.entries(SUPPORTED_PACKAGE_MANAGERS)) { - for (const lockfile of descriptor.lockfiles) { - LOCKFILE_TO_PACKAGE_MANAGER.set(lockfile, name as PackageManagerName); - } -} - /** * Searches a directory for lockfiles and returns a set of package managers that correspond to them. * @param host A `Host` instance for interacting with the file system. @@ -47,25 +35,32 @@ async function findLockfiles( ): Promise> { logger?.debug(`Searching for lockfiles in '${directory}'...`); - try { - const files = await host.readdir(directory); - const foundPackageManagers = new Set(); - - for (const file of files) { - const packageManager = LOCKFILE_TO_PACKAGE_MANAGER.get(file); - if (packageManager) { - logger?.debug(` Found '${file}'.`); - foundPackageManagers.add(packageManager); - } + const foundPackageManagers = new Set(); + const checks: Promise[] = []; + + for (const [name, descriptor] of Object.entries(SUPPORTED_PACKAGE_MANAGERS)) { + const manager = name as PackageManagerName; + for (const lockfile of descriptor.lockfiles) { + checks.push( + (async () => { + try { + const path = join(directory, lockfile); + const stats = await host.stat(path); + if (stats.isFile()) { + logger?.debug(` Found '${lockfile}'.`); + foundPackageManagers.add(manager); + } + } catch { + // File does not exist or cannot be accessed. + } + })(), + ); } + } - return foundPackageManagers; - } catch (e) { - logger?.debug(` Failed to read directory: ${e}`); + await Promise.all(checks); - // Ignore directories that don't exist or can't be read. - return new Set(); - } + return foundPackageManagers; } /** diff --git a/packages/angular/cli/src/package-managers/discovery_spec.ts b/packages/angular/cli/src/package-managers/discovery_spec.ts index 5570be1d614b..077d5d93c604 100644 --- a/packages/angular/cli/src/package-managers/discovery_spec.ts +++ b/packages/angular/cli/src/package-managers/discovery_spec.ts @@ -64,14 +64,6 @@ describe('discover', () => { expect(result).toBeNull(); }); - it('should handle file system errors during readdir gracefully', async () => { - const host = new MockHost({}); - host.readdir = () => Promise.reject(new Error('Permission denied')); - - const result = await discover(host, '/project'); - expect(result).toBeNull(); - }); - it('should handle file system errors during stat gracefully', async () => { const host = new MockHost({ '/project': ['.git'] }); host.stat = () => Promise.reject(new Error('Permission denied')); diff --git a/packages/angular/cli/src/package-managers/host.ts b/packages/angular/cli/src/package-managers/host.ts index f7509ff01a99..433b54414f69 100644 --- a/packages/angular/cli/src/package-managers/host.ts +++ b/packages/angular/cli/src/package-managers/host.ts @@ -39,13 +39,6 @@ export interface Host { */ stat(path: string): Promise; - /** - * Reads the contents of a directory. - * @param path The path to the directory. - * @returns A promise that resolves to an array of file and directory names. - */ - readdir(path: string): Promise; - /** * Reads the content of a file. * @param path The path to the file. @@ -108,7 +101,6 @@ export interface Host { */ export const NodeJS_HOST: Host = { stat, - readdir, mkdir, readFile: (path: string) => readFile(path, { encoding: 'utf8' }), copyFile: (src, dest) => copyFile(src, dest, constants.COPYFILE_FICLONE), diff --git a/packages/angular/cli/src/package-managers/testing/mock-host.ts b/packages/angular/cli/src/package-managers/testing/mock-host.ts index 0a22266467a0..ae4476c6501d 100644 --- a/packages/angular/cli/src/package-managers/testing/mock-host.ts +++ b/packages/angular/cli/src/package-managers/testing/mock-host.ts @@ -19,7 +19,16 @@ export class MockHost implements Host { constructor(files: Record = {}) { // Normalize paths to use forward slashes for consistency in tests. for (const [path, content] of Object.entries(files)) { - this.fs.set(path.replace(/\\/g, '/'), content); + const normalizedPath = path.replace(/\\/g, '/'); + this.fs.set(normalizedPath, content); + + // If the content is an array (directory listing), create entries for the files in it. + if (Array.isArray(content)) { + for (const file of content) { + const filePath = normalizedPath === '/' ? `/${file}` : `${normalizedPath}/${file}`; + this.fs.set(filePath, []); // Use empty array to represent a file (not `true` which is a dir) + } + } } } @@ -34,17 +43,11 @@ export class MockHost implements Host { } // A `true` value signifies a directory in our mock file system. - return Promise.resolve({ isDirectory: () => content === true } as Stats); - } - - readdir(path: string): Promise { - const content = this.fs.get(path.replace(/\\/g, '/')); - if (content === true || content === undefined) { - // This should be a directory with a file list. - return Promise.reject(new Error(`Directory not found or not a directory: ${path}`)); - } - - return Promise.resolve(content); + // Anything else is considered a file for the purpose of this mock. + return Promise.resolve({ + isDirectory: () => content === true, + isFile: () => content !== true, + } as Stats); } runCommand(): Promise<{ stdout: string; stderr: string }> {