Skip to content

fix(#31973): Added JavaScriptTransformer close to dispose method #32082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Aukevanoost wants to merge 1 commit into
base: main
Choose a base branch
from
+25 −5
Open

fix(#31973): Added JavaScriptTransformer close to dispose method #32082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/angular/build/src/private.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export function createCompilerPlugin(
),
);
}
export { getSharedCompilationStateCleanup } from './tools/esbuild/angular/compilation-state';

export type { AngularCompilation } from './tools/angular/compilation';
export { DiagnosticModes } from './tools/angular/compilation';
Expand Down
16 changes: 16 additions & 0 deletions packages/angular/build/src/tools/esbuild/angular/compilation-state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ export class SharedTSCompilationState {
#pendingCompilation = true;
#resolveCompilationReady: ((value: boolean) => void) | undefined;
#compilationReadyPromise: Promise<boolean> | undefined;

/**
* ESbuild doesnt allow for awaiting the cleanup of plugins, therefore this
* allows consumers of the compiler-plugin to await the disposal of the plugin
* after ESbuild dispose function was called.
*/
#disposeCompilation: (() => void) | undefined;
awaitCompilationDisposed: Promise<void> = new Promise((resolve) => {
this.#disposeCompilation = resolve;
});

#hasErrors = true;

get waitUntilReady(): Promise<boolean> {
Expand Down Expand Up @@ -37,6 +48,7 @@ export class SharedTSCompilationState {

dispose(): void {
this.markAsReady(true);
this.#disposeCompilation?.();
globalSharedCompilationState = undefined;
}
}
Expand All @@ -46,3 +58,7 @@ let globalSharedCompilationState: SharedTSCompilationState | undefined;
export function getSharedCompilationState(): SharedTSCompilationState {
return (globalSharedCompilationState ??= new SharedTSCompilationState());
}

export function getSharedCompilationStateCleanup() {
return globalSharedCompilationState?.awaitCompilationDisposed ?? Promise.resolve();
}
13 changes: 8 additions & 5 deletions packages/angular/build/src/tools/esbuild/angular/compiler-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,11 +588,14 @@ export function createCompilerPlugin(
logCumulativeDurations();
});

build.onDispose(() => {
sharedTSCompilationState?.dispose();
void compilation.close?.();
void cacheStore?.close();
});
build.onDispose(
Copy link
Collaborator

@alan-agius4 alan-agius4 Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this more, isn't the below enough to your issue? Is the getSharedCompilationStateCleanup truly needed?
build.onDispose(() => {
sharedTSCompilationState?.dispose();
void compilation.close?.();
void cacheStore?.close();
void javascriptTransformer.close();
});
The compilation, cacheStore and javascriptTransformer are local variables and scoped to a single plugin. The only shared instance is sharedTSCompilationState which it's clean up is sync. I am still failing to understand how the sharedTSCompilationState is causing the heap error, as this is used primarily for NoopCompilations which in your case you are not using.

My point here is that dispose can be fire and forget and it should not be blocking to start a new compilation, as long as sharedTSCompilationState.dispose is sync.

Copy link
Contributor Author

@Aukevanoost Aukevanoost Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi great question.

While the properties are local, their workers and state aren't. The sharedTSCompilationState is not the issue but the solution. The compilation and javascriptTransformer contain state and workers that crash the node:process on sequential ESbuilds. This is caused by the fact that the new build/compilation run is using workers and state that hasn't been cleaned up yet in the previous build, causing the heap corruption. The only way to fix this is to wait for all the workers and processes to be cleaned up before starting the next ESbuild run. The exposed getSharedCompilationStateCleanup function allows me to do this.

Copy link
Collaborator

@alan-agius4 alan-agius4 Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sharedTSCompilationState is not the issue but the solution.

But in your plugin you are not even using sharedTSCompilationState since none of the compilations mode is noop.

While the properties are local, their workers and state aren't .... The compilation and javascriptTransformer contain state and workers that crash the node:process on sequential ESbuilds. This is caused by the fact that the new build/compilation run is using workers and state that hasn't been cleaned up yet in the previous build

I'm confused by this. What non local state are you your referring too? Both the compilation and the transformer operate with local state; they do not rely on a share state. Therefore, creating multiple instances of createCompilerPlugin() will not result in a single, shared pool of workers or compilation environment. Each invocation remains completely isolated and initializes its own separate resources. We do this in the Angular CLI where each ng build creates multiple instances of createCompilerPlugin.

Copy link
Contributor Author

@Aukevanoost Aukevanoost Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are correct about the sharedTSCompilationState only used in noop mode, it is still being initialized regardless of the compilation type: https://github.com/angular/angular-cli/blob/c4af1932c3e8e94022237ea8e56adb19bf338978/packages/angular/build/src/tools/esbuild/angular/compiler-plugin.ts#L146C1-L146C64 therefore it is (only) being used for the clean disposal signal.

About the scoped workers and state, while I still can't proof exactly where it's happening, awaiting this disposal does prevent the heap error. I'll have to dig deeper to find out exactly which part is causing this.

Copy link
Collaborator

@alan-agius4 alan-agius4 Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are correct about the sharedTSCompilationState only used in noop mode, it is still being initialized regardless of the compilation type: https://github.com/angular/angular-cli/blob/c4af1932c3e8e94022237ea8e56adb19bf338978/packages/angular/build/src/tools/esbuild/angular/compiler-plugin.ts#L146C1-L146C64 therefore it is (only) being used for the clean disposal signal

That seems like an oversight.

() =>
void Promise.all(
[compilation?.close?.(), cacheStore?.close(), javascriptTransformer.close()].filter(
Copy link
Collaborator

@alan-agius4 alan-agius4 Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do not mind, I might create (or you can if you wish too), to actually call include just void javascriptTransformer.close(); so that it's included in today's release.

Copy link
Contributor Author

@Aukevanoost Aukevanoost Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can create a separate PR for this if you don't mind!

Copy link
Collaborator

@alan-agius4 alan-agius4 Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, go ahead.

Boolean,
),
).then(() => sharedTSCompilationState?.dispose()),
);

/**
* Checks if the file has side-effects when `advancedOptimizations` is enabled.
Expand Down