From cbe55df49a1dfbe0ef4a616988bec76e502c62e8 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Mon, 13 Nov 2023 11:58:26 -0500 Subject: [PATCH 1/2] fix(@angular-devkit/build-angular): maintain current watch files after build errors When using the `application` and/or `browser-esbuild` builders, the current watch files will be maintained in the event of a build error. This better ensures that files used in the last build/rebuild that may have caused an error will continue to be watched and refreshed when later changed. Only when a successful rebuild has occurred will any stale watch files be removed from the watch list. --- .../src/builders/application/build-action.ts | 25 +++++++++++++------ 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/angular_devkit/build_angular/src/builders/application/build-action.ts b/packages/angular_devkit/build_angular/src/builders/application/build-action.ts index 4ce023c4a70f..08b478687ac5 100644 --- a/packages/angular_devkit/build_angular/src/builders/application/build-action.ts +++ b/packages/angular_devkit/build_angular/src/builders/application/build-action.ts @@ -138,7 +138,7 @@ export async function* runEsBuildBuildAction( } // Wait for changes and rebuild as needed - let previousWatchFiles = new Set(result.watchFiles); + const currentWatchFiles = new Set(result.watchFiles); try { for await (const changes of watcher) { if (options.signal?.aborted) { @@ -154,12 +154,23 @@ export async function* runEsBuildBuildAction( ); // Update watched locations provided by the new build result. - // Add any new locations - watcher.add(result.watchFiles.filter((watchFile) => !previousWatchFiles.has(watchFile))); - const newWatchFiles = new Set(result.watchFiles); - // Remove any old locations - watcher.remove([...previousWatchFiles].filter((watchFile) => !newWatchFiles.has(watchFile))); - previousWatchFiles = newWatchFiles; + // Keep watching all previous files if there are any errors; otherwise consider all + // files stale until confirmed present in the new result's watch files. + const staleWatchFiles = result.errors.length > 0 ? undefined : new Set(currentWatchFiles); + for (const watchFile of result.watchFiles) { + if (!currentWatchFiles.has(watchFile)) { + // Add new watch location + watcher.add(watchFile); + currentWatchFiles.add(watchFile); + } + + // Present so remove from stale locations + staleWatchFiles?.delete(watchFile); + } + // Remove any stale locations if the build was successful + if (staleWatchFiles?.size) { + watcher.remove([...staleWatchFiles]); + } if (writeToFileSystem) { // Write output files From d0fb3b391e9e9ee1927228c6387e562c3793ebf2 Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Mon, 13 Nov 2023 14:38:14 -0500 Subject: [PATCH 2/2] fix(@angular-devkit/build-angular): cache stylesheet load errors with application builder When using the `application` and/or `browser-esbuild` builders, stylesheets that generate errors will now have the errors cached between rebuilds during watch mode (include `ng serve`). This not only avoids reprocessing files that contain errors and that have not changed but also provides file watching information from the cache to ensure the error-causing files are properly invalidated. --- .../behavior/rebuild-global_styles_spec.ts | 121 ++++++++++++++++++ .../src/tools/esbuild/bundler-context.ts | 27 ++-- .../src/tools/esbuild/load-result-cache.ts | 4 +- .../esbuild/stylesheets/less-language.ts | 7 +- .../esbuild/stylesheets/sass-language.ts | 4 +- 5 files changed, 147 insertions(+), 16 deletions(-) create mode 100644 packages/angular_devkit/build_angular/src/builders/application/tests/behavior/rebuild-global_styles_spec.ts diff --git a/packages/angular_devkit/build_angular/src/builders/application/tests/behavior/rebuild-global_styles_spec.ts b/packages/angular_devkit/build_angular/src/builders/application/tests/behavior/rebuild-global_styles_spec.ts new file mode 100644 index 000000000000..fde1b20ddb0b --- /dev/null +++ b/packages/angular_devkit/build_angular/src/builders/application/tests/behavior/rebuild-global_styles_spec.ts @@ -0,0 +1,121 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * 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 + */ + +import { concatMap, count, timeout } from 'rxjs'; +import { buildApplication } from '../../index'; +import { APPLICATION_BUILDER_INFO, BASE_OPTIONS, describeBuilder } from '../setup'; + +/** + * Maximum time in milliseconds for single build/rebuild + * This accounts for CI variability. + */ +export const BUILD_TIMEOUT = 30_000; + +describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => { + describe('Behavior: "Rebuilds when global stylesheets change"', () => { + beforeEach(async () => { + // Application code is not needed for styles tests + await harness.writeFile('src/main.ts', 'console.log("TEST");'); + }); + + it('rebuilds Sass stylesheet after error on rebuild from import', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + watch: true, + styles: ['src/styles.scss'], + }); + + await harness.writeFile('src/styles.scss', "@import './a';"); + await harness.writeFile('src/a.scss', '$primary: aqua;\\nh1 { color: $primary; }'); + + const builderAbort = new AbortController(); + const buildCount = await harness + .execute({ signal: builderAbort.signal, outputLogsOnFailure: false }) + .pipe( + timeout(30000), + concatMap(async ({ result }, index) => { + switch (index) { + case 0: + expect(result?.success).toBe(true); + harness.expectFile('dist/browser/styles.css').content.toContain('color: aqua'); + harness.expectFile('dist/browser/styles.css').content.not.toContain('color: blue'); + + await harness.writeFile( + 'src/a.scss', + 'invalid-invalid-invalid\\nh1 { color: $primary; }', + ); + break; + case 1: + expect(result?.success).toBe(false); + + await harness.writeFile('src/a.scss', '$primary: blue;\\nh1 { color: $primary; }'); + break; + case 2: + expect(result?.success).toBe(true); + harness.expectFile('dist/browser/styles.css').content.not.toContain('color: aqua'); + harness.expectFile('dist/browser/styles.css').content.toContain('color: blue'); + + // Test complete - abort watch mode + builderAbort.abort(); + break; + } + }), + count(), + ) + .toPromise(); + + expect(buildCount).toBe(3); + }); + + it('rebuilds Sass stylesheet after error on initial build from import', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + watch: true, + styles: ['src/styles.scss'], + }); + + await harness.writeFile('src/styles.scss', "@import './a';"); + await harness.writeFile('src/a.scss', 'invalid-invalid-invalid\\nh1 { color: $primary; }'); + + const builderAbort = new AbortController(); + const buildCount = await harness + .execute({ signal: builderAbort.signal, outputLogsOnFailure: false }) + .pipe( + timeout(30000), + concatMap(async ({ result }, index) => { + switch (index) { + case 0: + expect(result?.success).toBe(false); + + await harness.writeFile('src/a.scss', '$primary: aqua;\\nh1 { color: $primary; }'); + break; + case 1: + expect(result?.success).toBe(true); + harness.expectFile('dist/browser/styles.css').content.toContain('color: aqua'); + harness.expectFile('dist/browser/styles.css').content.not.toContain('color: blue'); + + await harness.writeFile('src/a.scss', '$primary: blue;\\nh1 { color: $primary; }'); + break; + case 2: + expect(result?.success).toBe(true); + harness.expectFile('dist/browser/styles.css').content.not.toContain('color: aqua'); + harness.expectFile('dist/browser/styles.css').content.toContain('color: blue'); + + // Test complete - abort watch mode + builderAbort.abort(); + break; + } + }), + count(), + ) + .toPromise(); + + expect(buildCount).toBe(3); + }); + }); +}); diff --git a/packages/angular_devkit/build_angular/src/tools/esbuild/bundler-context.ts b/packages/angular_devkit/build_angular/src/tools/esbuild/bundler-context.ts index 6d74c2594648..ab8dd55e489a 100644 --- a/packages/angular_devkit/build_angular/src/tools/esbuild/bundler-context.ts +++ b/packages/angular_devkit/build_angular/src/tools/esbuild/bundler-context.ts @@ -220,6 +220,16 @@ export class BundlerContext { } else { throw failure; } + } finally { + if (this.incremental) { + // When incremental always add any files from the load result cache + if (this.#loadCache) { + this.#loadCache.watchFiles + .filter((file) => !isInternalAngularFile(file)) + // watch files are fully resolved paths + .forEach((file) => this.watchFiles.add(file)); + } + } } // Update files that should be watched. @@ -228,16 +238,9 @@ export class BundlerContext { if (this.incremental) { // Add input files except virtual angular files which do not exist on disk Object.keys(result.metafile.inputs) - .filter((input) => !input.startsWith('angular:')) + .filter((input) => !isInternalAngularFile(input)) // input file paths are always relative to the workspace root .forEach((input) => this.watchFiles.add(join(this.workspaceRoot, input))); - // Also add any files from the load result cache - if (this.#loadCache) { - this.#loadCache.watchFiles - .filter((file) => !file.startsWith('angular:')) - // watch files are fully resolved paths - .forEach((file) => this.watchFiles.add(file)); - } } // Return if the build encountered any errors @@ -349,12 +352,12 @@ export class BundlerContext { #addErrorsToWatch(result: BuildFailure | BuildResult): void { for (const error of result.errors) { let file = error.location?.file; - if (file) { + if (file && !isInternalAngularFile(file)) { this.watchFiles.add(join(this.workspaceRoot, file)); } for (const note of error.notes) { file = note.location?.file; - if (file) { + if (file && !isInternalAngularFile(file)) { this.watchFiles.add(join(this.workspaceRoot, file)); } } @@ -406,3 +409,7 @@ export class BundlerContext { } } } + +function isInternalAngularFile(file: string) { + return file.startsWith('angular:'); +} diff --git a/packages/angular_devkit/build_angular/src/tools/esbuild/load-result-cache.ts b/packages/angular_devkit/build_angular/src/tools/esbuild/load-result-cache.ts index 987aea40caf1..78553d314833 100644 --- a/packages/angular_devkit/build_angular/src/tools/esbuild/load-result-cache.ts +++ b/packages/angular_devkit/build_angular/src/tools/esbuild/load-result-cache.ts @@ -30,8 +30,8 @@ export function createCachedLoad( if (result === undefined) { result = await callback(args); - // Do not cache null or undefined or results with errors - if (result && result.errors === undefined) { + // Do not cache null or undefined + if (result) { await cache.put(loadCacheKey, result); } } diff --git a/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/less-language.ts b/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/less-language.ts index 4d86a6424e6d..57fba55c65d3 100644 --- a/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/less-language.ts +++ b/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/less-language.ts @@ -115,6 +115,8 @@ async function compileString( }; } catch (error) { if (isLessException(error)) { + const location = convertExceptionLocation(error); + // Retry with a warning for less files requiring the deprecated inline JavaScript option if (error.message.includes('Inline JavaScript is not enabled.')) { const withJsResult = await compileString( @@ -127,7 +129,7 @@ async function compileString( withJsResult.warnings = [ { text: 'Deprecated inline execution of JavaScript has been enabled ("javascriptEnabled")', - location: convertExceptionLocation(error), + location, notes: [ { location: null, @@ -148,10 +150,11 @@ async function compileString( errors: [ { text: error.message, - location: convertExceptionLocation(error), + location, }, ], loader: 'css', + watchFiles: location.file ? [filename, location.file] : [filename], }; } diff --git a/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/sass-language.ts b/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/sass-language.ts index 438ff620aa6f..41505eb20c63 100644 --- a/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/sass-language.ts +++ b/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/sass-language.ts @@ -176,7 +176,7 @@ async function compileString( }; } catch (error) { if (isSassException(error)) { - const file = error.span.url ? fileURLToPath(error.span.url) : undefined; + const fileWithError = error.span.url ? fileURLToPath(error.span.url) : undefined; return { loader: 'css', @@ -186,7 +186,7 @@ async function compileString( }, ], warnings, - watchFiles: file ? [file] : undefined, + watchFiles: fileWithError ? [filePath, fileWithError] : [filePath], }; }