From 619f0900a295714afc9fa183ddc71abcd793a9cf Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 13 Dec 2022 17:32:50 +0000 Subject: [PATCH] refactor: change ngcc to only rely on ESM features (#48521) Refactors ngcc to only rely on ESM features because CJS is no longer needed & tested. PR Close #48521 --- .../ngcc/src/execution/cluster/master.ts | 22 +++++++------ .../ngcc/src/locking/lock_file.ts | 21 ++++++------ .../lock_file_with_child_process/index.ts | 26 ++++++++------- .../compiler-cli/ngcc/src/ngcc_options.ts | 2 +- packages/compiler-cli/ngcc/test/BUILD.bazel | 1 + .../test/execution/cluster/executor_spec.ts | 3 ++ .../compiler-cli/ngcc/test/helpers/utils.ts | 32 +++++++++++++------ .../ngcc/test/integration/ngcc_spec.ts | 13 +++++--- .../ngcc/test/integration/util.ts | 12 ++++--- .../lockfile_with_child_process/index_spec.ts | 9 +++--- .../unlocker_spec.ts | 4 +-- .../ngcc/test/ngcc_options_spec.ts | 2 +- 12 files changed, 90 insertions(+), 57 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/execution/cluster/master.ts b/packages/compiler-cli/ngcc/src/execution/cluster/master.ts index 126b8d2cc8f..050aaffa949 100644 --- a/packages/compiler-cli/ngcc/src/execution/cluster/master.ts +++ b/packages/compiler-cli/ngcc/src/execution/cluster/master.ts @@ -44,7 +44,7 @@ export class ClusterMaster { } // Set the worker entry-point - cluster.setupMaster({exec: getClusterWorkerScriptPath(fileSystem)}); + cluster.setupMaster({exec: ClusterWorkerScriptResolver.resolve(fileSystem)}); this.taskQueue = analyzeEntryPoints(); this.onTaskCompleted = createTaskCompletedCallback(this.taskQueue); @@ -343,13 +343,15 @@ export class ClusterMaster { } } -/** Gets the absolute file path to the cluster worker script. */ -export function getClusterWorkerScriptPath(fileSystem: PathManipulation): AbsoluteFsPath { - // NodeJS `import.meta.resolve` is experimental. We leverage `require`. - const requireFn = module.createRequire(import.meta.url); - // We resolve the worker script using module resolution as in the package output, - // the worker might be bundled but exposed through a subpath export mapping. - const workerScriptPath = - requireFn.resolve('@angular/compiler-cli/ngcc/src/execution/cluster/ngcc_cluster_worker'); - return fileSystem.resolve(workerScriptPath); +/** Wrapper for resolving the cluster worker script. Useful for test patching. */ +export class ClusterWorkerScriptResolver { + static resolve(fileSystem: PathManipulation): AbsoluteFsPath { + // NodeJS `import.meta.resolve` is experimental. We leverage `require`. + const requireFn = module.createRequire(import.meta.url); + // We resolve the worker script using module resolution as in the package output, + // the worker might be bundled but exposed through a subpath export mapping. + const workerScriptPath = + requireFn.resolve('@angular/compiler-cli/ngcc/src/execution/cluster/ngcc_cluster_worker'); + return fileSystem.resolve(workerScriptPath); + } } diff --git a/packages/compiler-cli/ngcc/src/locking/lock_file.ts b/packages/compiler-cli/ngcc/src/locking/lock_file.ts index 22b170a1e4c..4ffb94daaf3 100644 --- a/packages/compiler-cli/ngcc/src/locking/lock_file.ts +++ b/packages/compiler-cli/ngcc/src/locking/lock_file.ts @@ -10,15 +10,18 @@ import module from 'module'; import {AbsoluteFsPath, PathManipulation} from '../../../src/ngtsc/file_system'; -export function getLockFilePath(fs: PathManipulation) { - // NodeJS `import.meta.resolve` is experimental. We leverage `require`. - const requireFn = module.createRequire(import.meta.url); - // The lock file location is resolved based on the location of the `ngcc` entry-point as this - // allows us to have a consistent position for the lock file to reside. We are unable to rely - // on `__dirname` (or equivalent) as this code is being bundled and different entry-points - // will have dedicated bundles where the lock file location would differ then. - const ngccEntryPointFile = requireFn.resolve('@angular/compiler-cli/package.json'); - return fs.resolve(ngccEntryPointFile, '../../../.ngcc_lock_file'); +/** Wrapper for resolving the lcok file path. Useful for test patching. */ +export class LockFilePathResolver { + static resolve(fs: PathManipulation): AbsoluteFsPath { + // NodeJS `import.meta.resolve` is experimental. We leverage `require`. + const requireFn = module.createRequire(import.meta.url); + // The lock file location is resolved based on the location of the `ngcc` entry-point as this + // allows us to have a consistent position for the lock file to reside. We are unable to rely + // on `__dirname` (or equivalent) as this code is being bundled and different entry-points + // will have dedicated bundles where the lock file location would differ then. + const ngccEntryPointFile = requireFn.resolve('@angular/compiler-cli/package.json'); + return fs.resolve(ngccEntryPointFile, '../../../.ngcc_lock_file'); + } } export interface LockFile { diff --git a/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts b/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts index e7d8c89bf08..59ca5187d25 100644 --- a/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts +++ b/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts @@ -10,7 +10,7 @@ import module from 'module'; import {AbsoluteFsPath, FileSystem} from '../../../../src/ngtsc/file_system'; import {Logger, LogLevel} from '../../../../src/ngtsc/logging'; -import {getLockFilePath, LockFile} from '../lock_file'; +import {LockFile, LockFilePathResolver} from '../lock_file'; import {removeLockFile} from './util'; @@ -38,7 +38,7 @@ export class LockFileWithChildProcess implements LockFile { private unlocker: ChildProcess|null; constructor(protected fs: FileSystem, protected logger: Logger) { - this.path = getLockFilePath(fs); + this.path = LockFilePathResolver.resolve(fs); this.unlocker = this.createUnlocker(this.path); } @@ -79,7 +79,7 @@ export class LockFileWithChildProcess implements LockFile { this.logger.level !== undefined ? this.logger.level.toString() : LogLevel.info.toString(); const isWindows = process.platform === 'win32'; const unlocker = fork( - getLockFileUnlockerScriptPath(this.fs), [path, logLevel], + LockFileUnlockerScriptResolver.resolve(this.fs), [path, logLevel], {detached: true, stdio: isWindows ? 'pipe' : 'inherit'}); if (isWindows) { unlocker.stdout?.on('data', process.stdout.write.bind(process.stdout)); @@ -89,13 +89,15 @@ export class LockFileWithChildProcess implements LockFile { } } -/** Gets the absolute file path to the lock file unlocker script. */ -export function getLockFileUnlockerScriptPath(fileSystem: FileSystem): AbsoluteFsPath { - // NodeJS `import.meta.resolve` is experimental. We leverage `require`. - const requireFn = module.createRequire(import.meta.url); - // We resolve the worker script using module resolution as in the package output, - // the worker might be bundled but exposed through a subpath export mapping. - const unlockerScriptPath = requireFn.resolve( - '@angular/compiler-cli/ngcc/src/locking/lock_file_with_child_process/ngcc_lock_unlocker'); - return fileSystem.resolve(unlockerScriptPath); +/** Wrapper for resolving the lock file unlocker script. Useful for test patching. */ +export class LockFileUnlockerScriptResolver { + static resolve(fs: FileSystem): AbsoluteFsPath { + // NodeJS `import.meta.resolve` is experimental. We leverage `require`. + const requireFn = module.createRequire(import.meta.url); + // We resolve the worker script using module resolution as in the package output, + // the worker might be bundled but exposed through a subpath export mapping. + const unlockerScriptPath = requireFn.resolve( + '@angular/compiler-cli/ngcc/src/locking/lock_file_with_child_process/ngcc_lock_unlocker'); + return fs.resolve(unlockerScriptPath); + } } diff --git a/packages/compiler-cli/ngcc/src/ngcc_options.ts b/packages/compiler-cli/ngcc/src/ngcc_options.ts index 7acc6373606..4c078977334 100644 --- a/packages/compiler-cli/ngcc/src/ngcc_options.ts +++ b/packages/compiler-cli/ngcc/src/ngcc_options.ts @@ -5,7 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -import * as os from 'os'; +import os from 'os'; import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem, PathManipulation} from '../../src/ngtsc/file_system'; import {ConsoleLogger, Logger, LogLevel} from '../../src/ngtsc/logging'; diff --git a/packages/compiler-cli/ngcc/test/BUILD.bazel b/packages/compiler-cli/ngcc/test/BUILD.bazel index b14cf705894..092708deeb4 100644 --- a/packages/compiler-cli/ngcc/test/BUILD.bazel +++ b/packages/compiler-cli/ngcc/test/BUILD.bazel @@ -60,6 +60,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/file_system/testing", "//packages/compiler-cli/src/ngtsc/logging/testing", "//packages/compiler-cli/src/ngtsc/testing", + "@npm//@bazel/runfiles", "@npm//rxjs", "@npm//typescript", ], diff --git a/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts b/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts index 32e6cfd0de0..511c9608ca1 100644 --- a/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts +++ b/packages/compiler-cli/ngcc/test/execution/cluster/executor_spec.ts @@ -20,6 +20,7 @@ import {FileWriter} from '../../../src/writing/file_writer'; import {PackageJsonUpdater} from '../../../src/writing/package_json_updater'; import {MockLockFile} from '../../helpers/mock_lock_file'; import {mockProperty} from '../../helpers/spy_utils'; +import {mockRequireResolveForWorkerScript} from '../../helpers/utils'; runInEachFileSystem(() => { describe('ClusterExecutor', () => { @@ -33,6 +34,8 @@ runInEachFileSystem(() => { let createTaskCompletedCallback: jasmine.Spy; beforeEach(() => { + mockRequireResolveForWorkerScript(); + masterRunSpy = spyOn(ClusterMaster.prototype, 'run') .and.returnValue(Promise.resolve('ClusterMaster#run()' as any)); createTaskCompletedCallback = jasmine.createSpy('createTaskCompletedCallback'); diff --git a/packages/compiler-cli/ngcc/test/helpers/utils.ts b/packages/compiler-cli/ngcc/test/helpers/utils.ts index 0063f374e21..2f2d4220581 100644 --- a/packages/compiler-cli/ngcc/test/helpers/utils.ts +++ b/packages/compiler-cli/ngcc/test/helpers/utils.ts @@ -9,7 +9,10 @@ import ts from 'typescript'; import {absoluteFrom, AbsoluteFsPath, getFileSystem, NgtscCompilerHost} from '../../../src/ngtsc/file_system'; import {TestFile} from '../../../src/ngtsc/file_system/testing'; +import {ClusterWorkerScriptResolver} from '../../src/execution/cluster/master'; import {DtsProcessing} from '../../src/execution/tasks/api'; +import {LockFilePathResolver} from '../../src/locking/lock_file'; +import {LockFileUnlockerScriptResolver} from '../../src/locking/lock_file_with_child_process'; import {BundleProgram, makeBundleProgram} from '../../src/packages/bundle_program'; import {NgccEntryPointConfig} from '../../src/packages/configuration'; import {EntryPoint, EntryPointFormat} from '../../src/packages/entry_point'; @@ -125,13 +128,24 @@ export function getRootFiles(testFiles: TestFile[]): AbsoluteFsPath[] { * Mock out the lockfile path resolution, which uses `require.resolve()`. */ export function mockRequireResolveForLockfile() { - const moduleConstructor: any = module.constructor; - const originalResolveFileName = moduleConstructor._resolveFilename; - spyOn(moduleConstructor, '_resolveFilename').and.callFake(function(request: string) { - if (request === '@angular/compiler-cli/package.json') { - return '/node_modules/' + request; - } else { - return originalResolveFileName.apply(null, arguments as any); - } - }); + spyOn(LockFilePathResolver, 'resolve') + .and.returnValue(absoluteFrom('/node_modules/@angular/compiler-cli/package.json')); +} + +/** + * Mock out the worker script resolution, which uses `require.resolve()`. + */ +export function mockRequireResolveForWorkerScript() { + spyOn(ClusterWorkerScriptResolver, 'resolve') + .and.returnValue(absoluteFrom( + '/node_modules/@angular/compiler-cli/ngcc/src/execution/cluster/ngcc_cluster_worker')); +} + +/** + * Mock out the lock file unlocker script resolution, which uses `require.resolve()`. + */ +export function mockRequireResolveForLockFileUnlockerScript() { + spyOn(LockFileUnlockerScriptResolver, 'resolve') + .and.returnValue(absoluteFrom( + '/node_modules/@angular/compiler-cli/ngcc/src/locking/lock_file_with_child_process/ngcc_lock_unlocker')); } diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index 26e4e81e72d..56d974e9c13 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -7,6 +7,7 @@ */ /// +import {runfiles} from '@bazel/runfiles'; import realFs from 'fs'; import os from 'os'; @@ -14,7 +15,7 @@ import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem} from '../../../ import {Folder, MockFileSystem, runInEachFileSystem, TestFile} from '../../../src/ngtsc/file_system/testing'; import {MockLogger} from '../../../src/ngtsc/logging/testing'; import {loadTestFiles} from '../../../src/ngtsc/testing'; -import {getLockFilePath} from '../../src/locking/lock_file'; +import {LockFilePathResolver} from '../../src/locking/lock_file'; import {mainNgcc} from '../../src/main'; import {clearTsConfigCache} from '../../src/ngcc_options'; import {hasBeenProcessed, markAsProcessed} from '../../src/packages/build_marker'; @@ -22,7 +23,7 @@ import {EntryPointJsonProperty, EntryPointPackageJson, SUPPORTED_FORMAT_PROPERTI import {EntryPointManifestFile} from '../../src/packages/entry_point_manifest'; import {Transformer} from '../../src/packages/transformer'; import {DirectPackageJsonUpdater, PackageJsonUpdater} from '../../src/writing/package_json_updater'; -import {mockRequireResolveForLockfile} from '../helpers/utils'; +import {mockRequireResolveForLockfile, mockRequireResolveForLockFileUnlockerScript, mockRequireResolveForWorkerScript} from '../helpers/utils'; import {loadNgccIntegrationTestFiles} from './util'; @@ -47,7 +48,11 @@ runInEachFileSystem(() => { _ = absoluteFrom; fs = getFileSystem(); pkgJsonUpdater = new DirectPackageJsonUpdater(fs); + mockRequireResolveForLockfile(); + mockRequireResolveForWorkerScript(); + mockRequireResolveForLockFileUnlockerScript(); + initMockFileSystem(fs, testFiles); // Force single-process execution in unit tests by mocking available CPUs to 1. @@ -71,7 +76,7 @@ runInEachFileSystem(() => { fs.ensureDir(fs.join(pkgPath, 'fesm5')); fs.writeFile( fs.join(pkgPath, 'fesm5/core.js'), - realFs.readFileSync(require.resolve('../fesm5_angular_core.js'), 'utf8')); + realFs.readFileSync(runfiles.resolvePackageRelative('./fesm5_angular_core.js'), 'utf8')); pkgJson.esm5 = './fesm5/core.js'; pkgJson.fesm5 = './fesm5/core.js'; @@ -3940,7 +3945,7 @@ runInEachFileSystem(() => { function initMockFileSystem(fs: FileSystem, testFiles: Folder) { if (fs instanceof MockFileSystem) { fs.init(testFiles); - fs.ensureDir(fs.dirname(getLockFilePath(fs))); + fs.ensureDir(fs.dirname(LockFilePathResolver.resolve(fs))); } // a random test package that no metadata.json file so not compiled by Angular. diff --git a/packages/compiler-cli/ngcc/test/integration/util.ts b/packages/compiler-cli/ngcc/test/integration/util.ts index 6fa9e211d8b..661cde2a868 100644 --- a/packages/compiler-cli/ngcc/test/integration/util.ts +++ b/packages/compiler-cli/ngcc/test/integration/util.ts @@ -8,7 +8,7 @@ import {AbsoluteFsPath} from '../../../src/ngtsc/file_system'; import {Folder, MockFileSystemPosix} from '../../../src/ngtsc/file_system/testing'; -import {loadTestDirectory, loadTsLib, resolveNpmTreeArtifact} from '../../../src/ngtsc/testing'; +import {loadTestDirectory, loadTsLib, resolveFromRunfiles} from '../../../src/ngtsc/testing'; export function loadNgccIntegrationTestFiles(): Folder { const tmpFs = new MockFileSystemPosix(true); @@ -16,13 +16,15 @@ export function loadNgccIntegrationTestFiles(): Folder { loadTsLib(tmpFs, basePath); loadTestDirectory( - tmpFs, resolveNpmTreeArtifact('@angular/core-12'), + tmpFs, resolveFromRunfiles('npm/node_modules/@angular/core-12'), tmpFs.resolve('/node_modules/@angular/core')); loadTestDirectory( - tmpFs, resolveNpmTreeArtifact('@angular/common-12'), + tmpFs, resolveFromRunfiles('npm/node_modules/@angular/common-12'), tmpFs.resolve('/node_modules/@angular/common')); loadTestDirectory( - tmpFs, resolveNpmTreeArtifact('typescript'), tmpFs.resolve('/node_modules/typescript')); - loadTestDirectory(tmpFs, resolveNpmTreeArtifact('rxjs'), tmpFs.resolve('/node_modules/rxjs')); + tmpFs, resolveFromRunfiles('npm/node_modules/typescript'), + tmpFs.resolve('/node_modules/typescript')); + loadTestDirectory( + tmpFs, resolveFromRunfiles('npm/node_modules/rxjs'), tmpFs.resolve('/node_modules/rxjs')); return tmpFs.dump(); } diff --git a/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/index_spec.ts b/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/index_spec.ts index 95b84b9a42f..7f16e47fbc8 100644 --- a/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/index_spec.ts +++ b/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/index_spec.ts @@ -11,7 +11,7 @@ import * as process from 'process'; import {FileSystem, getFileSystem} from '../../../../src/ngtsc/file_system'; import {runInEachFileSystem} from '../../../../src/ngtsc/file_system/testing'; import {MockLogger} from '../../../../src/ngtsc/logging/testing'; -import {getLockFilePath} from '../../../src/locking/lock_file'; +import {LockFilePathResolver} from '../../../src/locking/lock_file'; import {LockFileWithChildProcess} from '../../../src/locking/lock_file_with_child_process'; import {mockRequireResolveForLockfile} from '../../helpers/utils'; @@ -72,10 +72,11 @@ runInEachFileSystem(() => { it('should write the lock-file to disk', () => { const fs = getFileSystem(); const lockFile = new LockFileUnderTest(fs); - expect(fs.exists(getLockFilePath(fs))).toBe(false); + const lockPath = LockFilePathResolver.resolve(fs); + expect(fs.exists(lockPath)).toBe(false); lockFile.write(); - expect(fs.exists(getLockFilePath(fs))).toBe(true); - expect(fs.readFile(getLockFilePath(fs))).toEqual('' + process.pid); + expect(fs.exists(lockPath)).toBe(true); + expect(fs.readFile(lockPath)).toEqual('' + process.pid); }); it('should create the unlocker process if it is not already created', () => { diff --git a/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/unlocker_spec.ts b/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/unlocker_spec.ts index 83b9bc5b4e4..28d1d932ba9 100644 --- a/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/unlocker_spec.ts +++ b/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/unlocker_spec.ts @@ -9,9 +9,9 @@ /// describe('unlocker', () => { - it('should attach a handler to the `disconnect` event', () => { + it('should attach a handler to the `disconnect` event', async () => { spyOn(process, 'on'); - require('../../../src/locking/lock_file_with_child_process/ngcc_lock_unlocker'); + await import('../../../src/locking/lock_file_with_child_process/ngcc_lock_unlocker'); // TODO: @JiaLiPassion, need to wait for @types/jasmine to handle the override case // https://github.com/DefinitelyTyped/DefinitelyTyped/issues/42455 expect(process.on).toHaveBeenCalledWith('disconnect' as any, jasmine.any(Function)); diff --git a/packages/compiler-cli/ngcc/test/ngcc_options_spec.ts b/packages/compiler-cli/ngcc/test/ngcc_options_spec.ts index f28c0a3a927..5f9c9c56246 100644 --- a/packages/compiler-cli/ngcc/test/ngcc_options_spec.ts +++ b/packages/compiler-cli/ngcc/test/ngcc_options_spec.ts @@ -5,6 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ + // Note: We do not use a namespace import here because this will result in the // named exports being modified if we apply jasmine spies on `realFs`. Using // the default export gives us an object where we can patch properties on. @@ -13,7 +14,6 @@ import os from 'os'; import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem} from '../../src/ngtsc/file_system'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; import {MockLogger} from '../../src/ngtsc/logging/testing'; - import {clearTsConfigCache, getMaxNumberOfWorkers, getSharedSetup, NgccOptions} from '../src/ngcc_options';