From 73a1676042f26a85c2feaf5317f60e24e2480c31 Mon Sep 17 00:00:00 2001 From: ahnpnl Date: Tue, 3 Dec 2024 08:29:24 +0100 Subject: [PATCH] fix: revert commit ad7a297e to fix perf regression As reported, the perf issue is caused by the introduction of an extra `Program` instance to fetch to AST transformers next to the hidden `Program` behind `ts.transpileModule`. We should only use one instance of `Program` therefore we have to manually copy the codes of `ts.transpileModule` to use for our case. Fixes #2886 --- .../apps/app1/src/app/app.component.ts | 9 +- .../example-app-v17/src/app/app.component.ts | 9 +- .../example-app-v18/src/app/app.component.ts | 9 +- .../example-app-v19/src/app/app.component.ts | 9 +- .../angular-app/src/app/app.component.ts | 9 +- src/compiler/ng-jest-compiler.spec.ts | 3 +- src/compiler/ng-jest-compiler.ts | 156 +++++++++++++----- 7 files changed, 153 insertions(+), 51 deletions(-) diff --git a/examples/example-app-monorepo/apps/app1/src/app/app.component.ts b/examples/example-app-monorepo/apps/app1/src/app/app.component.ts index e49202d5d9..e3b0c55a47 100644 --- a/examples/example-app-monorepo/apps/app1/src/app/app.component.ts +++ b/examples/example-app-monorepo/apps/app1/src/app/app.component.ts @@ -1,4 +1,5 @@ -import { Component } from '@angular/core'; +import { DOCUMENT } from '@angular/common'; +import { Component, Inject } from '@angular/core'; import { RouterLink, RouterOutlet } from '@angular/router'; import { BannerComponent } from './banner/banner.component'; @@ -10,4 +11,8 @@ import { WelcomeComponent } from './welcome/welcome.component'; templateUrl: './app.component.html', imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink], }) -export class AppComponent {} +export class AppComponent { + constructor(@Inject(DOCUMENT) injectedDoc: Document) { + injectedDoc.title = 'Example App'; + } +} diff --git a/examples/example-app-v17/src/app/app.component.ts b/examples/example-app-v17/src/app/app.component.ts index e49202d5d9..e3b0c55a47 100644 --- a/examples/example-app-v17/src/app/app.component.ts +++ b/examples/example-app-v17/src/app/app.component.ts @@ -1,4 +1,5 @@ -import { Component } from '@angular/core'; +import { DOCUMENT } from '@angular/common'; +import { Component, Inject } from '@angular/core'; import { RouterLink, RouterOutlet } from '@angular/router'; import { BannerComponent } from './banner/banner.component'; @@ -10,4 +11,8 @@ import { WelcomeComponent } from './welcome/welcome.component'; templateUrl: './app.component.html', imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink], }) -export class AppComponent {} +export class AppComponent { + constructor(@Inject(DOCUMENT) injectedDoc: Document) { + injectedDoc.title = 'Example App'; + } +} diff --git a/examples/example-app-v18/src/app/app.component.ts b/examples/example-app-v18/src/app/app.component.ts index e49202d5d9..e3b0c55a47 100644 --- a/examples/example-app-v18/src/app/app.component.ts +++ b/examples/example-app-v18/src/app/app.component.ts @@ -1,4 +1,5 @@ -import { Component } from '@angular/core'; +import { DOCUMENT } from '@angular/common'; +import { Component, Inject } from '@angular/core'; import { RouterLink, RouterOutlet } from '@angular/router'; import { BannerComponent } from './banner/banner.component'; @@ -10,4 +11,8 @@ import { WelcomeComponent } from './welcome/welcome.component'; templateUrl: './app.component.html', imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink], }) -export class AppComponent {} +export class AppComponent { + constructor(@Inject(DOCUMENT) injectedDoc: Document) { + injectedDoc.title = 'Example App'; + } +} diff --git a/examples/example-app-v19/src/app/app.component.ts b/examples/example-app-v19/src/app/app.component.ts index 197caf7593..cec7c30f6a 100644 --- a/examples/example-app-v19/src/app/app.component.ts +++ b/examples/example-app-v19/src/app/app.component.ts @@ -1,4 +1,5 @@ -import { Component } from '@angular/core'; +import { DOCUMENT } from '@angular/common'; +import { Component, Inject } from '@angular/core'; import { RouterLink, RouterOutlet } from '@angular/router'; import { BannerComponent } from './banner/banner.component'; @@ -9,4 +10,8 @@ import { WelcomeComponent } from './welcome/welcome.component'; templateUrl: './app.component.html', imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink], }) -export class AppComponent {} +export class AppComponent { + constructor(@Inject(DOCUMENT) injectedDoc: Document) { + injectedDoc.title = 'Example App'; + } +} diff --git a/examples/example-app-yarn-workspace/packages/angular-app/src/app/app.component.ts b/examples/example-app-yarn-workspace/packages/angular-app/src/app/app.component.ts index e49202d5d9..e3b0c55a47 100644 --- a/examples/example-app-yarn-workspace/packages/angular-app/src/app/app.component.ts +++ b/examples/example-app-yarn-workspace/packages/angular-app/src/app/app.component.ts @@ -1,4 +1,5 @@ -import { Component } from '@angular/core'; +import { DOCUMENT } from '@angular/common'; +import { Component, Inject } from '@angular/core'; import { RouterLink, RouterOutlet } from '@angular/router'; import { BannerComponent } from './banner/banner.component'; @@ -10,4 +11,8 @@ import { WelcomeComponent } from './welcome/welcome.component'; templateUrl: './app.component.html', imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink], }) -export class AppComponent {} +export class AppComponent { + constructor(@Inject(DOCUMENT) injectedDoc: Document) { + injectedDoc.title = 'Example App'; + } +} diff --git a/src/compiler/ng-jest-compiler.spec.ts b/src/compiler/ng-jest-compiler.spec.ts index 07a7704665..ff6ba19115 100644 --- a/src/compiler/ng-jest-compiler.spec.ts +++ b/src/compiler/ng-jest-compiler.spec.ts @@ -323,9 +323,10 @@ describe('NgJestCompiler', () => { `); expect(diagnostics.length).toBe(0); + // The `type: undefined` is ok here which is covered by example apps' tests. expect(code).toContain(dedent` MyDir.ctorParameters = () => [ - { type: Document, decorators: [{ type: core_1.Inject, args: [core_1.DOCUMENT,] }] } + { type: undefined, decorators: [{ type: core_1.Inject, args: [core_1.DOCUMENT,] }] } ]; exports.MyDir = MyDir = tslib_1.__decorate([ (0, core_1.Directive)() diff --git a/src/compiler/ng-jest-compiler.ts b/src/compiler/ng-jest-compiler.ts index dbd111e2a9..1f9550763d 100644 --- a/src/compiler/ng-jest-compiler.ts +++ b/src/compiler/ng-jest-compiler.ts @@ -7,78 +7,154 @@ import type ts from 'typescript'; import { angularJitApplicationTransform } from '../transformers/jit_transform'; import { replaceResources } from '../transformers/replace-resources'; -export class NgJestCompiler extends TsCompiler { - private readonly _defaultLibDirPath: string; - private readonly _libSourceFileCache = new Map(); +const barebonesLibContent = `/// +interface Boolean {} +interface Function {} +interface CallableFunction {} +interface NewableFunction {} +interface IArguments {} +interface Number {} +interface Object {} +interface RegExp {} +interface String {} +interface Array { length: number; [n: number]: T; } +interface SymbolConstructor { + (desc?: string | number): symbol; + for(name: string): symbol; + readonly toStringTag: symbol; +} +declare var Symbol: SymbolConstructor; +interface Symbol { + readonly [Symbol.toStringTag]: string; +}`; +const barebonesLibName = 'lib.d.ts'; +let barebonesLibSourceFile: ts.SourceFile | undefined; +export class NgJestCompiler extends TsCompiler { constructor(readonly configSet: ConfigSet, readonly jestCacheFS: Map) { super(configSet, jestCacheFS); this._logger.debug('created NgJestCompiler'); - this._defaultLibDirPath = path.dirname(this._ts.getDefaultLibFilePath(this._compilerOptions)); } /** * Copy from https://github.com/microsoft/TypeScript/blob/master/src/services/transpile.ts * This is required because the exposed function `transpileModule` from TypeScript doesn't allow to access `Program` - * and we need `Program` to be able to use Angular AST transformers. + * and we need `Program` to be able to use Angular `replace-resources` transformer. */ protected _transpileOutput(fileContent: string, filePath: string): ts.TranspileOutput { - const scriptTarget = this._compilerOptions.target ?? this._ts.ScriptTarget.Latest; - const sourceFile = this._ts.createSourceFile(filePath, fileContent, scriptTarget); + barebonesLibSourceFile ??= this._ts.createSourceFile(barebonesLibName, barebonesLibContent, { + languageVersion: this._ts.ScriptTarget.Latest, + }); + const diagnostics: ts.Diagnostic[] = []; + const compilerOptions = { ...this._compilerOptions }; + const options: ts.CompilerOptions = compilerOptions + ? // @ts-expect-error internal TypeScript API + this._ts.fixupCompilerOptions(compilerOptions, diagnostics) + : {}; + + // mix in default options + const defaultOptions = this._ts.getDefaultCompilerOptions(); + for (const key in defaultOptions) { + if (Object.prototype.hasOwnProperty.call(defaultOptions, key) && options[key] === undefined) { + options[key] = defaultOptions[key]; + } + } + + // @ts-expect-error internal TypeScript API + for (const option of this._ts.transpileOptionValueCompilerOptions) { + options[option.name] = option.transpileOptionValue; + } + + /** + * transpileModule does not write anything to disk so there is no need to verify that there are no conflicts between + * input and output paths. + */ + options.suppressOutputPathCheck = true; + + // Filename can be non-ts file. + options.allowNonTsExtensions = true; + + // if jsx is specified then treat file as .tsx + const inputFileName = filePath || (compilerOptions && compilerOptions.jsx ? 'module.tsx' : 'module.ts'); + const sourceFile = this._ts.createSourceFile( + inputFileName, + fileContent, + options.target ?? this._ts.ScriptTarget.Latest, + ); + + let outputText: string | undefined; + let sourceMapText: string | undefined; + const compilerHost: ts.CompilerHost = { getSourceFile: (fileName) => { - if (fileName === path.normalize(filePath)) { - return sourceFile; - } - - let libSourceFile = this._libSourceFileCache.get(fileName); - if (!libSourceFile) { - const libFilePath = path.join(this._defaultLibDirPath, fileName); - const libFileContent = this._ts.sys.readFile(libFilePath) ?? ''; - if (libFileContent) { - libSourceFile = this._ts.createSourceFile(fileName, libFileContent, scriptTarget); - this._libSourceFileCache.set(fileName, libSourceFile); - } + return fileName === path.normalize(inputFileName) + ? sourceFile + : fileName === path.normalize(barebonesLibName) + ? barebonesLibSourceFile + : undefined; + }, + writeFile: (name, text) => { + if (path.extname(name) === '.map') { + sourceMapText = text; + } else { + outputText = text; } - - return libSourceFile; }, - // eslint-disable-next-line @typescript-eslint/no-empty-function - writeFile: () => {}, getDefaultLibFileName: () => 'lib.d.ts', useCaseSensitiveFileNames: () => false, getCanonicalFileName: (fileName) => fileName, getCurrentDirectory: () => '', getNewLine: () => os.EOL, - fileExists: (fileName) => fileName === filePath, + fileExists: (fileName) => fileName === inputFileName, readFile: () => '', directoryExists: () => true, getDirectories: () => [], }; - this.program = this._ts.createProgram([filePath], this._compilerOptions, compilerHost); - return this._ts.transpileModule(fileContent, { - fileName: filePath, - transformers: this._makeTransformers(this.configSet.resolvedTransformers), - compilerOptions: this._compilerOptions, - reportDiagnostics: this.configSet.shouldReportDiagnostics(filePath), - }); + this.program = this._ts.createProgram([inputFileName], options, compilerHost); + if (this.configSet.shouldReportDiagnostics(inputFileName)) { + diagnostics.push( + ...this.program.getSyntacticDiagnostics(sourceFile), + ...this.program.getOptionsDiagnostics(), + ); + } + this.program.emit( + undefined, + undefined, + undefined, + undefined, + this._makeTransformers(this.configSet.resolvedTransformers), + ); + if (outputText === undefined) { + diagnostics.push({ + file: sourceFile, + start: 0, + length: 0, + messageText: 'Output generation failed', + category: this._ts.DiagnosticCategory.Error, + code: 9999, + source: fileContent, + }); + } + + return { outputText: outputText ?? '', diagnostics, sourceMapText }; } protected _makeTransformers(customTransformers: TsJestAstTransformer): ts.CustomTransformers { - const allTransformers = super._makeTransformers(customTransformers); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const program = this.program!; return { - ...allTransformers.after, - ...allTransformers.afterDeclarations, + ...super._makeTransformers(customTransformers).after, + ...super._makeTransformers(customTransformers).afterDeclarations, before: [ - ...(allTransformers.before ?? []), - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - replaceResources(this.program!), - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - angularJitApplicationTransform(this.program!), - ], + ...customTransformers.before.map((beforeTransformer) => + beforeTransformer.factory(this, beforeTransformer.options), + ), + replaceResources(program), + angularJitApplicationTransform(program), + ] as Array | ts.CustomTransformerFactory>, }; } }