Skip to content

Commit 3e5a99c

Browse files
clydinalan-agius4
clydin
authored and
alan-agius4
committed
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. (cherry picked from commit 06dacb0)
1 parent 58bd397 commit 3e5a99c

File tree

3 files changed

+322
-1
lines changed

3 files changed

+322
-1
lines changed

‎packages/angular_devkit/build_angular/BUILD.bazel

+3-1
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,9 @@ ts_library(
310310

311311
LARGE_SPECS = {
312312
"application": {
313-
"shards": 10,
313+
"shards": 16,
314+
"size": "large",
315+
"flaky": True,
314316
"extra_deps": [
315317
"@npm//buffer",
316318
],

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

+317
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,317 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
import { logging } from '@angular-devkit/core';
10+
import { concatMap, count, timeout } from 'rxjs';
11+
import { buildApplication } from '../../index';
12+
import { APPLICATION_BUILDER_INFO, BASE_OPTIONS, describeBuilder } from '../setup';
13+
14+
/**
15+
* Maximum time in milliseconds for single build/rebuild
16+
* This accounts for CI variability.
17+
*/
18+
export const BUILD_TIMEOUT = 30_000;
19+
20+
describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
21+
describe('Behavior: "Rebuild Error Detection"', () => {
22+
it('detects template errors with no AOT codegen or TS emit differences', async () => {
23+
harness.useTarget('build', {
24+
...BASE_OPTIONS,
25+
watch: true,
26+
});
27+
28+
const goodDirectiveContents = `
29+
import { Directive, Input } from '@angular/core';
30+
@Directive({ selector: 'dir' })
31+
export class Dir {
32+
@Input() foo: number;
33+
}
34+
`;
35+
36+
const typeErrorText = `Type 'number' is not assignable to type 'string'.`;
37+
38+
// Create a directive and add to application
39+
await harness.writeFile('src/app/dir.ts', goodDirectiveContents);
40+
await harness.writeFile(
41+
'src/app/app.module.ts',
42+
`
43+
import { NgModule } from '@angular/core';
44+
import { BrowserModule } from '@angular/platform-browser';
45+
import { AppComponent } from './app.component';
46+
import { Dir } from './dir';
47+
@NgModule({
48+
declarations: [
49+
AppComponent,
50+
Dir,
51+
],
52+
imports: [
53+
BrowserModule
54+
],
55+
providers: [],
56+
bootstrap: [AppComponent]
57+
})
58+
export class AppModule { }
59+
`,
60+
);
61+
62+
// Create app component that uses the directive
63+
await harness.writeFile(
64+
'src/app/app.component.ts',
65+
`
66+
import { Component } from '@angular/core'
67+
@Component({
68+
selector: 'app-root',
69+
template: '<dir [foo]="123">',
70+
})
71+
export class AppComponent { }
72+
`,
73+
);
74+
75+
const builderAbort = new AbortController();
76+
const buildCount = await harness
77+
.execute({ outputLogsOnFailure: false, signal: builderAbort.signal })
78+
.pipe(
79+
timeout(BUILD_TIMEOUT),
80+
concatMap(async ({ result, logs }, index) => {
81+
switch (index) {
82+
case 0:
83+
expect(result?.success).toBeTrue();
84+
85+
// Update directive to use a different input type for 'foo' (number -> string)
86+
// Should cause a template error
87+
await harness.writeFile(
88+
'src/app/dir.ts',
89+
`
90+
import { Directive, Input } from '@angular/core';
91+
@Directive({ selector: 'dir' })
92+
export class Dir {
93+
@Input() foo: string;
94+
}
95+
`,
96+
);
97+
98+
break;
99+
case 1:
100+
expect(result?.success).toBeFalse();
101+
expect(logs).toContain(
102+
jasmine.objectContaining<logging.LogEntry>({
103+
message: jasmine.stringMatching(typeErrorText),
104+
}),
105+
);
106+
107+
// Make an unrelated change to verify error cache was updated
108+
// Should persist error in the next rebuild
109+
await harness.modifyFile('src/main.ts', (content) => content + '\n');
110+
111+
break;
112+
case 2:
113+
expect(result?.success).toBeFalse();
114+
expect(logs).toContain(
115+
jasmine.objectContaining<logging.LogEntry>({
116+
message: jasmine.stringMatching(typeErrorText),
117+
}),
118+
);
119+
120+
// Revert the directive change that caused the error
121+
// Should remove the error
122+
await harness.writeFile('src/app/dir.ts', goodDirectiveContents);
123+
124+
break;
125+
case 3:
126+
expect(result?.success).toBeTrue();
127+
expect(logs).not.toContain(
128+
jasmine.objectContaining<logging.LogEntry>({
129+
message: jasmine.stringMatching(typeErrorText),
130+
}),
131+
);
132+
133+
// Make an unrelated change to verify error cache was updated
134+
// Should continue showing no error
135+
await harness.modifyFile('src/main.ts', (content) => content + '\n');
136+
137+
break;
138+
case 4:
139+
expect(result?.success).toBeTrue();
140+
expect(logs).not.toContain(
141+
jasmine.objectContaining<logging.LogEntry>({
142+
message: jasmine.stringMatching(typeErrorText),
143+
}),
144+
);
145+
146+
// Test complete - abort watch mode
147+
builderAbort?.abort();
148+
break;
149+
}
150+
}),
151+
count(),
152+
)
153+
.toPromise();
154+
155+
expect(buildCount).toBe(5);
156+
});
157+
158+
it('detects cumulative block syntax errors', async () => {
159+
harness.useTarget('build', {
160+
...BASE_OPTIONS,
161+
watch: true,
162+
});
163+
164+
const builderAbort = new AbortController();
165+
const buildCount = await harness
166+
.execute({ outputLogsOnFailure: false, signal: builderAbort.signal })
167+
.pipe(
168+
timeout(BUILD_TIMEOUT),
169+
concatMap(async ({ logs }, index) => {
170+
switch (index) {
171+
case 0:
172+
// Add invalid block syntax
173+
await harness.appendToFile('src/app/app.component.html', '@one');
174+
175+
break;
176+
case 1:
177+
expect(logs).toContain(
178+
jasmine.objectContaining<logging.LogEntry>({
179+
message: jasmine.stringContaining('@one'),
180+
}),
181+
);
182+
183+
// Make an unrelated change to verify error cache was updated
184+
// Should persist error in the next rebuild
185+
await harness.modifyFile('src/main.ts', (content) => content + '\n');
186+
187+
break;
188+
case 2:
189+
expect(logs).toContain(
190+
jasmine.objectContaining<logging.LogEntry>({
191+
message: jasmine.stringContaining('@one'),
192+
}),
193+
);
194+
195+
// Add more invalid block syntax
196+
await harness.appendToFile('src/app/app.component.html', '@two');
197+
198+
break;
199+
case 3:
200+
expect(logs).toContain(
201+
jasmine.objectContaining<logging.LogEntry>({
202+
message: jasmine.stringContaining('@one'),
203+
}),
204+
);
205+
expect(logs).toContain(
206+
jasmine.objectContaining<logging.LogEntry>({
207+
message: jasmine.stringContaining('@two'),
208+
}),
209+
);
210+
211+
// Add more invalid block syntax
212+
await harness.appendToFile('src/app/app.component.html', '@three');
213+
214+
break;
215+
case 4:
216+
expect(logs).toContain(
217+
jasmine.objectContaining<logging.LogEntry>({
218+
message: jasmine.stringContaining('@one'),
219+
}),
220+
);
221+
expect(logs).toContain(
222+
jasmine.objectContaining<logging.LogEntry>({
223+
message: jasmine.stringContaining('@two'),
224+
}),
225+
);
226+
expect(logs).toContain(
227+
jasmine.objectContaining<logging.LogEntry>({
228+
message: jasmine.stringContaining('@three'),
229+
}),
230+
);
231+
232+
// Revert the changes that caused the error
233+
// Should remove the error
234+
await harness.writeFile('src/app/app.component.html', '<p>GOOD</p>');
235+
236+
break;
237+
case 5:
238+
expect(logs).not.toContain(
239+
jasmine.objectContaining<logging.LogEntry>({
240+
message: jasmine.stringContaining('@one'),
241+
}),
242+
);
243+
expect(logs).not.toContain(
244+
jasmine.objectContaining<logging.LogEntry>({
245+
message: jasmine.stringContaining('@two'),
246+
}),
247+
);
248+
expect(logs).not.toContain(
249+
jasmine.objectContaining<logging.LogEntry>({
250+
message: jasmine.stringContaining('@three'),
251+
}),
252+
);
253+
254+
// Test complete - abort watch mode
255+
builderAbort?.abort();
256+
break;
257+
}
258+
}),
259+
count(),
260+
)
261+
.toPromise();
262+
263+
expect(buildCount).toBe(6);
264+
});
265+
266+
it('recovers from component stylesheet error', async () => {
267+
harness.useTarget('build', {
268+
...BASE_OPTIONS,
269+
watch: true,
270+
aot: false,
271+
});
272+
273+
const builderAbort = new AbortController();
274+
const buildCount = await harness
275+
.execute({ outputLogsOnFailure: false, signal: builderAbort.signal })
276+
.pipe(
277+
timeout(BUILD_TIMEOUT),
278+
concatMap(async ({ result, logs }, index) => {
279+
switch (index) {
280+
case 0:
281+
await harness.writeFile('src/app/app.component.css', 'invalid-css-content');
282+
283+
break;
284+
case 1:
285+
expect(logs).toContain(
286+
jasmine.objectContaining<logging.LogEntry>({
287+
message: jasmine.stringMatching('invalid-css-content'),
288+
}),
289+
);
290+
291+
await harness.writeFile('src/app/app.component.css', 'p { color: green }');
292+
293+
break;
294+
case 2:
295+
expect(logs).not.toContain(
296+
jasmine.objectContaining<logging.LogEntry>({
297+
message: jasmine.stringMatching('invalid-css-content'),
298+
}),
299+
);
300+
301+
harness
302+
.expectFile('dist/browser/main.js')
303+
.content.toContain('p {\\n color: green;\\n}');
304+
305+
// Test complete - abort watch mode
306+
builderAbort?.abort();
307+
break;
308+
}
309+
}),
310+
count(),
311+
)
312+
.toPromise();
313+
314+
expect(buildCount).toBe(3);
315+
});
316+
});
317+
});

‎packages/angular_devkit/build_angular/src/tools/esbuild/angular/compilation/aot-compilation.ts

+2
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,8 @@ export class AotCompilation extends AngularCompilation {
105105
for (const resourceDependency of resourceDependencies) {
106106
if (hostOptions.modifiedFiles.has(resourceDependency)) {
107107
this.#state.diagnosticCache.delete(sourceFile);
108+
// Also mark as affected in case changed template affects diagnostics
109+
affectedFiles.add(sourceFile);
108110
}
109111
}
110112
}

0 commit comments

Comments
 (0)