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 9e1311de20..06d766cd23 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: '<dir [foo]="123">', + }) + 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<logging.LogEntry>({ + 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<logging.LogEntry>({ + 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<logging.LogEntry>({ + 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<logging.LogEntry>({ + 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: '<dir [foo]="123">', + }) + 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<logging.LogEntry>({ + 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<logging.LogEntry>({ + 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<logging.LogEntry>({ + 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<logging.LogEntry>({ + 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 839aa2a548..956e76c763 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 ceccd1389a..1b3856b238 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<string, ts.SourceFile> { + private readonly angularDiagnostics = new Map<ts.SourceFile, ts.Diagnostic[]>(); + invalidate( fileTimestamps: Map<string, 'ignore' | number | { safeTime: number } | null>, buildTimestamp: number, @@ -29,11 +31,27 @@ export class SourceFileCache extends Map<string, ts.SourceFile> { 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 c22db38f0c..5144a69a10 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<string>, - ): void { + private markResourceUsed(normalizedResourcePath: string, currentUnused: Set<string>): void { if (!currentUnused.has(normalizedResourcePath)) { return; } @@ -422,13 +421,37 @@ export class AngularWebpackPlugin { } // Update semantic diagnostics cache + const affectedFiles = new Set<ts.SourceFile>(); 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),