From b1d6fad5e3727fef705c2e5a7bfcc82ab515c9d4 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 25 Sep 2021 18:46:59 +0200 Subject: [PATCH] refactor(compiler-cli): do not use `__filename` or `__dirname` global for ESM compatibility (#43431) Switches the compiler-cli usage of `__filename` to `import.meta.url` when ESM bundles are generated. Unfortunately we cannot start using only `import.meta` yet as we still build and run all code in Angular in CommonJS module output for devmode tests. This commit also fixes various instances where a jasmine spy was applied on a namespace export that will break with ES module (and the interop for CommonJS output). We fix these spies by using a default import. PR Close #43431 --- packages/compiler-cli/BUILD.bazel | 7 +++++++ packages/compiler-cli/import_meta_url.d.ts | 14 ++++++++++++++ packages/compiler-cli/ngcc/BUILD.bazel | 1 + packages/compiler-cli/ngcc/index.ts | 15 ++++++++++++--- .../ngcc/src/execution/cluster/master.ts | 17 +++++++++++++++-- .../lock_file_with_child_process/index.ts | 16 +++++++++++++++- .../ngcc/src/packages/configuration.ts | 9 ++++++++- .../ngcc/test/integration/ngcc_spec.ts | 6 +++--- .../compiler-cli/ngcc/test/ngcc_options_spec.ts | 5 ++++- packages/compiler-cli/package.json | 6 ++++++ .../src/ngtsc/file_system/BUILD.bazel | 3 +++ .../file_system/src/node_js_file_system.ts | 17 +++++++++++++++-- .../test/node_js_file_system_spec.ts | 6 +++++- .../test/reflector_host_spec.ts | 2 +- .../test/typescript_host_spec.ts | 5 ++++- 15 files changed, 113 insertions(+), 16 deletions(-) create mode 100644 packages/compiler-cli/import_meta_url.d.ts diff --git a/packages/compiler-cli/BUILD.bazel b/packages/compiler-cli/BUILD.bazel index c0d3110687f..28931d8e288 100644 --- a/packages/compiler-cli/BUILD.bazel +++ b/packages/compiler-cli/BUILD.bazel @@ -68,6 +68,11 @@ ts_config( deps = ["//packages:tsconfig-build.json"], ) +ts_library( + name = "import_meta_url_types", + srcs = ["import_meta_url.d.ts"], +) + ts_library( name = "compiler-cli", srcs = glob( @@ -76,11 +81,13 @@ ts_library( "src/**/*.ts", ], exclude = [ + "import_meta_url.d.ts", "src/integrationtest/**/*.ts", ], ), tsconfig = ":tsconfig", deps = [ + ":import_meta_url_types", "//packages/compiler", "//packages/compiler-cli/src/ngtsc/core", "//packages/compiler-cli/src/ngtsc/core:api", diff --git a/packages/compiler-cli/import_meta_url.d.ts b/packages/compiler-cli/import_meta_url.d.ts new file mode 100644 index 00000000000..ae46c8b1a1b --- /dev/null +++ b/packages/compiler-cli/import_meta_url.d.ts @@ -0,0 +1,14 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * 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: The `__ESM_IMPORT_META_URL__` identifier will be defined as part of bundling. +// We cannot use `import.meta.url` directly as this code currently still runs in CommonJS +// for devmode. This is a solution allowing for both ESModule and CommonJS to run this file. +// TODO(devversion): replace all of this with `import.meta.url` once devmode is using ESM. +declare const __ESM_IMPORT_META_URL__: string; \ No newline at end of file diff --git a/packages/compiler-cli/ngcc/BUILD.bazel b/packages/compiler-cli/ngcc/BUILD.bazel index 6502721b914..7c1ddf6cb8e 100644 --- a/packages/compiler-cli/ngcc/BUILD.bazel +++ b/packages/compiler-cli/ngcc/BUILD.bazel @@ -13,6 +13,7 @@ ts_library( "//packages:types", "//packages/compiler", "//packages/compiler-cli", + "//packages/compiler-cli:import_meta_url_types", "//packages/compiler-cli/src/ngtsc/annotations", "//packages/compiler-cli/src/ngtsc/cycles", "//packages/compiler-cli/src/ngtsc/diagnostics", diff --git a/packages/compiler-cli/ngcc/index.ts b/packages/compiler-cli/ngcc/index.ts index dc1ea337953..686431070e5 100644 --- a/packages/compiler-cli/ngcc/index.ts +++ b/packages/compiler-cli/ngcc/index.ts @@ -6,7 +6,9 @@ * found in the LICENSE file at https://angular.io/license */ -import {join} from 'path'; +import {dirname, join} from 'path'; +import {fileURLToPath} from 'url'; + import {NodeJSFileSystem, setFileSystem} from '../src/ngtsc/file_system'; import {mainNgcc} from './src/main'; @@ -23,10 +25,17 @@ export function process(options: AsyncNgccOptions|SyncNgccOptions): void|Promise return mainNgcc(options); } + +// CommonJS/ESM interop for determining the current file name and containing +// directory. These path is needed for providing an absolute path to the ngcc +// command line entry-point script (for the CLI). +export const containingDirPath = + typeof __dirname !== 'undefined' ? __dirname : dirname(fileURLToPath(__ESM_IMPORT_META_URL__)); + /** - * Absolute file path that points to the `ngcc` entry-point. + * Absolute file path that points to the `ngcc` command line entry-point. * * This can be used by the Angular CLI to spawn a process running ngcc using * command line options. */ -export const ngccMainFilePath = join(__dirname, './main-ngcc.js'); +export const ngccMainFilePath = join(containingDirPath, './main-ngcc.js'); diff --git a/packages/compiler-cli/ngcc/src/execution/cluster/master.ts b/packages/compiler-cli/ngcc/src/execution/cluster/master.ts index 759e4f3dde0..77d84aec9e6 100644 --- a/packages/compiler-cli/ngcc/src/execution/cluster/master.ts +++ b/packages/compiler-cli/ngcc/src/execution/cluster/master.ts @@ -9,6 +9,7 @@ /// import cluster from 'cluster'; +import module from 'module'; import {AbsoluteFsPath, PathManipulation} from '../../../../src/ngtsc/file_system'; import {Logger} from '../../../../src/ngtsc/logging'; @@ -21,7 +22,6 @@ import {stringifyTask} from '../tasks/utils'; import {MessageFromWorker, TaskCompletedMessage, TransformedFilesMessage, UpdatePackageJsonMessage} from './api'; import {Deferred, sendMessageToWorker} from './utils'; - /** * The cluster master is responsible for analyzing all entry-points, planning the work that needs to * be done, distributing it to worker-processes and collecting/post-processing the results. @@ -44,7 +44,7 @@ export class ClusterMaster { } // Set the worker entry-point - cluster.setupMaster({exec: this.fileSystem.resolve(__dirname, 'ngcc_cluster_worker.js')}); + cluster.setupMaster({exec: getClusterWorkerScriptPath(fileSystem)}); this.taskQueue = analyzeEntryPoints(); this.onTaskCompleted = createTaskCompletedCallback(this.taskQueue); @@ -330,3 +330,16 @@ export class ClusterMaster { }; } } + +/** Gets the absolute file path to the cluster worker script. */ +export function getClusterWorkerScriptPath(fileSystem: PathManipulation): AbsoluteFsPath { + // This is an interop allowing for the worker script to be determined in both + // a CommonJS module, or an ES module which does not come with `require` by default. + const requireFn = + typeof require !== 'undefined' ? require : module.createRequire(__ESM_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_with_child_process/index.ts b/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts index 8a0c59a8a58..839c42edebe 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 @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {ChildProcess, fork} from 'child_process'; +import module from 'module'; import {AbsoluteFsPath, FileSystem} from '../../../../src/ngtsc/file_system'; import {Logger, LogLevel} from '../../../../src/ngtsc/logging'; @@ -78,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( - __dirname + '/ngcc_lock_unlocker.js', [path, logLevel], + getLockFileUnlockerScriptPath(this.fs), [path, logLevel], {detached: true, stdio: isWindows ? 'pipe' : 'inherit'}); if (isWindows) { unlocker.stdout?.on('data', process.stdout.write.bind(process.stdout)); @@ -87,3 +88,16 @@ export class LockFileWithChildProcess implements LockFile { return unlocker; } } + +/** Gets the absolute file path to the lock file unlocker script. */ +export function getLockFileUnlockerScriptPath(fileSystem: FileSystem): AbsoluteFsPath { + // This is an interop allowing for the unlocking script to be determined in both + // a CommonJS module, or an ES module which does not come with `require` by default. + const requireFn = + typeof require !== 'undefined' ? require : module.createRequire(__ESM_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); +} diff --git a/packages/compiler-cli/ngcc/src/packages/configuration.ts b/packages/compiler-cli/ngcc/src/packages/configuration.ts index c226efc0f57..13f3c857534 100644 --- a/packages/compiler-cli/ngcc/src/packages/configuration.ts +++ b/packages/compiler-cli/ngcc/src/packages/configuration.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ import {createHash} from 'crypto'; +import module from 'module'; import semver from 'semver'; import * as vm from 'vm'; @@ -267,6 +268,11 @@ export const DEFAULT_NGCC_CONFIG: NgccProjectConfig = { const NGCC_CONFIG_FILENAME = 'ngcc.config.js'; +// CommonJS/ESM interop for determining the current file name and containing +// directory. The path is needed for loading the user configuration. +const isCommonJS = typeof require !== 'undefined'; +const currentFileUrl = isCommonJS ? null : __ESM_IMPORT_META_URL__; + /** * The processed package level configuration as a result of processing a raw package level config. */ @@ -435,12 +441,13 @@ export class NgccConfiguration { } private evalSrcFile(srcPath: AbsoluteFsPath): any { + const requireFn = isCommonJS ? require : module.createRequire(currentFileUrl!); const src = this.fs.readFile(srcPath); const theExports = {}; const sandbox = { module: {exports: theExports}, exports: theExports, - require, + require: requireFn, __dirname: this.fs.dirname(srcPath), __filename: srcPath }; diff --git a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts index e6c091d8ff8..c80ec6ae5d2 100644 --- a/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts +++ b/packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts @@ -7,8 +7,8 @@ */ /// -import {readFileSync} from 'fs'; -import * as os from 'os'; +import realFs from 'fs'; +import os from 'os'; import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem} from '../../../src/ngtsc/file_system'; import {Folder, MockFileSystem, runInEachFileSystem, TestFile} from '../../../src/ngtsc/file_system/testing'; @@ -69,7 +69,7 @@ runInEachFileSystem(() => { fs.ensureDir(fs.join(pkgPath, 'fesm5')); fs.writeFile( fs.join(pkgPath, 'fesm5/core.js'), - readFileSync(require.resolve('../fesm5_angular_core.js'), 'utf8')); + realFs.readFileSync(require.resolve('../fesm5_angular_core.js'), 'utf8')); pkgJson.esm5 = './fesm5/core.js'; pkgJson.fesm5 = './fesm5/core.js'; diff --git a/packages/compiler-cli/ngcc/test/ngcc_options_spec.ts b/packages/compiler-cli/ngcc/test/ngcc_options_spec.ts index 31ee7522a58..f28c0a3a927 100644 --- a/packages/compiler-cli/ngcc/test/ngcc_options_spec.ts +++ b/packages/compiler-cli/ngcc/test/ngcc_options_spec.ts @@ -5,7 +5,10 @@ * 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'; +// 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. +import os from 'os'; import {absoluteFrom, AbsoluteFsPath, FileSystem, getFileSystem} from '../../src/ngtsc/file_system'; import {runInEachFileSystem} from '../../src/ngtsc/file_system/testing'; diff --git a/packages/compiler-cli/package.json b/packages/compiler-cli/package.json index 03d9e54a364..a9d5b1577bd 100644 --- a/packages/compiler-cli/package.json +++ b/packages/compiler-cli/package.json @@ -40,6 +40,12 @@ "./private/migrations": { "types": "./private/migrations.d.ts", "default": "./bundles/private/migrations.js" + }, + "./ngcc/src/execution/cluster/ngcc_cluster_worker": { + "default": "./bundles/ngcc/src/execution/cluster/ngcc_cluster_worker.js" + }, + "./ngcc/src/locking/lock_file_with_child_process/ngcc_lock_unlocker": { + "default": "./bundles/ngcc/src/locking/lock_file_with_child_process/ngcc_lock_unlocker.js" } }, "dependencies": { diff --git a/packages/compiler-cli/src/ngtsc/file_system/BUILD.bazel b/packages/compiler-cli/src/ngtsc/file_system/BUILD.bazel index 1dc56c80e76..bc181642c3f 100644 --- a/packages/compiler-cli/src/ngtsc/file_system/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/file_system/BUILD.bazel @@ -8,6 +8,9 @@ ts_library( "src/**/*.ts", ]), deps = [ + # Allows for the `import.meta.url` constant to be used. This is + # a temporary interop until devmode is switched to ESM. + "//packages/compiler-cli:import_meta_url_types", "@npm//@types/node", "@npm//typescript", ], diff --git a/packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts b/packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts index b9556b5043b..b926c95126f 100644 --- a/packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts +++ b/packages/compiler-cli/src/ngtsc/file_system/src/node_js_file_system.ts @@ -7,7 +7,10 @@ */ /// import * as fs from 'fs'; +import module from 'module'; import * as p from 'path'; +import {fileURLToPath} from 'url'; + import {AbsoluteFsPath, FileStats, FileSystem, PathManipulation, PathSegment, PathString, ReadonlyFileSystem} from './types'; /** @@ -51,6 +54,14 @@ export class NodeJSPathManipulation implements PathManipulation { } } +// CommonJS/ESM interop for determining the current file name and containing +// directory. These paths are needed for detecting whether the file system +// is case sensitive, or for finding the TypeScript default libraries. +// TODO(devversion): Simplify this in the future if devmode uses ESM as well. +const isCommonJS = typeof __filename !== 'undefined'; +const currentFileUrl = isCommonJS ? null : __ESM_IMPORT_META_URL__; +const currentFileName = isCommonJS ? __filename : fileURLToPath(currentFileUrl!); + /** * A wrapper around the Node.js file-system that supports readonly operations and path manipulation. */ @@ -60,7 +71,7 @@ export class NodeJSReadonlyFileSystem extends NodeJSPathManipulation implements if (this._caseSensitive === undefined) { // Note the use of the real file-system is intentional: // `this.exists()` relies upon `isCaseSensitive()` so that would cause an infinite recursion. - this._caseSensitive = !fs.existsSync(this.normalize(toggleCase(__filename))); + this._caseSensitive = !fs.existsSync(this.normalize(toggleCase(currentFileName))); } return this._caseSensitive; } @@ -86,7 +97,9 @@ export class NodeJSReadonlyFileSystem extends NodeJSPathManipulation implements return this.resolve(fs.realpathSync(path)); } getDefaultLibLocation(): AbsoluteFsPath { - return this.resolve(require.resolve('typescript'), '..'); + // TODO(devversion): Once devmode output uses ESM, we can simplify this. + const requireFn = isCommonJS ? require : module.createRequire(currentFileUrl!); + return this.resolve(requireFn.resolve('typescript'), '..'); } } diff --git a/packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts b/packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts index dc9dbf43627..b973d201aa5 100644 --- a/packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts +++ b/packages/compiler-cli/src/ngtsc/file_system/test/node_js_file_system_spec.ts @@ -5,8 +5,12 @@ * 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 realFs from 'fs'; +// 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. +import realFs from 'fs'; import * as os from 'os'; + import {NodeJSFileSystem, NodeJSPathManipulation, NodeJSReadonlyFileSystem} from '../src/node_js_file_system'; import {AbsoluteFsPath, PathSegment} from '../src/types'; diff --git a/packages/language-service/test/reflector_host_spec.ts b/packages/language-service/test/reflector_host_spec.ts index 96f1a10ac34..79850af500b 100644 --- a/packages/language-service/test/reflector_host_spec.ts +++ b/packages/language-service/test/reflector_host_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import * as path from 'path'; +import path from 'path'; import ts from 'typescript'; import {ReflectorHost} from '../src/reflector_host'; diff --git a/packages/language-service/test/typescript_host_spec.ts b/packages/language-service/test/typescript_host_spec.ts index 21156312894..e549e01eb9c 100644 --- a/packages/language-service/test/typescript_host_spec.ts +++ b/packages/language-service/test/typescript_host_spec.ts @@ -6,7 +6,10 @@ * found in the LICENSE file at https://angular.io/license */ -import * as path from 'path'; +// 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 `path`. Using +// the default export gives us an object where we can patch properties on. +import path from 'path'; import ts from 'typescript'; import {TypeScriptServiceHost} from '../src/typescript_host';