Skip to content

Commit 7686d66

Browse files
clydinalan-agius4
authored andcommitted
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 (cherry picked from commit bcadbbe)
1 parent 1d9122f commit 7686d66

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
@@ -48,12 +48,26 @@ export function createCachedLoad(
4848
export class MemoryLoadResultCache implements LoadResultCache {
4949
#loadResults = new Map<string, OnLoadResult>();
5050
#fileDependencies = new Map<string, Set<string>>();
51+
#watchFilesPerKey = new Map<string, ReadonlyArray<string>>();
5152

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

5657
async put(path: string, result: OnLoadResult): Promise<void> {
58+
if (result.errors && result.errors.length > 0) {
59+
const previousWatchFiles = this.#watchFilesPerKey.get(path);
60+
if (previousWatchFiles) {
61+
result.watchFiles = Array.from(
62+
new Set([...(result.watchFiles ?? []), ...previousWatchFiles]),
63+
);
64+
}
65+
} else if (result.watchFiles && result.watchFiles.length > 0) {
66+
this.#watchFilesPerKey.set(path, [...result.watchFiles]);
67+
} else {
68+
this.#watchFilesPerKey.delete(path);
69+
}
70+
5771
this.#loadResults.set(path, result);
5872
if (result.watchFiles) {
5973
for (const watchFile of result.watchFiles) {
@@ -94,5 +108,6 @@ export class MemoryLoadResultCache implements LoadResultCache {
94108
clear(): void {
95109
this.#loadResults.clear();
96110
this.#fileDependencies.clear();
111+
this.#watchFilesPerKey.clear();
97112
}
98113
}
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 && error.file !== filename ? [filename, error.file] : [filename],
430431
};
431432
} else {
432433
assertIsError(error);

0 commit comments

Comments
 (0)