Skip to content

perf(@angular/build): directly check code for Angular partial linking #30375

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

Merged
merged 1 commit into from
May 22, 2025
Merged

perf(@angular/build): directly check code for Angular partial linking #30375

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
28 changes: 12 additions & 16 deletions packages/angular/build/src/tools/esbuild/javascript-transformer-worker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,14 @@ interface JavaScriptTransformRequest {
const textDecoder = new TextDecoder();
const textEncoder = new TextEncoder();

/**
* The function name prefix for all Angular partial compilation functions.
* Used to determine if linking of a JavaScript file is required.
* If any additional declarations are added or otherwise changed in the linker,
* the names MUST begin with this prefix.
*/
const LINKER_DECLARATION_PREFIX = 'ɵɵngDeclare';

export default async function transformJavaScript(
request: JavaScriptTransformRequest,
): Promise<unknown> {
Expand All @@ -46,11 +54,6 @@ let linkerPluginCreator:
| typeof import('@angular/compiler-cli/linker/babel').createEs2015LinkerPlugin
| undefined;

/**
* Cached instance of the compiler-cli linker's needsLinking function.
*/
let needsLinking: typeof import('@angular/compiler-cli/linker').needsLinking | undefined;

async function transformWithBabel(
filename: string,
data: string,
Expand Down Expand Up @@ -125,17 +128,10 @@ async function requiresLinking(path: string, source: string): Promise<boolean> {
return false;
}

if (!needsLinking) {
// Load ESM `@angular/compiler-cli/linker` using the TypeScript dynamic import workaround.
// Once TypeScript provides support for keeping the dynamic import this workaround can be
// changed to a direct dynamic import.
const linkerModule = await loadEsmModule<typeof import('@angular/compiler-cli/linker')>(
'@angular/compiler-cli/linker',
);
needsLinking = linkerModule.needsLinking;
}

return needsLinking(path, source);
// Check if the source code includes one of the declaration functions.
Copy link
Collaborator

@alan-agius4 alan-agius4 May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

NIT: Can we also add a faster check to check that the file extension is .mjs ? All Angular libraries are .mjs. This could also simplify the regexp for core and compiler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Third party angular libraries may not be though. I don't think APF mandates the file extension.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Libraries that are compiled with ng-packagr is though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Most probably use ng-packagr but i would be hesitant to assume all do. i think it may be good to do as a separate change in case it causes unintentional breakage.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, maybe APF should mandate something like .partial.mjs for a more encompassing check?

Copy link
Collaborator

@alan-agius4 alan-agius4 May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thoughts @devversion ?

Copy link
Member

Choose a reason for hiding this comment

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

IMO APF should always be partial compilation output, but since that is not necessarily the case for 1st party libraries, I could see adding a field to the package.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

Walking up the file system to find a relevant package.json for each file could get expensive.
At a minimum something like .partial.mjs could provide a definitive signal that it needs to be processed and no additional check would be needed. Other files could fallback to the existing checks.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I thought it would be easy to determine the package root, and have some caching.

In either case, I think it would be easy enough to introduce the .partial suffix

// There is a low chance of a false positive but the names are fairly unique
// and the result would be an unnecessary no-op additional plugin pass.
return source.includes(LINKER_DECLARATION_PREFIX);
}

async function createLinkerPlugin(options: Omit<JavaScriptTransformRequest, 'filename' | 'data'>) {
Expand Down