From b2a36d80db9560ff9c33835d88cdb47b403a7fe0 Mon Sep 17 00:00:00 2001 From: arturovt Date: Sun, 4 Feb 2024 22:30:57 +0200 Subject: [PATCH] fix(common): update TS module resolution flow This commit updates the implementation for resolving `.ts` files. Instead of registering the `ts-node` project only once, we now refrain from doing so since there might be multiple projects with different configurations. The current approach involves dynamically switching the implementation for registering and unregistering the project after the `.ts` file has been transpiled and resolved. This change addresses an issue where warnings were encountered when `ts-node` attempted to register with different configurations. The number of configurations is no longer a concern, as each time we need to read a `.ts` file, a new TS project is registered. This adjustment does not impact performance or other attributes because `ts-node` allows native project disabling. Part of the implementation has been adapted from what Nrwl Nx already has; we can find their implementation here: https://github.com/nrwl/nx/blob/master/packages/nx/src/plugins/js/utils/register.ts It's worth noting that their implementation is somewhat versatile, as it also supports SWC. Closes: #1197 Closes: #1213 --- .../sanity-app-esm/custom-webpack.config.ts | 7 ++ .../sanity-app-esm/package.json | 3 +- .../webpack-esm-plugin/package.json | 17 ++++ .../webpack-esm-plugin.d.ts | 5 ++ .../webpack-esm-plugin/webpack-esm-plugin.mjs | 7 ++ packages/common/package.json | 1 - packages/common/src/load-module.ts | 83 ++---------------- packages/common/src/register-ts-project.ts | 87 +++++++++++++++++++ .../custom-esbuild/src/application/index.ts | 8 +- .../custom-esbuild/src/dev-server/index.ts | 19 +--- packages/custom-esbuild/src/load-plugins.ts | 6 +- .../src/custom-webpack-builder.ts | 3 +- .../src/transform-factories.spec.ts | 11 +-- .../custom-webpack/src/transform-factories.ts | 9 +- packages/jest/src/custom-config.resolver.ts | 4 +- yarn.lock | 9 +- 16 files changed, 160 insertions(+), 119 deletions(-) create mode 100644 examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/package.json create mode 100644 examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/webpack-esm-plugin.d.ts create mode 100644 examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/webpack-esm-plugin.mjs create mode 100644 packages/common/src/register-ts-project.ts diff --git a/examples/custom-webpack/sanity-app-esm/custom-webpack.config.ts b/examples/custom-webpack/sanity-app-esm/custom-webpack.config.ts index f578ffa6da..571973cdda 100644 --- a/examples/custom-webpack/sanity-app-esm/custom-webpack.config.ts +++ b/examples/custom-webpack/sanity-app-esm/custom-webpack.config.ts @@ -1,7 +1,14 @@ import type { Configuration } from 'webpack'; +import { WebpackEsmPlugin } from 'webpack-esm-plugin'; + export default async (cfg: Configuration) => { const { default: configFromEsm } = await import('./custom-webpack.config.js'); + + // This is used to ensure we fixed the following issue: + // https://github.com/just-jeb/angular-builders/issues/1213 + cfg.plugins!.push(new WebpackEsmPlugin()); + // Do some stuff with config and configFromEsm return { ...cfg, ...configFromEsm }; }; diff --git a/examples/custom-webpack/sanity-app-esm/package.json b/examples/custom-webpack/sanity-app-esm/package.json index 0e4ad28331..c7541b31e1 100644 --- a/examples/custom-webpack/sanity-app-esm/package.json +++ b/examples/custom-webpack/sanity-app-esm/package.json @@ -44,6 +44,7 @@ "karma-jasmine": "5.1.0", "karma-jasmine-html-reporter": "2.1.0", "puppeteer": "21.10.0", - "typescript": "5.3.3" + "typescript": "5.3.3", + "webpack-esm-plugin": "file:./webpack-esm-plugin" } } diff --git a/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/package.json b/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/package.json new file mode 100644 index 0000000000..dfd8a05061 --- /dev/null +++ b/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/package.json @@ -0,0 +1,17 @@ +{ + "name": "webpack-esm-plugin", + "version": "0.0.1", + "module": "./webpack-esm-plugin.mjs", + "typings": "./webpack-esm-plugin.d.ts", + "exports": { + "./package.json": { + "default": "./package.json" + }, + ".": { + "types": "./webpack-esm-plugin.d.ts", + "node": "./webpack-esm-plugin.mjs", + "default": "./webpack-esm-plugin.mjs" + } + }, + "sideEffects": false +} diff --git a/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/webpack-esm-plugin.d.ts b/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/webpack-esm-plugin.d.ts new file mode 100644 index 0000000000..9f1d912306 --- /dev/null +++ b/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/webpack-esm-plugin.d.ts @@ -0,0 +1,5 @@ +import * as webpack from 'webpack'; + +export declare class WebpackEsmPlugin { + apply(compiler: webpack.Compiler): void; +} diff --git a/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/webpack-esm-plugin.mjs b/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/webpack-esm-plugin.mjs new file mode 100644 index 0000000000..9b001b658f --- /dev/null +++ b/examples/custom-webpack/sanity-app-esm/webpack-esm-plugin/webpack-esm-plugin.mjs @@ -0,0 +1,7 @@ +class WebpackEsmPlugin { + apply(compiler) { + console.error('hello from the WebpackEsmPlugin'); + } +} + +export { WebpackEsmPlugin }; diff --git a/packages/common/package.json b/packages/common/package.json index 488bb7f059..03b4e7eb03 100644 --- a/packages/common/package.json +++ b/packages/common/package.json @@ -20,7 +20,6 @@ "clean": "rimraf dist" }, "dependencies": { - "@angular-devkit/core": "^17.1.0", "ts-node": "^10.0.0", "tsconfig-paths": "^4.1.0" }, diff --git a/packages/common/src/load-module.ts b/packages/common/src/load-module.ts index cfeca85ae0..75db6d3bf7 100644 --- a/packages/common/src/load-module.ts +++ b/packages/common/src/load-module.ts @@ -1,57 +1,7 @@ import * as path from 'node:path'; import * as url from 'node:url'; -import type { logging } from '@angular-devkit/core'; -const _tsNodeRegister = (() => { - let lastTsConfig: string | undefined; - return (tsConfig: string, logger: logging.LoggerApi) => { - // Check if the function was previously called with the same tsconfig - if (lastTsConfig && lastTsConfig !== tsConfig) { - logger.warn(`Trying to register ts-node again with a different tsconfig - skipping the registration. - tsconfig 1: ${lastTsConfig} - tsconfig 2: ${tsConfig}`); - } - - if (lastTsConfig) { - return; - } - - lastTsConfig = tsConfig; - - loadTsNode().register({ - project: tsConfig, - compilerOptions: { - module: 'CommonJS', - types: [ - 'node', // NOTE: `node` is added because users scripts can also use pure node's packages as webpack or others - ], - }, - }); - - const tsConfigPaths = loadTsConfigPaths(); - const result = tsConfigPaths.loadConfig(tsConfig); - // The `loadConfig` returns a `ConfigLoaderResult` which must be guarded with - // the `resultType` check. - if (result.resultType === 'success') { - const { absoluteBaseUrl: baseUrl, paths } = result; - if (baseUrl && paths) { - tsConfigPaths.register({ baseUrl, paths }); - } - } - }; -})(); - -/** - * check for TS node registration - * @param file: file name or file directory are allowed - * @todo tsNodeRegistration: require ts-node if file extension is TypeScript - */ -function tsNodeRegister(file: string = '', tsConfig: string, logger: logging.LoggerApi) { - if (file?.endsWith('.ts')) { - // Register TS compiler lazily - _tsNodeRegister(tsConfig, logger); - } -} +import { registerTsProject } from './register-ts-project'; /** * This uses a dynamic import to load a module which may be ESM. @@ -72,22 +22,20 @@ function loadEsmModule(modulePath: string | URL): Promise { /** * Loads CJS and ESM modules based on extension */ -export async function loadModule( - modulePath: string, - tsConfig: string, - logger: logging.LoggerApi -): Promise { - tsNodeRegister(modulePath, tsConfig, logger); - +export async function loadModule(modulePath: string, tsConfig: string): Promise { switch (path.extname(modulePath)) { case '.mjs': // Load the ESM configuration file using the TypeScript dynamic import workaround. // Once TypeScript provides support for keeping the dynamic import this workaround can be // changed to a direct dynamic import. return (await loadEsmModule<{ default: T }>(url.pathToFileURL(modulePath))).default; + case '.cjs': return require(modulePath); + case '.ts': + const unregisterTsProject = registerTsProject(tsConfig); + try { // If it's a TS file then there are 2 cases for exporing an object. // The first one is `export blah`, transpiled into `module.exports = { blah} `. @@ -101,7 +49,10 @@ export async function loadModule( return (await loadEsmModule<{ default: T }>(url.pathToFileURL(modulePath))).default; } throw e; + } finally { + unregisterTsProject(); } + //.js default: // The file could be either CommonJS or ESM. @@ -120,19 +71,3 @@ export async function loadModule( } } } - -/** - * Loads `ts-node` lazily. Moved to a separate function to declare - * a return type, more readable than an inline variant. - */ -function loadTsNode(): typeof import('ts-node') { - return require('ts-node'); -} - -/** - * Loads `tsconfig-paths` lazily. Moved to a separate function to declare - * a return type, more readable than an inline variant. - */ -function loadTsConfigPaths(): typeof import('tsconfig-paths') { - return require('tsconfig-paths'); -} diff --git a/packages/common/src/register-ts-project.ts b/packages/common/src/register-ts-project.ts new file mode 100644 index 0000000000..c7f78f5fa8 --- /dev/null +++ b/packages/common/src/register-ts-project.ts @@ -0,0 +1,87 @@ +import * as path from 'node:path'; +import type { CompilerOptions } from 'typescript'; + +let ts: typeof import('typescript'); +let isTsEsmLoaderRegistered = false; + +export function registerTsProject(tsConfig: string) { + const cleanupFunctions = [registerTsConfigPaths(tsConfig), registerTsNodeService(tsConfig)]; + + // Add ESM support for `.ts` files. + // NOTE: There is no cleanup function for this, as it's not possible to unregister the loader. + // Based on limited testing, it doesn't seem to matter if we register it multiple times, but just in + // case let's keep a flag to prevent it. + if (!isTsEsmLoaderRegistered) { + const module = require('node:module'); + if (module.register && packageIsInstalled('ts-node/esm')) { + const url = require('node:url'); + module.register(url.pathToFileURL(require.resolve('ts-node/esm'))); + } + isTsEsmLoaderRegistered = true; + } + + return () => { + cleanupFunctions.forEach(fn => fn()); + }; +} + +function registerTsNodeService(tsConfig: string): VoidFunction { + const { register } = require('ts-node') as typeof import('ts-node'); + + const service = register({ + project: tsConfig, + compilerOptions: { + module: 'CommonJS', + types: [ + 'node', // NOTE: `node` is added because users scripts can also use pure node's packages as webpack or others + ], + }, + }); + + return () => { + service.enabled(false); + }; +} + +function registerTsConfigPaths(tsConfig: string): VoidFunction { + const tsConfigPaths = require('tsconfig-paths') as typeof import('tsconfig-paths'); + const result = tsConfigPaths.loadConfig(tsConfig); + if (result.resultType === 'success') { + const { absoluteBaseUrl: baseUrl, paths } = result; + if (baseUrl && paths) { + // Returns a function to undo paths registration. + return tsConfigPaths.register({ baseUrl, paths }); + } + } + + // We cannot return anything here if paths failed to be registered. + // Additionally, I don't think we should perform any logging in this + // context, considering that this is internal information not exposed + // to the end user + return () => {}; +} + +function packageIsInstalled(m: string): boolean { + try { + require.resolve(m); + return true; + } catch { + return false; + } +} + +function readCompilerOptions(tsConfig: string): CompilerOptions { + ts ??= require('typescript'); + + const jsonContent = ts.readConfigFile(tsConfig, ts.sys.readFile); + const { options } = ts.parseJsonConfigFileContent( + jsonContent.config, + ts.sys, + path.dirname(tsConfig) + ); + + // This property is returned in compiler options for some reason, but not part of the typings. + // ts-node fails on unknown props, so we have to remove it. + delete options.configFilePath; + return options; +} diff --git a/packages/custom-esbuild/src/application/index.ts b/packages/custom-esbuild/src/application/index.ts index 385b7abaca..c64f913cec 100644 --- a/packages/custom-esbuild/src/application/index.ts +++ b/packages/custom-esbuild/src/application/index.ts @@ -17,14 +17,10 @@ export function buildCustomEsbuildApplication( const tsConfig = path.join(workspaceRoot, options.tsConfig); return defer(async () => { - const codePlugins = await loadPlugins(options.plugins, workspaceRoot, tsConfig, context.logger); + const codePlugins = await loadPlugins(options.plugins, workspaceRoot, tsConfig); const indexHtmlTransformer = options.indexHtmlTransformer - ? await loadModule( - path.join(workspaceRoot, options.indexHtmlTransformer), - tsConfig, - context.logger - ) + ? await loadModule(path.join(workspaceRoot, options.indexHtmlTransformer), tsConfig) : undefined; return { codePlugins, indexHtmlTransformer } as ApplicationBuilderExtensions; diff --git a/packages/custom-esbuild/src/dev-server/index.ts b/packages/custom-esbuild/src/dev-server/index.ts index 74693a75e5..9b637c4789 100644 --- a/packages/custom-esbuild/src/dev-server/index.ts +++ b/packages/custom-esbuild/src/dev-server/index.ts @@ -42,27 +42,14 @@ export function executeCustomDevServerBuilder( const middleware = await Promise.all( (options.middlewares || []).map(middlewarePath => // https://github.com/angular/angular-cli/pull/26212/files#diff-a99020cbdb97d20b2bc686bcb64b31942107d56db06fd880171b0a86f7859e6eR52 - loadModule( - path.join(workspaceRoot, middlewarePath), - tsConfig, - context.logger - ) + loadModule(path.join(workspaceRoot, middlewarePath), tsConfig) ) ); - const buildPlugins = await loadPlugins( - buildOptions.plugins, - workspaceRoot, - tsConfig, - context.logger - ); + const buildPlugins = await loadPlugins(buildOptions.plugins, workspaceRoot, tsConfig); const indexHtmlTransformer: IndexHtmlTransform = buildOptions.indexHtmlTransformer - ? await loadModule( - path.join(workspaceRoot, buildOptions.indexHtmlTransformer), - tsConfig, - context.logger - ) + ? await loadModule(path.join(workspaceRoot, buildOptions.indexHtmlTransformer), tsConfig) : undefined; patchBuilderContext(context, buildTarget); diff --git a/packages/custom-esbuild/src/load-plugins.ts b/packages/custom-esbuild/src/load-plugins.ts index f0e154943b..db80564f7f 100644 --- a/packages/custom-esbuild/src/load-plugins.ts +++ b/packages/custom-esbuild/src/load-plugins.ts @@ -1,17 +1,15 @@ import * as path from 'node:path'; import type { Plugin } from 'esbuild'; -import type { logging } from '@angular-devkit/core'; import { loadModule } from '@angular-builders/common'; export async function loadPlugins( paths: string[] | undefined, workspaceRoot: string, - tsConfig: string, - logger: logging.LoggerApi + tsConfig: string ): Promise { const plugins = await Promise.all( (paths || []).map(pluginPath => - loadModule(path.join(workspaceRoot, pluginPath), tsConfig, logger) + loadModule(path.join(workspaceRoot, pluginPath), tsConfig) ) ); diff --git a/packages/custom-webpack/src/custom-webpack-builder.ts b/packages/custom-webpack/src/custom-webpack-builder.ts index cc1d54ea80..a1b7a945a9 100644 --- a/packages/custom-webpack/src/custom-webpack-builder.ts +++ b/packages/custom-webpack/src/custom-webpack-builder.ts @@ -47,8 +47,7 @@ export class CustomWebpackBuilder { ); const configOrFactoryOrPromise = await loadModule( webpackConfigPath, - tsConfig, - logger + tsConfig ); if (typeof configOrFactoryOrPromise === 'function') { diff --git a/packages/custom-webpack/src/transform-factories.spec.ts b/packages/custom-webpack/src/transform-factories.spec.ts index aa94b4ccec..b11c0b3f4c 100644 --- a/packages/custom-webpack/src/transform-factories.spec.ts +++ b/packages/custom-webpack/src/transform-factories.spec.ts @@ -1,8 +1,12 @@ jest.mock('ts-node', () => ({ - register: jest.fn(), + register: jest.fn().mockReturnValue({ + enabled: jest.fn(), + }), })); jest.mock('tsconfig-paths', () => ({ - loadConfig: jest.fn().mockReturnValue({}), + loadConfig: jest.fn().mockReturnValue({ + register: jest.fn().mockReturnValue(() => {}), + }), })); import { getTransforms } from './transform-factories'; @@ -32,7 +36,6 @@ describe('getTransforms', () => { transforms.webpackConfiguration({}); expect(tsNode.register).toHaveBeenCalledTimes(1); - expect(logger.warn).not.toHaveBeenCalled(); const transforms2 = getTransforms( { @@ -45,7 +48,5 @@ describe('getTransforms', () => { { workspaceRoot: './test', logger } as any ); transforms2.webpackConfiguration({}); - - expect(logger.warn).toHaveBeenCalledTimes(1); }); }); diff --git a/packages/custom-webpack/src/transform-factories.ts b/packages/custom-webpack/src/transform-factories.ts index bc4f3952a4..f503bd14a9 100644 --- a/packages/custom-webpack/src/transform-factories.ts +++ b/packages/custom-webpack/src/transform-factories.ts @@ -32,19 +32,14 @@ export const customWebpackConfigTransformFactory: ( export const indexHtmlTransformFactory: ( options: CustomWebpackSchema, context: BuilderContext -) => IndexHtmlTransform = (options, { workspaceRoot, target, logger }) => { +) => IndexHtmlTransform = (options, { workspaceRoot, target }) => { if (!options.indexTransform) return null; const transformPath = path.join(getSystemPath(normalize(workspaceRoot)), options.indexTransform); const tsConfig = path.join(getSystemPath(normalize(workspaceRoot)), options.tsConfig); return async (indexHtml: string) => { - const transform = await loadModule( - transformPath, - tsConfig, - logger - ); - + const transform = await loadModule(transformPath, tsConfig); return transform(target, indexHtml); }; }; diff --git a/packages/jest/src/custom-config.resolver.ts b/packages/jest/src/custom-config.resolver.ts index 47c8eb4375..38fb026883 100644 --- a/packages/jest/src/custom-config.resolver.ts +++ b/packages/jest/src/custom-config.resolver.ts @@ -33,7 +33,7 @@ export class CustomConfigResolver { return {}; } - return await loadModule(workspaceJestConfigPath, tsConfig, this.logger); + return await loadModule(workspaceJestConfigPath, tsConfig); } async resolveForProject(projectRoot: Path, configPath: string): Promise { @@ -45,6 +45,6 @@ export class CustomConfigResolver { return {}; } const tsConfig = getTsConfigPath(projectRoot, this.options); - return await loadModule(jestConfigPath, tsConfig, this.logger); + return await loadModule(jestConfigPath, tsConfig); } } diff --git a/yarn.lock b/yarn.lock index ea60688c6c..4a56b96833 100644 --- a/yarn.lock +++ b/yarn.lock @@ -34,7 +34,6 @@ __metadata: version: 0.0.0-use.local resolution: "@angular-builders/common@workspace:packages/common" dependencies: - "@angular-devkit/core": ^17.1.0 rimraf: ^5.0.0 ts-node: ^10.0.0 tsconfig-paths: ^4.1.0 @@ -14284,6 +14283,7 @@ __metadata: rxjs: 7.8.1 tslib: 2.6.2 typescript: 5.3.3 + webpack-esm-plugin: "file:./webpack-esm-plugin" zone.js: 0.14.3 languageName: unknown linkType: soft @@ -16560,6 +16560,13 @@ __metadata: languageName: node linkType: hard +"webpack-esm-plugin@file:./webpack-esm-plugin::locator=sanity-app-esm%40workspace%3Aexamples%2Fcustom-webpack%2Fsanity-app-esm": + version: 0.0.1 + resolution: "webpack-esm-plugin@file:./webpack-esm-plugin#./webpack-esm-plugin::hash=961e19&locator=sanity-app-esm%40workspace%3Aexamples%2Fcustom-webpack%2Fsanity-app-esm" + checksum: 665e5b4c7ab7bd776bafd8fab37c62ed8e4dac7292b874d45412947e974f371f63655af6766f96cc5df7a8d524bbd92f87b96cd06e6cc5d4e3db5cc49d3f3393 + languageName: node + linkType: hard + "webpack-merge@npm:5.10.0, webpack-merge@npm:^5.7.3": version: 5.10.0 resolution: "webpack-merge@npm:5.10.0"