From aeebd14f04b8e520b0144a77e765da807a08dda0 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Fri, 19 Mar 2021 20:11:22 -0400 Subject: [PATCH] perf(@ngtools/webpack): only check affected files for Angular semantic diagnostics This change improves the performance of incremental type checking of Angular templates by reducing the number of calls to retrieve the diagnostics. Only the set of affected files will be queried on a rebuild. The affected set includes files TypeScript deems affected, files that are required to be emitted by the Angular compiler, and the original file for any TTC shim file that TypeScript deems affected. --- .../tests/behavior/rebuild-errors_spec.ts | 265 ++++++++++++++++++ .../test/hello-world-app/tsconfig.json | 3 +- packages/ngtools/webpack/src/ivy/cache.ts | 20 +- packages/ngtools/webpack/src/ivy/plugin.ts | 105 +++++-- 4 files changed, 360 insertions(+), 33 deletions(-) diff --git a/packages/angular_devkit/build_angular/src/browser/tests/behavior/rebuild-errors_spec.ts b/packages/angular_devkit/build_angular/src/browser/tests/behavior/rebuild-errors_spec.ts index 9e1311de2028..06d766cd23fa 100644 --- a/packages/angular_devkit/build_angular/src/browser/tests/behavior/rebuild-errors_spec.ts +++ b/packages/angular_devkit/build_angular/src/browser/tests/behavior/rebuild-errors_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 */ +// tslint:disable: no-big-function import { logging } from '@angular-devkit/core'; import { concatMap, count, take, timeout } from 'rxjs/operators'; import { buildWebpackBrowser } from '../../index'; @@ -12,6 +13,270 @@ import { BASE_OPTIONS, BROWSER_BUILDER_INFO, describeBuilder } from '../setup'; describeBuilder(buildWebpackBrowser, BROWSER_BUILDER_INFO, (harness) => { describe('Behavior: "Rebuild Error"', () => { + + it('detects template errors with no AOT codegen or TS emit differences', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + aot: true, + watch: true, + }); + + const goodDirectiveContents = ` + import { Directive, Input } from '@angular/core'; + @Directive({ selector: 'dir' }) + export class Dir { + @Input() foo: number; + } + `; + + const typeErrorText = `Type 'number' is not assignable to type 'string'.`; + + // Create a directive and add to application + await harness.writeFile('src/app/dir.ts', goodDirectiveContents); + await harness.writeFile('src/app/app.module.ts', ` + import { NgModule } from '@angular/core'; + import { BrowserModule } from '@angular/platform-browser'; + import { AppComponent } from './app.component'; + import { Dir } from './dir'; + @NgModule({ + declarations: [ + AppComponent, + Dir, + ], + imports: [ + BrowserModule + ], + providers: [], + bootstrap: [AppComponent] + }) + export class AppModule { } + `); + + // Create app component that uses the directive + await harness.writeFile('src/app/app.component.ts', ` + import { Component } from '@angular/core' + @Component({ + selector: 'app-root', + template: '', + }) + export class AppComponent { } + `); + + const buildCount = await harness + .execute({ outputLogsOnFailure: false }) + .pipe( + timeout(60000), + concatMap(async ({ result, logs }, index) => { + switch (index) { + case 0: + expect(result?.success).toBeTrue(); + + // Update directive to use a different input type for 'foo' (number -> string) + // Should cause a template error + await harness.writeFile('src/app/dir.ts', ` + import { Directive, Input } from '@angular/core'; + @Directive({ selector: 'dir' }) + export class Dir { + @Input() foo: string; + } + `); + + break; + case 1: + expect(result?.success).toBeFalse(); + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching(typeErrorText), + }), + ); + + // Make an unrelated change to verify error cache was updated + // Should persist error in the next rebuild + await harness.modifyFile('src/main.ts', (content) => content + '\n'); + + break; + case 2: + expect(result?.success).toBeFalse(); + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching(typeErrorText), + }), + ); + + // Revert the directive change that caused the error + // Should remove the error + await harness.writeFile('src/app/dir.ts', goodDirectiveContents); + + break; + case 3: + expect(result?.success).toBeTrue(); + expect(logs).not.toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching(typeErrorText), + }), + ); + + // Make an unrelated change to verify error cache was updated + // Should continue showing no error + await harness.modifyFile('src/main.ts', (content) => content + '\n'); + + break; + case 4: + expect(result?.success).toBeTrue(); + expect(logs).not.toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching(typeErrorText), + }), + ); + + break; + } + }), + take(5), + count(), + ) + .toPromise(); + + expect(buildCount).toBe(5); + }); + + it('detects template errors with AOT codegen differences', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + aot: true, + watch: true, + }); + + const typeErrorText = `Type 'number' is not assignable to type 'string'.`; + + // Create two directives and add to application + await harness.writeFile('src/app/dir.ts', ` + import { Directive, Input } from '@angular/core'; + @Directive({ selector: 'dir' }) + export class Dir { + @Input() foo: number; + } + `); + + // Same selector with a different type on the `foo` property but initially no `@Input` + const goodDirectiveContents = ` + import { Directive } from '@angular/core'; + @Directive({ selector: 'dir' }) + export class Dir2 { + foo: string; + } + `; + await harness.writeFile('src/app/dir2.ts', goodDirectiveContents); + + await harness.writeFile('src/app/app.module.ts', ` + import { NgModule } from '@angular/core'; + import { BrowserModule } from '@angular/platform-browser'; + import { AppComponent } from './app.component'; + import { Dir } from './dir'; + import { Dir2 } from './dir2'; + @NgModule({ + declarations: [ + AppComponent, + Dir, + Dir2, + ], + imports: [ + BrowserModule + ], + providers: [], + bootstrap: [AppComponent] + }) + export class AppModule { } + `); + + // Create app component that uses the directive + await harness.writeFile('src/app/app.component.ts', ` + import { Component } from '@angular/core' + @Component({ + selector: 'app-root', + template: '', + }) + export class AppComponent { } + `); + + const buildCount = await harness + .execute({ outputLogsOnFailure: false }) + .pipe( + timeout(60000), + concatMap(async ({ result, logs }, index) => { + switch (index) { + case 0: + expect(result?.success).toBeTrue(); + + // Update second directive to use string property `foo` as an Input + // Should cause a template error + await harness.writeFile('src/app/dir2.ts', ` + import { Directive, Input } from '@angular/core'; + @Directive({ selector: 'dir' }) + export class Dir2 { + @Input() foo: string; + } + `); + + break; + case 1: + expect(result?.success).toBeFalse(); + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching(typeErrorText), + }), + ); + + // Make an unrelated change to verify error cache was updated + // Should persist error in the next rebuild + await harness.modifyFile('src/main.ts', (content) => content + '\n'); + + break; + case 2: + expect(result?.success).toBeFalse(); + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching(typeErrorText), + }), + ); + + // Revert the directive change that caused the error + // Should remove the error + await harness.writeFile('src/app/dir2.ts', goodDirectiveContents); + + break; + case 3: + expect(result?.success).toBeTrue(); + expect(logs).not.toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching(typeErrorText), + }), + ); + + // Make an unrelated change to verify error cache was updated + // Should continue showing no error + await harness.modifyFile('src/main.ts', (content) => content + '\n'); + + break; + case 4: + expect(result?.success).toBeTrue(); + expect(logs).not.toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching(typeErrorText), + }), + ); + + break; + } + }), + take(5), + count(), + ) + .toPromise(); + + expect(buildCount).toBe(5); + }); + it('recovers from component stylesheet error', async () => { harness.useTarget('build', { ...BASE_OPTIONS, diff --git a/packages/angular_devkit/build_angular/test/hello-world-app/tsconfig.json b/packages/angular_devkit/build_angular/test/hello-world-app/tsconfig.json index 839aa2a548d7..956e76c7636f 100644 --- a/packages/angular_devkit/build_angular/test/hello-world-app/tsconfig.json +++ b/packages/angular_devkit/build_angular/test/hello-world-app/tsconfig.json @@ -20,6 +20,7 @@ }, "angularCompilerOptions": { "enableIvy": true, - "disableTypeScriptVersionCheck": true + "disableTypeScriptVersionCheck": true, + "strictTemplates": true } } diff --git a/packages/ngtools/webpack/src/ivy/cache.ts b/packages/ngtools/webpack/src/ivy/cache.ts index ceccd1389aec..1b3856b238c0 100644 --- a/packages/ngtools/webpack/src/ivy/cache.ts +++ b/packages/ngtools/webpack/src/ivy/cache.ts @@ -9,6 +9,8 @@ import * as ts from 'typescript'; import { normalizePath } from './paths'; export class SourceFileCache extends Map { + private readonly angularDiagnostics = new Map(); + invalidate( fileTimestamps: Map, buildTimestamp: number, @@ -29,11 +31,27 @@ export class SourceFileCache extends Map { if (!time || time >= buildTimestamp) { // Cache stores paths using the POSIX directory separator const normalizedFile = normalizePath(file); - this.delete(normalizedFile); + const sourceFile = this.get(normalizedFile); + if (sourceFile) { + this.delete(normalizedFile); + this.angularDiagnostics.delete(sourceFile); + } changedFiles.add(normalizedFile); } } return changedFiles; } + + updateAngularDiagnostics(sourceFile: ts.SourceFile, diagnostics: ts.Diagnostic[]): void { + if (diagnostics.length > 0) { + this.angularDiagnostics.set(sourceFile, diagnostics); + } else { + this.angularDiagnostics.delete(sourceFile); + } + } + + getAngularDiagnostics(sourceFile: ts.SourceFile): ts.Diagnostic[] | undefined { + return this.angularDiagnostics.get(sourceFile); + } } diff --git a/packages/ngtools/webpack/src/ivy/plugin.ts b/packages/ngtools/webpack/src/ivy/plugin.ts index c22db38f0c2c..5144a69a1084 100644 --- a/packages/ngtools/webpack/src/ivy/plugin.ts +++ b/packages/ngtools/webpack/src/ivy/plugin.ts @@ -9,12 +9,7 @@ import { CompilerHost, CompilerOptions, readConfiguration } from '@angular/compi import { NgtscProgram } from '@angular/compiler-cli/src/ngtsc/program'; import { createHash } from 'crypto'; import * as ts from 'typescript'; -import { - Compiler, - NormalModuleReplacementPlugin, - WebpackFourCompiler, - compilation, -} from 'webpack'; +import { Compiler, NormalModuleReplacementPlugin, WebpackFourCompiler, compilation } from 'webpack'; import { NgccProcessor } from '../ngcc_processor'; import { TypeScriptPathsPlugin } from '../paths-plugin'; import { WebpackResourceLoader } from '../resource_loader'; @@ -36,6 +31,13 @@ import { AngularPluginSymbol, EmitFileResult, FileEmitter } from './symbol'; import { InputFileSystemSync, createWebpackSystem } from './system'; import { createAotTransformers, createJitTransformers, mergeTransformers } from './transformation'; +/** + * The threshold used to determine whether Angular file diagnostics should optimize for full programs + * or single files. If the number of affected files for a build is more than the threshold, full + * program optimization will be used. + */ +const DIAGNOSTICS_AFFECTED_THRESHOLD = 1; + export interface AngularPluginOptions { tsconfig: string; compilerOptions?: CompilerOptions; @@ -296,10 +298,7 @@ export class AngularWebpackPlugin { }); } - private markResourceUsed( - normalizedResourcePath: string, - currentUnused: Set, - ): void { + private markResourceUsed(normalizedResourcePath: string, currentUnused: Set): void { if (!currentUnused.has(normalizedResourcePath)) { return; } @@ -422,13 +421,37 @@ export class AngularWebpackPlugin { } // Update semantic diagnostics cache + const affectedFiles = new Set(); while (true) { - const result = builder.getSemanticDiagnosticsOfNextAffectedFile(undefined, (sourceFile) => - ignoreForDiagnostics.has(sourceFile), - ); + const result = builder.getSemanticDiagnosticsOfNextAffectedFile(undefined, (sourceFile) => { + // If the affected file is a TTC shim, add the shim's original source file. + // This ensures that changes that affect TTC are typechecked even when the changes + // are otherwise unrelated from a TS perspective and do not result in Ivy codegen changes. + // For example, changing @Input property types of a directive used in another component's + // template. + if ( + ignoreForDiagnostics.has(sourceFile) && + sourceFile.fileName.endsWith('.ngtypecheck.ts') + ) { + // This file name conversion relies on internal compiler logic and should be converted + // to an official method when available. 15 is length of `.ngtypecheck.ts` + const originalFilename = sourceFile.fileName.slice(0, -15) + '.ts'; + const originalSourceFile = builder.getSourceFile(originalFilename); + if (originalSourceFile) { + affectedFiles.add(originalSourceFile); + } + + return true; + } + + return false; + }); + if (!result) { break; } + + affectedFiles.add(result.affected as ts.SourceFile); } // Collect non-semantic diagnostics @@ -468,35 +491,55 @@ export class AngularWebpackPlugin { this.requiredFilesToEmit.clear(); for (const sourceFile of builder.getSourceFiles()) { - // Collect Angular template diagnostics - if (!ignoreForDiagnostics.has(sourceFile)) { - // The below check should be removed once support for compiler 11.0 is dropped. - // Also, the below require should be changed to an ES6 import. - if (angularCompiler.getDiagnosticsForFile) { - // @angular/compiler-cli 11.1+ - const { OptimizeFor } = require('@angular/compiler-cli/src/ngtsc/typecheck/api'); - diagnosticsReporter( - angularCompiler.getDiagnosticsForFile(sourceFile, OptimizeFor.WholeProgram), - ); - } else { - // @angular/compiler-cli 11.0+ - const getDiagnostics = angularCompiler.getDiagnostics as ( - sourceFile: ts.SourceFile, - ) => ts.Diagnostic[]; - diagnosticsReporter(getDiagnostics.call(angularCompiler, sourceFile)); - } + if (sourceFile.isDeclarationFile) { + continue; } // Collect sources that are required to be emitted if ( - !sourceFile.isDeclarationFile && !ignoreForEmit.has(sourceFile) && !angularCompiler.incrementalDriver.safeToSkipEmit(sourceFile) ) { this.requiredFilesToEmit.add(normalizePath(sourceFile.fileName)); + + // If required to emit, diagnostics may have also changed + if (!ignoreForDiagnostics.has(sourceFile)) { + affectedFiles.add(sourceFile); + } + } else if ( + this.sourceFileCache && + !affectedFiles.has(sourceFile) && + !ignoreForDiagnostics.has(sourceFile) + ) { + // Use cached Angular diagnostics for unchanged and unaffected files + const angularDiagnostics = this.sourceFileCache.getAngularDiagnostics(sourceFile); + if (angularDiagnostics) { + diagnosticsReporter(angularDiagnostics); + } } } + // Collect new Angular diagnostics for files affected by changes + const { OptimizeFor } = require('@angular/compiler-cli/src/ngtsc/typecheck/api'); + const optimizeDiagnosticsFor = + affectedFiles.size <= DIAGNOSTICS_AFFECTED_THRESHOLD + ? OptimizeFor.SingleFile + : OptimizeFor.WholeProgram; + for (const affectedFile of affectedFiles) { + const angularDiagnostics = angularCompiler.getDiagnosticsForFile( + affectedFile, + optimizeDiagnosticsFor, + ); + diagnosticsReporter(angularDiagnostics); + this.sourceFileCache?.updateAngularDiagnostics(affectedFile, angularDiagnostics); + } + + // NOTE: Workaround to fix stale reuse program. Can be removed once fixed upstream. + // tslint:disable-next-line: no-any + (angularProgram as any).reuseTsProgram = + // tslint:disable-next-line: no-any + angularCompiler?.getNextProgram() || (angularCompiler as any)?.getCurrentProgram(); + return this.createFileEmitter( builder, mergeTransformers(angularCompiler.prepareEmit().transformers, transformers),