Skip to content

Commit

Permalink
perf(@ngtools/webpack): only check affected files for Angular semanti…
Browse files Browse the repository at this point in the history 
…c 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.

(cherry picked from commit aeebd14)
  • Loading branch information
@clydin @alan-agius4
clydin authored and alan-agius4 committed Apr 19, 2021
1 parent 4c927de commit 6f44b20
Show file tree
Hide file tree
Showing 4 changed files with 357 additions and 27 deletions.
265 changes: 265 additions & 0 deletions packages/angular_devkit/build_angular/src/browser/tests/behavior/rebuild-errors_spec.ts
View file
Expand Up @@ -5,13 +5,278 @@
* 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';
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,
Expand Down
3 changes: 2 additions & 1 deletion packages/angular_devkit/build_angular/test/hello-world-app/tsconfig.json
View file
Expand Up @@ -20,6 +20,7 @@
},
"angularCompilerOptions": {
"enableIvy": true,
"disableTypeScriptVersionCheck": true
"disableTypeScriptVersionCheck": true,
"strictTemplates": true
}
}
20 changes: 19 additions & 1 deletion packages/ngtools/webpack/src/ivy/cache.ts
View file
Expand Up @@ -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, number | { timestamp: number } | null>,
buildTimestamp: number,
Expand All @@ -20,11 +22,27 @@ export class SourceFileCache extends Map<string, ts.SourceFile> {
if (time === null || buildTimestamp < time) {
// 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);
}
}

0 comments on commit 6f44b20

Please sign in to comment.