From 2bc66652a001357557d3b87cc90db3d0d9561a1a Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Tue, 3 Oct 2023 15:35:13 -0400 Subject: [PATCH] refactor(@angular-devkit/build-angular): improve accuracy of programmatic watch mode usage for esbuild builders To better capture file changes after the initial build for the esbuild-based builders in a programmatic usage, the file watching initialization has been moved to before the first build results are yielded. This allows tests that execute code to change files with improved accuracy of the watch mode triggering. The application builder now also supports aborting the watch mode programmatically. This allows tests to gracefully stop the watch mode and more fully cleanup the test at completion. --- .../src/builders/application/build-action.ts | 127 ++++++++++-------- .../src/builders/application/index.ts | 4 +- .../options/inline-style-language_spec.ts | 9 +- .../src/builders/application/tests/setup.ts | 4 + .../src/testing/builder-harness.ts | 4 + 5 files changed, 88 insertions(+), 60 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 602e3a8f4ad9..a0e677cc4e8d 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 @@ -30,6 +30,7 @@ export async function* runEsBuildBuildAction( progress?: boolean; deleteOutputPath?: boolean; poll?: number; + signal?: AbortSignal; }, ): AsyncIterable<(ExecutionResult['outputWithFiles'] | ExecutionResult['output']) & BuilderOutput> { const { @@ -75,22 +76,6 @@ export async function* runEsBuildBuildAction( let result: ExecutionResult; try { result = await withProgress('Building...', () => action()); - - if (writeToFileSystem) { - // Write output files - await writeResultFiles(result.outputFiles, result.assetFiles, outputPath); - - yield result.output; - } else { - // Requires casting due to unneeded `JsonObject` requirement. Remove once fixed. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - yield result.outputWithFiles as any; - } - - // Finish if watch mode is not enabled - if (!watch) { - return; - } } finally { // Ensure Sass workers are shutdown if not watching if (!watch) { @@ -98,52 +83,82 @@ export async function* runEsBuildBuildAction( } } - if (progress) { - logger.info('Watch mode enabled. Watching for file changes...'); + // Setup watcher if watch mode enabled + let watcher: import('../../tools/esbuild/watcher').BuildWatcher | undefined; + if (watch) { + if (progress) { + logger.info('Watch mode enabled. Watching for file changes...'); + } + + // Setup a watcher + const { createWatcher } = await import('../../tools/esbuild/watcher'); + watcher = createWatcher({ + polling: typeof poll === 'number', + interval: poll, + ignored: [ + // Ignore the output and cache paths to avoid infinite rebuild cycles + outputPath, + cacheOptions.basePath, + // Ignore all node modules directories to avoid excessive file watchers. + // Package changes are handled below by watching manifest and lock files. + '**/node_modules/**', + '**/.*/**', + ], + }); + + // Setup abort support + options.signal?.addEventListener('abort', () => void watcher?.close()); + + // Temporarily watch the entire project + watcher.add(projectRoot); + + // Watch workspace for package manager changes + const packageWatchFiles = [ + // manifest can affect module resolution + 'package.json', + // npm lock file + 'package-lock.json', + // pnpm lock file + 'pnpm-lock.yaml', + // yarn lock file including Yarn PnP manifest files (https://yarnpkg.com/advanced/pnp-spec/) + 'yarn.lock', + '.pnp.cjs', + '.pnp.data.json', + ]; + + watcher.add(packageWatchFiles.map((file) => path.join(workspaceRoot, file))); + + // Watch locations provided by the initial build result + watcher.add(result.watchFiles); } - // Setup a watcher - const { createWatcher } = await import('../../tools/esbuild/watcher'); - const watcher = createWatcher({ - polling: typeof poll === 'number', - interval: poll, - ignored: [ - // Ignore the output and cache paths to avoid infinite rebuild cycles - outputPath, - cacheOptions.basePath, - // Ignore all node modules directories to avoid excessive file watchers. - // Package changes are handled below by watching manifest and lock files. - '**/node_modules/**', - '**/.*/**', - ], - }); - - // Temporarily watch the entire project - watcher.add(projectRoot); - - // Watch workspace for package manager changes - const packageWatchFiles = [ - // manifest can affect module resolution - 'package.json', - // npm lock file - 'package-lock.json', - // pnpm lock file - 'pnpm-lock.yaml', - // yarn lock file including Yarn PnP manifest files (https://yarnpkg.com/advanced/pnp-spec/) - 'yarn.lock', - '.pnp.cjs', - '.pnp.data.json', - ]; - - watcher.add(packageWatchFiles.map((file) => path.join(workspaceRoot, file))); - - // Watch locations provided by the initial build result - let previousWatchFiles = new Set(result.watchFiles); - watcher.add(result.watchFiles); + // Output the first build results after setting up the watcher to ensure that any code executed + // higher in the iterator call stack will trigger the watcher. This is particularly relevant for + // unit tests which execute the builder and modify the file system programmatically. + if (writeToFileSystem) { + // Write output files + await writeResultFiles(result.outputFiles, result.assetFiles, outputPath); + + yield result.output; + } else { + // Requires casting due to unneeded `JsonObject` requirement. Remove once fixed. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + yield result.outputWithFiles as any; + } + + // Finish if watch mode is not enabled + if (!watcher) { + return; + } // Wait for changes and rebuild as needed + let previousWatchFiles = new Set(result.watchFiles); try { for await (const changes of watcher) { + if (options.signal?.aborted) { + break; + } + if (verbose) { logger.info(changes.toDebugString()); } diff --git a/packages/angular_devkit/build_angular/src/builders/application/index.ts b/packages/angular_devkit/build_angular/src/builders/application/index.ts index f9426ffb5c4e..fa51619def5c 100644 --- a/packages/angular_devkit/build_angular/src/builders/application/index.ts +++ b/packages/angular_devkit/build_angular/src/builders/application/index.ts @@ -17,7 +17,8 @@ import { Schema as ApplicationBuilderOptions } from './schema'; export async function* buildApplicationInternal( options: ApplicationBuilderInternalOptions, - context: BuilderContext, + // TODO: Integrate abort signal support into builder system + context: BuilderContext & { signal?: AbortSignal }, infrastructureSettings?: { write?: boolean; }, @@ -73,6 +74,7 @@ export async function* buildApplicationInternal( progress: normalizedOptions.progress, writeToFileSystem: infrastructureSettings?.write, logger: context.logger, + signal: context.signal, }, ); } diff --git a/packages/angular_devkit/build_angular/src/builders/application/tests/options/inline-style-language_spec.ts b/packages/angular_devkit/build_angular/src/builders/application/tests/options/inline-style-language_spec.ts index 564901da7f8e..154c449817d3 100644 --- a/packages/angular_devkit/build_angular/src/builders/application/tests/options/inline-style-language_spec.ts +++ b/packages/angular_devkit/build_angular/src/builders/application/tests/options/inline-style-language_spec.ts @@ -75,7 +75,7 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => { harness.expectFile('dist/main.js').content.toContain('color: indianred'); }); - xit('updates produced stylesheet in watch mode', async () => { + it('updates produced stylesheet in watch mode', async () => { harness.useTarget('build', { ...BASE_OPTIONS, inlineStyleLanguage: InlineStyleLanguage.Scss, @@ -87,8 +87,9 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => { content.replace('__STYLE_MARKER__', '$primary: indianred;\\nh1 { color: $primary; }'), ); + const builderAbort = new AbortController(); const buildCount = await harness - .execute() + .execute({ signal: builderAbort.signal }) .pipe( timeout(30000), concatMap(async ({ result }, index) => { @@ -121,10 +122,12 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => { harness.expectFile('dist/main.js').content.not.toContain('color: indianred'); harness.expectFile('dist/main.js').content.not.toContain('color: aqua'); harness.expectFile('dist/main.js').content.toContain('color: blue'); + + // Test complete - abort watch mode + builderAbort.abort(); break; } }), - take(3), count(), ) .toPromise(); diff --git a/packages/angular_devkit/build_angular/src/builders/application/tests/setup.ts b/packages/angular_devkit/build_angular/src/builders/application/tests/setup.ts index 4d07a8b4171a..bd98be89e442 100644 --- a/packages/angular_devkit/build_angular/src/builders/application/tests/setup.ts +++ b/packages/angular_devkit/build_angular/src/builders/application/tests/setup.ts @@ -28,4 +28,8 @@ export const BASE_OPTIONS = Object.freeze({ // Disable optimizations optimization: false, + + // Enable polling (if a test enables watch mode). + // This is a workaround for bazel isolation file watch not triggering in tests. + poll: 100, }); diff --git a/packages/angular_devkit/build_angular/src/testing/builder-harness.ts b/packages/angular_devkit/build_angular/src/testing/builder-harness.ts index c46c5c17363f..5c881e9e7b04 100644 --- a/packages/angular_devkit/build_angular/src/testing/builder-harness.ts +++ b/packages/angular_devkit/build_angular/src/testing/builder-harness.ts @@ -51,6 +51,7 @@ export interface BuilderHarnessExecutionOptions { outputLogsOnFailure: boolean; outputLogsOnException: boolean; useNativeFileWatching: boolean; + signal: AbortSignal; } /** @@ -235,6 +236,7 @@ export class BuilderHarness { this.builderInfo, this.resolvePath('.'), contextHost, + options.signal, useNativeFileWatching ? undefined : this.watcherNotifier, ); if (this.targetName !== undefined) { @@ -389,6 +391,7 @@ class HarnessBuilderContext implements BuilderContext { public builder: BuilderInfo, basePath: string, private readonly contextHost: ContextHost, + public readonly signal: AbortSignal | undefined, public readonly watcherFactory: BuilderWatcherFactory | undefined, ) { this.workspaceRoot = this.currentDirectory = basePath; @@ -442,6 +445,7 @@ class HarnessBuilderContext implements BuilderContext { info, this.workspaceRoot, this.contextHost, + this.signal, this.watcherFactory, ); context.target = target;