fix(@angular-devkit/build-angular): Set chunk names explicitly

[gultyayev]

PR Checklist

Please check to confirm your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular-cli/blob/main/CONTRIBUTING.md#-commit-message-guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

What is the current behavior?

Currently, chunk names are determined by ESBuild. Some libraries may resolve to a name that is not in a format of chunk.hash.mjs. This causes Jest to run over the lib files and fail as it contains no test suite.

Fixes: #25189

What is the new behavior?

The change sets chunk name pattern explicitly so that all chunks are exluded by pattern from running tests over.

Does this PR introduce a breaking change?

• Yes
• No

Other information

I didn't figure out how this could be covered by tests. I did run a local build & installation and verified on a sample project that @azure/msal-angular no longer creates a "StandardController" file. Instead, it's named as chunk... and hence is excluded from tests.

[GinMitch]

This seems like a good idea, but can you please provide some context?

[gultyayev]

This seems like a good idea, but can you please provide some context?

Mostly, the issue comes down to ESBuild generating chunk names as chunk.hash.mjs in 99% of the cases. Sometimes, however it generates a chunk with a different name. For me it was StandardController.mjs for imported @azure/msal-angular library (v3.0.0-alpha.0).

When running tests with Jest, the builder is using pattern **/*.mjs to literally pick up all the mjs files. It does have exclusion for chunk files. Because of that, when running tests with Jest it fails, because StandardController.mjs is included to run tests over and it doesn't have any.

First idea was to simply change the pattern to .spec.mjs, but as it was highlighted by @dgp1130 users may change the tests name pattern. Therefore, it's easier to make all chunks follow the name pattern.

For build code it doesn't make any difference. For tests it makes sure that all chunks have the same name pattern and are excluded from the tests.

[dgp1130]

Thanks @gultyayev for looking into this, I haven't had a chance to dig in yet. I think this is probably a reasonable solution, though it looks like one of the tests is failing, as it is expecting the app to print the lazy loaded file's name which is no longer true. Maybe we could search for a more generic "rebuild done" string like "Application bundle generation complete"?

Also making this change in builder-context.ts means that it will apply to all ESBuild executions, not just Jest. I think that's probably ok, but I wonder if there might be some unintended consequences from it. @clydin, do you have any concerns with that?

[gultyayev]

Also making this change in builder-context.ts means that it will apply to all ESBuild executions, not just Jest.

TBH I can't think of any possible shortcomings from this. The names are always generated and hence cannot be relied on. Furthermore, current behavior is to create chunks with chunk-hsh.mjs name. So, technically we are fixing those limited cases when the name of the chunk is different.

It's also possible to pass this change for Jest builder only. I didn't locate the place though. Not sure if it would make much sense to do.

[alan-agius4]

Instead of setting the option in the builder context which only contains infrastructure related options. It would be better to follow the existing pattern for similar options and set it in

angular-cli/packages/angular_devkit/build_angular/src/tools/esbuild/application-code-bundle.ts

Line 183 in 4a8edff

function getEsBuildCommonOptions(options: NormalizedApplicationBuildOptions): BuildOptions {

or even better

angular-cli/packages/angular_devkit/build_angular/src/builders/application/options.ts

Line 92 in 4a8edff

const outputNames = {

[gultyayev]

Instead of setting the option in the builder context which only contains infrastructure related options. It would be better to follow the existing pattern for similar options and set it in

angular-cli/packages/angular_devkit/build_angular/src/tools/esbuild/application-code-bundle.ts

Line 183 in 4a8edff

function getEsBuildCommonOptions(options: NormalizedApplicationBuildOptions): BuildOptions {

or even better

angular-cli/packages/angular_devkit/build_angular/src/builders/application/options.ts

Line 92 in 4a8edff

const outputNames = {

Did the first suggested change. The 2nd one doesn't work.

[gultyayev]

One concern that may be is that chunk names become obscure. Current ng build outputs lazy loaded chunks as component-name.component-[hash].js. With the change it becomes just chunk-[hash] which may be undesirable.

[dgp1130]

@gultyayev, thanks for your patience. Using chunk-[hash] everywhere should be fine. It does make chunks a little harder to debug, however there is no general 1:1 correlation between chunks and source files, so preserving the name is maybe setting a bad expectation in the first place.

Just the one comment from @clydin about making sure a chunk is created on the rebuild and this should be good to go.

[dgp1130]

Thanks! This looks good to merge.

[gultyayev]

Thank you for guidance.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}