From 06dacb015ed300fb5723c8d2b27a440571f9c50d Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Tue, 17 Oct 2023 17:38:24 -0400 Subject: [PATCH] fix(@angular-devkit/build-angular): ensure recalculation of component diagnostics when template changes Previously, the application builder relied on TypeScript to detect affected component files via changes to the internal template type check code. However, not all AOT compiler generated template errors will cause the template type check code to be changed. Block syntax errors are one example. To ensure that component diagnostics are recalculated when required, the component file will now always be considered affected when a used template file is changed. --- .../angular_devkit/build_angular/BUILD.bazel | 4 +- .../tests/behavior/rebuild-errors_spec.ts | 317 ++++++++++++++++++ .../angular/compilation/aot-compilation.ts | 2 + 3 files changed, 322 insertions(+), 1 deletion(-) create mode 100644 packages/angular_devkit/build_angular/src/builders/application/tests/behavior/rebuild-errors_spec.ts diff --git a/packages/angular_devkit/build_angular/BUILD.bazel b/packages/angular_devkit/build_angular/BUILD.bazel index 70c753060de5..e4bf59477b95 100644 --- a/packages/angular_devkit/build_angular/BUILD.bazel +++ b/packages/angular_devkit/build_angular/BUILD.bazel @@ -310,7 +310,9 @@ ts_library( LARGE_SPECS = { "application": { - "shards": 10, + "shards": 16, + "size": "large", + "flaky": True, "extra_deps": [ "@npm//buffer", ], diff --git a/packages/angular_devkit/build_angular/src/builders/application/tests/behavior/rebuild-errors_spec.ts b/packages/angular_devkit/build_angular/src/builders/application/tests/behavior/rebuild-errors_spec.ts new file mode 100644 index 000000000000..7dcac124afb0 --- /dev/null +++ b/packages/angular_devkit/build_angular/src/builders/application/tests/behavior/rebuild-errors_spec.ts @@ -0,0 +1,317 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * 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 + */ + +import { logging } from '@angular-devkit/core'; +import { concatMap, count, timeout } from 'rxjs'; +import { buildApplication } from '../../index'; +import { APPLICATION_BUILDER_INFO, BASE_OPTIONS, describeBuilder } from '../setup'; + +/** + * Maximum time in milliseconds for single build/rebuild + * This accounts for CI variability. + */ +export const BUILD_TIMEOUT = 30_000; + +describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => { + describe('Behavior: "Rebuild Error Detection"', () => { + it('detects template errors with no AOT codegen or TS emit differences', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + 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 builderAbort = new AbortController(); + const buildCount = await harness + .execute({ outputLogsOnFailure: false, signal: builderAbort.signal }) + .pipe( + timeout(BUILD_TIMEOUT), + 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), + }), + ); + + // Test complete - abort watch mode + builderAbort?.abort(); + break; + } + }), + count(), + ) + .toPromise(); + + expect(buildCount).toBe(5); + }); + + it('detects cumulative block syntax errors', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + watch: true, + }); + + const builderAbort = new AbortController(); + const buildCount = await harness + .execute({ outputLogsOnFailure: false, signal: builderAbort.signal }) + .pipe( + timeout(BUILD_TIMEOUT), + concatMap(async ({ logs }, index) => { + switch (index) { + case 0: + // Add invalid block syntax + await harness.appendToFile('src/app/app.component.html', '@one'); + + break; + case 1: + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@one'), + }), + ); + + // 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(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@one'), + }), + ); + + // Add more invalid block syntax + await harness.appendToFile('src/app/app.component.html', '@two'); + + break; + case 3: + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@one'), + }), + ); + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@two'), + }), + ); + + // Add more invalid block syntax + await harness.appendToFile('src/app/app.component.html', '@three'); + + break; + case 4: + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@one'), + }), + ); + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@two'), + }), + ); + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@three'), + }), + ); + + // Revert the changes that caused the error + // Should remove the error + await harness.writeFile('src/app/app.component.html', '

GOOD

'); + + break; + case 5: + expect(logs).not.toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@one'), + }), + ); + expect(logs).not.toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@two'), + }), + ); + expect(logs).not.toContain( + jasmine.objectContaining({ + message: jasmine.stringContaining('@three'), + }), + ); + + // Test complete - abort watch mode + builderAbort?.abort(); + break; + } + }), + count(), + ) + .toPromise(); + + expect(buildCount).toBe(6); + }); + + it('recovers from component stylesheet error', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + watch: true, + aot: false, + }); + + const builderAbort = new AbortController(); + const buildCount = await harness + .execute({ outputLogsOnFailure: false, signal: builderAbort.signal }) + .pipe( + timeout(BUILD_TIMEOUT), + concatMap(async ({ result, logs }, index) => { + switch (index) { + case 0: + await harness.writeFile('src/app/app.component.css', 'invalid-css-content'); + + break; + case 1: + expect(logs).toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching('invalid-css-content'), + }), + ); + + await harness.writeFile('src/app/app.component.css', 'p { color: green }'); + + break; + case 2: + expect(logs).not.toContain( + jasmine.objectContaining({ + message: jasmine.stringMatching('invalid-css-content'), + }), + ); + + harness + .expectFile('dist/browser/main.js') + .content.toContain('p {\\n color: green;\\n}'); + + // Test complete - abort watch mode + builderAbort?.abort(); + break; + } + }), + count(), + ) + .toPromise(); + + expect(buildCount).toBe(3); + }); + }); +}); diff --git a/packages/angular_devkit/build_angular/src/tools/esbuild/angular/compilation/aot-compilation.ts b/packages/angular_devkit/build_angular/src/tools/esbuild/angular/compilation/aot-compilation.ts index 8df67c906e59..8e896db3faf6 100644 --- a/packages/angular_devkit/build_angular/src/tools/esbuild/angular/compilation/aot-compilation.ts +++ b/packages/angular_devkit/build_angular/src/tools/esbuild/angular/compilation/aot-compilation.ts @@ -105,6 +105,8 @@ export class AotCompilation extends AngularCompilation { for (const resourceDependency of resourceDependencies) { if (hostOptions.modifiedFiles.has(resourceDependency)) { this.#state.diagnosticCache.delete(sourceFile); + // Also mark as affected in case changed template affects diagnostics + affectedFiles.add(sourceFile); } } }