Skip to content

Commit

Permalink
fix(@angular-devkit/build-angular): ensure recalculation of component…
Browse files Browse the repository at this point in the history
… 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.

(cherry picked from commit 06dacb0)
  • Loading branch information
clydin authored and alan-agius4 committed Oct 18, 2023
1 parent 58bd397 commit 3e5a99c
Show file tree
Hide file tree
Showing 3 changed files with 322 additions and 1 deletion.
4 changes: 3 additions & 1 deletion packages/angular_devkit/build_angular/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,9 @@ ts_library(

LARGE_SPECS = {
"application": {
"shards": 10,
"shards": 16,
"size": "large",
"flaky": True,
"extra_deps": [
"@npm//buffer",
],
Expand Down
Original file line number Diff line number Diff line change
@@ -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: '<dir [foo]="123">',
})
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<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),
}),
);

// 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<logging.LogEntry>({
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<logging.LogEntry>({
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<logging.LogEntry>({
message: jasmine.stringContaining('@one'),
}),
);
expect(logs).toContain(
jasmine.objectContaining<logging.LogEntry>({
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<logging.LogEntry>({
message: jasmine.stringContaining('@one'),
}),
);
expect(logs).toContain(
jasmine.objectContaining<logging.LogEntry>({
message: jasmine.stringContaining('@two'),
}),
);
expect(logs).toContain(
jasmine.objectContaining<logging.LogEntry>({
message: jasmine.stringContaining('@three'),
}),
);

// Revert the changes that caused the error
// Should remove the error
await harness.writeFile('src/app/app.component.html', '<p>GOOD</p>');

break;
case 5:
expect(logs).not.toContain(
jasmine.objectContaining<logging.LogEntry>({
message: jasmine.stringContaining('@one'),
}),
);
expect(logs).not.toContain(
jasmine.objectContaining<logging.LogEntry>({
message: jasmine.stringContaining('@two'),
}),
);
expect(logs).not.toContain(
jasmine.objectContaining<logging.LogEntry>({
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<logging.LogEntry>({
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<logging.LogEntry>({
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);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down

0 comments on commit 3e5a99c

Please sign in to comment.