Skip to content

Commit b08a6c0

Browse files
committed
fix(@angular/build): retain watch files on error in load result cache
During incremental builds (serve and build --watch), caching an error result that lacks watch files would cause previously tracked dependency files to be dropped from the file watcher dependency map. For PostCSS plugins that read external dependency files (such as Tailwind configs or theme files), a syntax or parse error in a dependency file would result in an error being cached without watch files, preventing any subsequent edits to the dependency file from clearing the error until the entry stylesheet itself was modified. This commit improves MemoryLoadResultCache to remember the last-known watch set for each cache key across invalidations and union it into any newly cached error result. Additionally, compileString() in stylesheet-plugin-factory now explicitly returns error.file in watchFiles when catching a PostCSS CssSyntaxError, mirroring Sass and Less behavior. Closes #33666
1 parent b24b444 commit b08a6c0

4 files changed

Lines changed: 215 additions & 0 deletions

File tree

packages/angular/build/src/builders/application/tests/behavior/rebuild-global_styles_spec.ts

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,5 +132,119 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
132132
{ outputLogsOnFailure: false },
133133
);
134134
});
135+
136+
it('rebuilds PostCSS stylesheet after error on rebuild from plugin dependency', async () => {
137+
harness.useTarget('build', {
138+
...BASE_OPTIONS,
139+
watch: true,
140+
styles: ['src/styles.css'],
141+
});
142+
143+
await harness.writeFile(
144+
'test-plugin.js',
145+
`
146+
const fs = require('fs');
147+
const path = require('path');
148+
module.exports = () => {
149+
return {
150+
postcssPlugin: 'test-plugin',
151+
Once(root, { result }) {
152+
const themePath = path.join(path.dirname(root.source.input.file), 'theme.json');
153+
result.messages.push({
154+
type: 'dependency',
155+
file: themePath,
156+
});
157+
const data = fs.readFileSync(themePath, 'utf-8');
158+
const json = JSON.parse(data);
159+
root.append('body { color: ' + json.color + '; }');
160+
},
161+
};
162+
};
163+
module.exports.postcss = true;
164+
`,
165+
);
166+
await harness.writeFile(
167+
'.postcssrc.json',
168+
JSON.stringify({
169+
plugins: {
170+
'./test-plugin.js': {},
171+
},
172+
}),
173+
);
174+
await harness.writeFile('src/styles.css', '/* base */');
175+
await harness.writeFile('src/theme.json', '{"color": "aqua"}');
176+
177+
await harness.executeWithCases(
178+
[
179+
async ({ result }) => {
180+
expect(result?.success).toBe(true);
181+
harness.expectFile('dist/browser/styles.css').content.toContain('color: aqua');
182+
harness.expectFile('dist/browser/styles.css').content.not.toContain('color: blue');
183+
184+
await harness.writeFile('src/theme.json', 'invalid-json');
185+
},
186+
async ({ result }) => {
187+
expect(result?.success).toBe(false);
188+
189+
await harness.writeFile('src/theme.json', '{"color": "blue"}');
190+
},
191+
({ result }) => {
192+
expect(result?.success).toBe(true);
193+
harness.expectFile('dist/browser/styles.css').content.not.toContain('color: aqua');
194+
harness.expectFile('dist/browser/styles.css').content.toContain('color: blue');
195+
},
196+
],
197+
{ outputLogsOnFailure: false },
198+
);
199+
});
200+
201+
it('rebuilds PostCSS stylesheet after CSS syntax error on initial build from import', async () => {
202+
harness.useTarget('build', {
203+
...BASE_OPTIONS,
204+
watch: true,
205+
styles: ['src/styles.css'],
206+
});
207+
208+
await harness.writeFile(
209+
'noop-plugin.js',
210+
`
211+
module.exports = () => ({ postcssPlugin: 'noop-plugin' });
212+
module.exports.postcss = true;
213+
`,
214+
);
215+
await harness.writeFile(
216+
'.postcssrc.json',
217+
JSON.stringify({
218+
plugins: {
219+
'./noop-plugin.js': {},
220+
},
221+
}),
222+
);
223+
await harness.writeFile('src/styles.css', "@import './a.css';");
224+
await harness.writeFile('src/a.css', "a { ' }");
225+
226+
await harness.executeWithCases(
227+
[
228+
async ({ result }) => {
229+
expect(result?.success).toBe(false);
230+
231+
await harness.writeFile('src/a.css', 'body { color: aqua; }');
232+
},
233+
async ({ result }) => {
234+
expect(result?.success).toBe(true);
235+
harness.expectFile('dist/browser/styles.css').content.toContain('color: aqua');
236+
harness.expectFile('dist/browser/styles.css').content.not.toContain('color: blue');
237+
238+
await harness.writeFile('src/a.css', 'body { color: blue; }');
239+
},
240+
({ result }) => {
241+
expect(result?.success).toBe(true);
242+
harness.expectFile('dist/browser/styles.css').content.not.toContain('color: aqua');
243+
harness.expectFile('dist/browser/styles.css').content.toContain('color: blue');
244+
},
245+
],
246+
{ outputLogsOnFailure: false },
247+
);
248+
});
135249
});
136250
});

packages/angular/build/src/tools/esbuild/load-result-cache.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,26 @@ export function createCachedLoad(
5050
export class MemoryLoadResultCache implements LoadResultCache {
5151
#loadResults = new Map<string, OnLoadResult>();
5252
#fileDependencies = new Map<string, Set<string>>();
53+
#watchFilesPerKey = new Map<string, ReadonlyArray<string>>();
5354

5455
get(path: string): OnLoadResult | undefined {
5556
return this.#loadResults.get(path);
5657
}
5758

5859
async put(path: string, result: OnLoadResult): Promise<void> {
60+
if (result.errors && result.errors.length > 0) {
61+
const previousWatchFiles = this.#watchFilesPerKey.get(path);
62+
if (previousWatchFiles) {
63+
result.watchFiles = Array.from(
64+
new Set([...(result.watchFiles ?? []), ...previousWatchFiles]),
65+
);
66+
}
67+
} else if (result.watchFiles && result.watchFiles.length > 0) {
68+
this.#watchFilesPerKey.set(path, result.watchFiles);
69+
} else {
70+
this.#watchFilesPerKey.delete(path);
71+
}
72+
5973
this.#loadResults.set(path, result);
6074
if (result.watchFiles) {
6175
for (const watchFile of result.watchFiles) {
@@ -96,5 +110,6 @@ export class MemoryLoadResultCache implements LoadResultCache {
96110
clear(): void {
97111
this.#loadResults.clear();
98112
this.#fileDependencies.clear();
113+
this.#watchFilesPerKey.clear();
99114
}
100115
}
Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,85 @@
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.dev/license
7+
*/
8+
9+
import { MemoryLoadResultCache } from './load-result-cache';
10+
11+
describe('MemoryLoadResultCache', () => {
12+
let cache: MemoryLoadResultCache;
13+
14+
beforeEach(() => {
15+
cache = new MemoryLoadResultCache();
16+
});
17+
18+
it('should store and retrieve results', async () => {
19+
const result = {
20+
contents: 'body { color: red; }',
21+
loader: 'css' as const,
22+
};
23+
24+
await cache.put('file:/test/styles.css', result);
25+
const cached = cache.get('file:/test/styles.css');
26+
27+
expect(cached).toBe(result);
28+
});
29+
30+
it('should track watch files in fileDependencies', async () => {
31+
const result = {
32+
contents: 'body { color: red; }',
33+
loader: 'css' as const,
34+
watchFiles: ['/test/styles.css', '/test/theme.json'],
35+
};
36+
37+
await cache.put('file:/test/styles.css', result);
38+
39+
expect(cache.watchFiles).toContain('/test/styles.css');
40+
expect(cache.watchFiles).toContain('/test/theme.json');
41+
});
42+
43+
it('should invalidate cached results when a dependency changes', async () => {
44+
const result = {
45+
contents: 'body { color: red; }',
46+
loader: 'css' as const,
47+
watchFiles: ['/test/styles.css', '/test/theme.json'],
48+
};
49+
50+
await cache.put('file:/test/styles.css', result);
51+
expect(cache.get('file:/test/styles.css')).toBe(result);
52+
53+
const invalidated = cache.invalidate('/test/theme.json');
54+
expect(invalidated).toBeTrue();
55+
expect(cache.get('file:/test/styles.css')).toBeUndefined();
56+
});
57+
58+
it('should preserve previous watch files when caching an error result', async () => {
59+
const successResult = {
60+
contents: 'body { color: red; }',
61+
loader: 'css' as const,
62+
watchFiles: ['/test/styles.css', '/test/theme.json'],
63+
};
64+
65+
await cache.put('file:/test/styles.css', successResult);
66+
cache.invalidate('/test/theme.json');
67+
68+
// Simulate an incremental rebuild error result that only has the entry file in watchFiles
69+
const errorResult = {
70+
errors: [{ text: 'Syntax error in theme.json' }],
71+
watchFiles: ['/test/styles.css'],
72+
};
73+
74+
await cache.put('file:/test/styles.css', errorResult);
75+
76+
// Both the entry file and the previous dependency should be tracked
77+
expect(cache.watchFiles).toContain('/test/styles.css');
78+
expect(cache.watchFiles).toContain('/test/theme.json');
79+
80+
// Invalidating the dependency should clear the cached error result
81+
const invalidated = cache.invalidate('/test/theme.json');
82+
expect(invalidated).toBeTrue();
83+
expect(cache.get('file:/test/styles.css')).toBeUndefined();
84+
});
85+
});

packages/angular/build/src/tools/esbuild/stylesheets/stylesheet-plugin-factory.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,6 +427,7 @@ async function compileString(
427427
},
428428
},
429429
],
430+
watchFiles: error.file ? [filename, error.file] : [filename],
430431
};
431432
} else {
432433
assertIsError(error);

0 commit comments

Comments
 (0)