Skip to content

Commit aeebd14

Browse files
clydinfilipesilva
clydin
authored and
filipesilva
committed
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.
1 parent 81fcfb7 commit aeebd14

File tree

4 files changed

+360
-33
lines changed

4 files changed

+360
-33
lines changed

‎packages/angular_devkit/build_angular/src/browser/tests/behavior/rebuild-errors_spec.ts

+265
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,278 @@
55
* Use of this source code is governed by an MIT-style license that can be
66
* found in the LICENSE file at https://angular.io/license
77
*/
8+
// tslint:disable: no-big-function
89
import { logging } from '@angular-devkit/core';
910
import { concatMap, count, take, timeout } from 'rxjs/operators';
1011
import { buildWebpackBrowser } from '../../index';
1112
import { BASE_OPTIONS, BROWSER_BUILDER_INFO, describeBuilder } from '../setup';
1213

1314
describeBuilder(buildWebpackBrowser, BROWSER_BUILDER_INFO, (harness) => {
1415
describe('Behavior: "Rebuild Error"', () => {
16+
17+
it('detects template errors with no AOT codegen or TS emit differences', async () => {
18+
harness.useTarget('build', {
19+
...BASE_OPTIONS,
20+
aot: true,
21+
watch: true,
22+
});
23+
24+
const goodDirectiveContents = `
25+
import { Directive, Input } from '@angular/core';
26+
@Directive({ selector: 'dir' })
27+
export class Dir {
28+
@Input() foo: number;
29+
}
30+
`;
31+
32+
const typeErrorText = `Type 'number' is not assignable to type 'string'.`;
33+
34+
// Create a directive and add to application
35+
await harness.writeFile('src/app/dir.ts', goodDirectiveContents);
36+
await harness.writeFile('src/app/app.module.ts', `
37+
import { NgModule } from '@angular/core';
38+
import { BrowserModule } from '@angular/platform-browser';
39+
import { AppComponent } from './app.component';
40+
import { Dir } from './dir';
41+
@NgModule({
42+
declarations: [
43+
AppComponent,
44+
Dir,
45+
],
46+
imports: [
47+
BrowserModule
48+
],
49+
providers: [],
50+
bootstrap: [AppComponent]
51+
})
52+
export class AppModule { }
53+
`);
54+
55+
// Create app component that uses the directive
56+
await harness.writeFile('src/app/app.component.ts', `
57+
import { Component } from '@angular/core'
58+
@Component({
59+
selector: 'app-root',
60+
template: '<dir [foo]="123">',
61+
})
62+
export class AppComponent { }
63+
`);
64+
65+
const buildCount = await harness
66+
.execute({ outputLogsOnFailure: false })
67+
.pipe(
68+
timeout(60000),
69+
concatMap(async ({ result, logs }, index) => {
70+
switch (index) {
71+
case 0:
72+
expect(result?.success).toBeTrue();
73+
74+
// Update directive to use a different input type for 'foo' (number -> string)
75+
// Should cause a template error
76+
await harness.writeFile('src/app/dir.ts', `
77+
import { Directive, Input } from '@angular/core';
78+
@Directive({ selector: 'dir' })
79+
export class Dir {
80+
@Input() foo: string;
81+
}
82+
`);
83+
84+
break;
85+
case 1:
86+
expect(result?.success).toBeFalse();
87+
expect(logs).toContain(
88+
jasmine.objectContaining<logging.LogEntry>({
89+
message: jasmine.stringMatching(typeErrorText),
90+
}),
91+
);
92+
93+
// Make an unrelated change to verify error cache was updated
94+
// Should persist error in the next rebuild
95+
await harness.modifyFile('src/main.ts', (content) => content + '\n');
96+
97+
break;
98+
case 2:
99+
expect(result?.success).toBeFalse();
100+
expect(logs).toContain(
101+
jasmine.objectContaining<logging.LogEntry>({
102+
message: jasmine.stringMatching(typeErrorText),
103+
}),
104+
);
105+
106+
// Revert the directive change that caused the error
107+
// Should remove the error
108+
await harness.writeFile('src/app/dir.ts', goodDirectiveContents);
109+
110+
break;
111+
case 3:
112+
expect(result?.success).toBeTrue();
113+
expect(logs).not.toContain(
114+
jasmine.objectContaining<logging.LogEntry>({
115+
message: jasmine.stringMatching(typeErrorText),
116+
}),
117+
);
118+
119+
// Make an unrelated change to verify error cache was updated
120+
// Should continue showing no error
121+
await harness.modifyFile('src/main.ts', (content) => content + '\n');
122+
123+
break;
124+
case 4:
125+
expect(result?.success).toBeTrue();
126+
expect(logs).not.toContain(
127+
jasmine.objectContaining<logging.LogEntry>({
128+
message: jasmine.stringMatching(typeErrorText),
129+
}),
130+
);
131+
132+
break;
133+
}
134+
}),
135+
take(5),
136+
count(),
137+
)
138+
.toPromise();
139+
140+
expect(buildCount).toBe(5);
141+
});
142+
143+
it('detects template errors with AOT codegen differences', async () => {
144+
harness.useTarget('build', {
145+
...BASE_OPTIONS,
146+
aot: true,
147+
watch: true,
148+
});
149+
150+
const typeErrorText = `Type 'number' is not assignable to type 'string'.`;
151+
152+
// Create two directives and add to application
153+
await harness.writeFile('src/app/dir.ts', `
154+
import { Directive, Input } from '@angular/core';
155+
@Directive({ selector: 'dir' })
156+
export class Dir {
157+
@Input() foo: number;
158+
}
159+
`);
160+
161+
// Same selector with a different type on the `foo` property but initially no `@Input`
162+
const goodDirectiveContents = `
163+
import { Directive } from '@angular/core';
164+
@Directive({ selector: 'dir' })
165+
export class Dir2 {
166+
foo: string;
167+
}
168+
`;
169+
await harness.writeFile('src/app/dir2.ts', goodDirectiveContents);
170+
171+
await harness.writeFile('src/app/app.module.ts', `
172+
import { NgModule } from '@angular/core';
173+
import { BrowserModule } from '@angular/platform-browser';
174+
import { AppComponent } from './app.component';
175+
import { Dir } from './dir';
176+
import { Dir2 } from './dir2';
177+
@NgModule({
178+
declarations: [
179+
AppComponent,
180+
Dir,
181+
Dir2,
182+
],
183+
imports: [
184+
BrowserModule
185+
],
186+
providers: [],
187+
bootstrap: [AppComponent]
188+
})
189+
export class AppModule { }
190+
`);
191+
192+
// Create app component that uses the directive
193+
await harness.writeFile('src/app/app.component.ts', `
194+
import { Component } from '@angular/core'
195+
@Component({
196+
selector: 'app-root',
197+
template: '<dir [foo]="123">',
198+
})
199+
export class AppComponent { }
200+
`);
201+
202+
const buildCount = await harness
203+
.execute({ outputLogsOnFailure: false })
204+
.pipe(
205+
timeout(60000),
206+
concatMap(async ({ result, logs }, index) => {
207+
switch (index) {
208+
case 0:
209+
expect(result?.success).toBeTrue();
210+
211+
// Update second directive to use string property `foo` as an Input
212+
// Should cause a template error
213+
await harness.writeFile('src/app/dir2.ts', `
214+
import { Directive, Input } from '@angular/core';
215+
@Directive({ selector: 'dir' })
216+
export class Dir2 {
217+
@Input() foo: string;
218+
}
219+
`);
220+
221+
break;
222+
case 1:
223+
expect(result?.success).toBeFalse();
224+
expect(logs).toContain(
225+
jasmine.objectContaining<logging.LogEntry>({
226+
message: jasmine.stringMatching(typeErrorText),
227+
}),
228+
);
229+
230+
// Make an unrelated change to verify error cache was updated
231+
// Should persist error in the next rebuild
232+
await harness.modifyFile('src/main.ts', (content) => content + '\n');
233+
234+
break;
235+
case 2:
236+
expect(result?.success).toBeFalse();
237+
expect(logs).toContain(
238+
jasmine.objectContaining<logging.LogEntry>({
239+
message: jasmine.stringMatching(typeErrorText),
240+
}),
241+
);
242+
243+
// Revert the directive change that caused the error
244+
// Should remove the error
245+
await harness.writeFile('src/app/dir2.ts', goodDirectiveContents);
246+
247+
break;
248+
case 3:
249+
expect(result?.success).toBeTrue();
250+
expect(logs).not.toContain(
251+
jasmine.objectContaining<logging.LogEntry>({
252+
message: jasmine.stringMatching(typeErrorText),
253+
}),
254+
);
255+
256+
// Make an unrelated change to verify error cache was updated
257+
// Should continue showing no error
258+
await harness.modifyFile('src/main.ts', (content) => content + '\n');
259+
260+
break;
261+
case 4:
262+
expect(result?.success).toBeTrue();
263+
expect(logs).not.toContain(
264+
jasmine.objectContaining<logging.LogEntry>({
265+
message: jasmine.stringMatching(typeErrorText),
266+
}),
267+
);
268+
269+
break;
270+
}
271+
}),
272+
take(5),
273+
count(),
274+
)
275+
.toPromise();
276+
277+
expect(buildCount).toBe(5);
278+
});
279+
15280
it('recovers from component stylesheet error', async () => {
16281
harness.useTarget('build', {
17282
...BASE_OPTIONS,

‎packages/angular_devkit/build_angular/test/hello-world-app/tsconfig.json

+2-1
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
},
2121
"angularCompilerOptions": {
2222
"enableIvy": true,
23-
"disableTypeScriptVersionCheck": true
23+
"disableTypeScriptVersionCheck": true,
24+
"strictTemplates": true
2425
}
2526
}

‎packages/ngtools/webpack/src/ivy/cache.ts

+19-1
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import * as ts from 'typescript';
99
import { normalizePath } from './paths';
1010

1111
export class SourceFileCache extends Map<string, ts.SourceFile> {
12+
private readonly angularDiagnostics = new Map<ts.SourceFile, ts.Diagnostic[]>();
13+
1214
invalidate(
1315
fileTimestamps: Map<string, 'ignore' | number | { safeTime: number } | null>,
1416
buildTimestamp: number,
@@ -29,11 +31,27 @@ export class SourceFileCache extends Map<string, ts.SourceFile> {
2931
if (!time || time >= buildTimestamp) {
3032
// Cache stores paths using the POSIX directory separator
3133
const normalizedFile = normalizePath(file);
32-
this.delete(normalizedFile);
34+
const sourceFile = this.get(normalizedFile);
35+
if (sourceFile) {
36+
this.delete(normalizedFile);
37+
this.angularDiagnostics.delete(sourceFile);
38+
}
3339
changedFiles.add(normalizedFile);
3440
}
3541
}
3642

3743
return changedFiles;
3844
}
45+
46+
updateAngularDiagnostics(sourceFile: ts.SourceFile, diagnostics: ts.Diagnostic[]): void {
47+
if (diagnostics.length > 0) {
48+
this.angularDiagnostics.set(sourceFile, diagnostics);
49+
} else {
50+
this.angularDiagnostics.delete(sourceFile);
51+
}
52+
}
53+
54+
getAngularDiagnostics(sourceFile: ts.SourceFile): ts.Diagnostic[] | undefined {
55+
return this.angularDiagnostics.get(sourceFile);
56+
}
3957
}

0 commit comments

Comments
 (0)