Skip to content

Commit

Permalink
fix: revert commit ad7a297 to fix perf regression
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ahnpnl committed Dec 3, 2024
1 parent bd37f7f commit 73a1676
Show file tree
Hide file tree
Showing 7 changed files with 153 additions and 51 deletions.
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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';
}
}
9 changes: 7 additions & 2 deletions examples/example-app-v17/src/app/app.component.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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';
}
}
9 changes: 7 additions & 2 deletions examples/example-app-v18/src/app/app.component.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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';
}
}
9 changes: 7 additions & 2 deletions examples/example-app-v19/src/app/app.component.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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';
}
}
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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';
}
}
3 changes: 2 additions & 1 deletion src/compiler/ng-jest-compiler.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)()
Expand Down
156 changes: 116 additions & 40 deletions src/compiler/ng-jest-compiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, ts.SourceFile>();
const barebonesLibContent = `/// <reference no-default-lib="true"/>
interface Boolean {}
interface Function {}
interface CallableFunction {}
interface NewableFunction {}
interface IArguments {}
interface Number {}
interface Object {}
interface RegExp {}
interface String {}
interface Array<T> { 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<string, string>) {
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.TransformerFactory<ts.SourceFile> | ts.CustomTransformerFactory>,
};
}
}

0 comments on commit 73a1676

Please sign in to comment.