Skip to content

Commit 9c4a6be

Browse files
clydinclydin
committed
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.
1 parent 03a1eaf commit 9c4a6be

File tree

5 files changed

+88
-60
lines changed

5 files changed

+88
-60
lines changed

‎packages/angular_devkit/build_angular/src/builders/application/build-action.ts

Lines changed: 71 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ export async function* runEsBuildBuildAction(
3030
progress?: boolean;
3131
deleteOutputPath?: boolean;
3232
poll?: number;
33+
signal?: AbortSignal;
3334
},
3435
): AsyncIterable<(ExecutionResult['outputWithFiles'] | ExecutionResult['output']) & BuilderOutput> {
3536
const {
@@ -75,75 +76,89 @@ export async function* runEsBuildBuildAction(
7576
let result: ExecutionResult;
7677
try {
7778
result = await withProgress('Building...', () => action());
78-
79-
if (writeToFileSystem) {
80-
// Write output files
81-
await writeResultFiles(result.outputFiles, result.assetFiles, outputPath);
82-
83-
yield result.output;
84-
} else {
85-
// Requires casting due to unneeded `JsonObject` requirement. Remove once fixed.
86-
// eslint-disable-next-line @typescript-eslint/no-explicit-any
87-
yield result.outputWithFiles as any;
88-
}
89-
90-
// Finish if watch mode is not enabled
91-
if (!watch) {
92-
return;
93-
}
9479
} finally {
9580
// Ensure Sass workers are shutdown if not watching
9681
if (!watch) {
9782
shutdownSassWorkerPool();
9883
}
9984
}
10085

101-
if (progress) {
102-
logger.info('Watch mode enabled. Watching for file changes...');
86+
// Setup watcher if watch mode enabled
87+
let watcher: import('../../tools/esbuild/watcher').BuildWatcher | undefined;
88+
if (watch) {
89+
if (progress) {
90+
logger.info('Watch mode enabled. Watching for file changes...');
91+
}
92+
93+
// Setup a watcher
94+
const { createWatcher } = await import('../../tools/esbuild/watcher');
95+
watcher = createWatcher({
96+
polling: typeof poll === 'number',
97+
interval: poll,
98+
ignored: [
99+
// Ignore the output and cache paths to avoid infinite rebuild cycles
100+
outputPath,
101+
cacheOptions.basePath,
102+
// Ignore all node modules directories to avoid excessive file watchers.
103+
// Package changes are handled below by watching manifest and lock files.
104+
'**/node_modules/**',
105+
'**/.*/**',
106+
],
107+
});
108+
109+
// Setup abort support
110+
options.signal?.addEventListener('abort', () => void watcher?.close());
111+
112+
// Temporarily watch the entire project
113+
watcher.add(projectRoot);
114+
115+
// Watch workspace for package manager changes
116+
const packageWatchFiles = [
117+
// manifest can affect module resolution
118+
'package.json',
119+
// npm lock file
120+
'package-lock.json',
121+
// pnpm lock file
122+
'pnpm-lock.yaml',
123+
// yarn lock file including Yarn PnP manifest files (https://yarnpkg.com/advanced/pnp-spec/)
124+
'yarn.lock',
125+
'.pnp.cjs',
126+
'.pnp.data.json',
127+
];
128+
129+
watcher.add(packageWatchFiles.map((file) => path.join(workspaceRoot, file)));
130+
131+
// Watch locations provided by the initial build result
132+
watcher.add(result.watchFiles);
103133
}
104134

105-
// Setup a watcher
106-
const { createWatcher } = await import('../../tools/esbuild/watcher');
107-
const watcher = createWatcher({
108-
polling: typeof poll === 'number',
109-
interval: poll,
110-
ignored: [
111-
// Ignore the output and cache paths to avoid infinite rebuild cycles
112-
outputPath,
113-
cacheOptions.basePath,
114-
// Ignore all node modules directories to avoid excessive file watchers.
115-
// Package changes are handled below by watching manifest and lock files.
116-
'**/node_modules/**',
117-
'**/.*/**',
118-
],
119-
});
120-
121-
// Temporarily watch the entire project
122-
watcher.add(projectRoot);
123-
124-
// Watch workspace for package manager changes
125-
const packageWatchFiles = [
126-
// manifest can affect module resolution
127-
'package.json',
128-
// npm lock file
129-
'package-lock.json',
130-
// pnpm lock file
131-
'pnpm-lock.yaml',
132-
// yarn lock file including Yarn PnP manifest files (https://yarnpkg.com/advanced/pnp-spec/)
133-
'yarn.lock',
134-
'.pnp.cjs',
135-
'.pnp.data.json',
136-
];
137-
138-
watcher.add(packageWatchFiles.map((file) => path.join(workspaceRoot, file)));
139-
140-
// Watch locations provided by the initial build result
141-
let previousWatchFiles = new Set(result.watchFiles);
142-
watcher.add(result.watchFiles);
135+
// Output the first build results after setting up the watcher to ensure that any code executed
136+
// higher in the iterator call stack will trigger the watcher. This is particularly relevant for
137+
// unit tests which execute the builder and modify the file system programmatically.
138+
if (writeToFileSystem) {
139+
// Write output files
140+
await writeResultFiles(result.outputFiles, result.assetFiles, outputPath);
141+
142+
yield result.output;
143+
} else {
144+
// Requires casting due to unneeded `JsonObject` requirement. Remove once fixed.
145+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
146+
yield result.outputWithFiles as any;
147+
}
148+
149+
// Finish if watch mode is not enabled
150+
if (!watcher) {
151+
return;
152+
}
143153

144154
// Wait for changes and rebuild as needed
155+
let previousWatchFiles = new Set(result.watchFiles);
145156
try {
146157
for await (const changes of watcher) {
158+
if (options.signal?.aborted) {
159+
break;
160+
}
161+
147162
if (verbose) {
148163
logger.info(changes.toDebugString());
149164
}

‎packages/angular_devkit/build_angular/src/builders/application/index.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ import { Schema as ApplicationBuilderOptions } from './schema';
1717

1818
export async function* buildApplicationInternal(
1919
options: ApplicationBuilderInternalOptions,
20-
context: BuilderContext,
20+
// TODO: Integrate abort signal support into builder system
21+
context: BuilderContext & { signal?: AbortSignal },
2122
infrastructureSettings?: {
2223
write?: boolean;
2324
},
@@ -73,6 +74,7 @@ export async function* buildApplicationInternal(
7374
progress: normalizedOptions.progress,
7475
writeToFileSystem: infrastructureSettings?.write,
7576
logger: context.logger,
77+
signal: context.signal,
7678
},
7779
);
7880
}

‎packages/angular_devkit/build_angular/src/builders/application/tests/options/inline-style-language_spec.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
7575
harness.expectFile('dist/main.js').content.toContain('color: indianred');
7676
});
7777

78-
xit('updates produced stylesheet in watch mode', async () => {
78+
it('updates produced stylesheet in watch mode', async () => {
7979
harness.useTarget('build', {
8080
...BASE_OPTIONS,
8181
inlineStyleLanguage: InlineStyleLanguage.Scss,
@@ -87,8 +87,9 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
8787
content.replace('__STYLE_MARKER__', '$primary: indianred;\\nh1 { color: $primary; }'),
8888
);
8989

90+
const builderAbort = new AbortController();
9091
const buildCount = await harness
91-
.execute()
92+
.execute({ signal: builderAbort.signal })
9293
.pipe(
9394
timeout(30000),
9495
concatMap(async ({ result }, index) => {
@@ -121,10 +122,12 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
121122
harness.expectFile('dist/main.js').content.not.toContain('color: indianred');
122123
harness.expectFile('dist/main.js').content.not.toContain('color: aqua');
123124
harness.expectFile('dist/main.js').content.toContain('color: blue');
125+
126+
// Test complete - abort watch mode
127+
builderAbort.abort();
124128
break;
125129
}
126130
}),
127-
take(3),
128131
count(),
129132
)
130133
.toPromise();

‎packages/angular_devkit/build_angular/src/builders/application/tests/setup.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,4 +28,8 @@ export const BASE_OPTIONS = Object.freeze<Schema>({
2828

2929
// Disable optimizations
3030
optimization: false,
31+
32+
// Enable polling (if a test enables watch mode).
33+
// This is a workaround for bazel isolation file watch not triggering in tests.
34+
poll: 100,
3135
});

‎packages/angular_devkit/build_angular/src/testing/builder-harness.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ export interface BuilderHarnessExecutionOptions {
5151
outputLogsOnFailure: boolean;
5252
outputLogsOnException: boolean;
5353
useNativeFileWatching: boolean;
54+
signal: AbortSignal;
5455
}
5556

5657
/**
@@ -235,6 +236,7 @@ export class BuilderHarness<T> {
235236
this.builderInfo,
236237
this.resolvePath('.'),
237238
contextHost,
239+
options.signal,
238240
useNativeFileWatching ? undefined : this.watcherNotifier,
239241
);
240242
if (this.targetName !== undefined) {
@@ -389,6 +391,7 @@ class HarnessBuilderContext implements BuilderContext {
389391
public builder: BuilderInfo,
390392
basePath: string,
391393
private readonly contextHost: ContextHost,
394+
public readonly signal: AbortSignal | undefined,
392395
public readonly watcherFactory: BuilderWatcherFactory | undefined,
393396
) {
394397
this.workspaceRoot = this.currentDirectory = basePath;
@@ -442,6 +445,7 @@ class HarnessBuilderContext implements BuilderContext {
442445
info,
443446
this.workspaceRoot,
444447
this.contextHost,
448+
this.signal,
445449
this.watcherFactory,
446450
);
447451
context.target = target;

0 commit comments

Comments
 (0)