fix(@angular/build): disable code splitting for unit test builds - #33729
fix(@angular/build): disable code splitting for unit test builds#33729jonmarozick wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
b8a3e00 to
9e99820
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new internal option, disableCodeSplitting, to the application builder options. This option is designed to disable esbuild code splitting for browser code bundles, which is particularly useful for unit-test runners (such as Vitest) where module loading does not reliably preserve live ESM bindings across chunk boundaries. The option has been integrated into the option normalization process, the Vitest build options, and the browser code bundle creation logic. I have no feedback to provide as there are no review comments to address.
Every spec file is its own entry point, so esbuild code splitting hoists any module reached from more than one spec into a chunk shared between them. A module placed in a shared chunk is wrapped in a lazy `__esm` initializer, so its exported value is only assigned once that initializer runs, and importing chunks read the export as a live ESM binding. The unit test runners load the generated output through a module runner rather than the browser's own ESM implementation, and that does not reliably preserve those bindings. An importing chunk can therefore observe the export as `undefined`. A component whose class field initializer reads a `const` exported from a module that was hoisted into a shared chunk fails with a `TypeError`, while the same value read later, or read from within the shared chunk itself, is correct. It only appears once a project has more than one spec file, because a single entry point inlines everything and never splits. Test bundles are never downloaded by a browser, so splitting has nothing to optimize here. This disables it for the unit test build only, via an internal option, leaving application builds unaffected. The regression test mirrors the reproduction's exact shape: a shared const read during class-field initialization from a spec with an async test callback, under zone.js polyfills. That combination is load-bearing: zone.js downlevels async, the spec then imports the `__async` helper, and esbuild emits the spec entry CommonJS-wrapped with the component module behind a lazy `__esm` initializer. The fixture file set was verified to fail against an unpatched 21.2.19 build and pass with this change applied.
9e99820 to
746b351
Compare
1 participant
Fixes #33728
PR Checklist
Please check to confirm your PR fulfills the following requirements:
unit-test/tests/behavior/vitest-shared-chunk-init_spec.ts; see "Other information" for how the fixture was validated red/green.PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #33728
Under
@angular/build:unit-testwith the default (jsdom) runner, an export from a module that codesplitting hoisted into a shared chunk can be observed as
undefinedby an importing chunk — acomponent reading an exported
constin a class-field initializer fails withTypeError: Cannot read properties of undefined (reading 'map'), in a test that never touches therunner's own API. The emitted bundles are valid ESM and the same specs pass in browser mode, so the
defect is in how the Node-side module-runner path evaluates the split output, not in the bundles.
Every spec file is its own entry point, so splitting hoists any module reached from more than one
spec into a shared chunk behind lazy
__esminitializers; importing chunks then read the export asa live ESM binding, which that path does not reliably preserve at class-field-initialization time.
The full analysis — including the exact four-condition trigger set (shared chunk + cross-chunk
field read + an
asynctest callback + zone.js polyfills) isolated by single-variable toggles — isin #33728, with a minimal reproduction at https://github.com/jonmarozick/ng-shared-chunk-repro
(
npm install && npm test: expected 2 passed, actual 1 failed).The failure only appears once a project has more than one spec file, because a single entry point
inlines everything and never splits — so a suite can pass for a long time and then break when a
second spec file is added. The impact can also be silent: in a larger project on the same setup,
jsdom reported 108 of 216 tests (three spec files never loaded) in half the wall-clock of browser
mode.
What is the new behavior?
esbuild code splitting is disabled for the unit-test build only, through a new internal option
(
disableCodeSplitting) alongside the existing internal test-only options. Test bundles are neverdownloaded by a browser, so splitting has nothing to optimize here. Three small changes:
application/options.ts— declare the internal option and pass it throughnormalizeOptions.esbuild/application-code-bundle.ts— honour it increateBrowserCodeBundleOptions, followingthe existing
buildOptions.splitting = falseprecedent used for the polyfills bundle.unit-test/runners/vitest/build-options.ts— set it for the Vitest runner's build.With the change, the minimal reproduction passes (2/2) and no shared chunks are emitted for test
builds. Application builds are untouched.
Does this PR introduce a breaking change?
Cost: shared modules are duplicated into each spec bundle. Measured on a real 105-spec-file
project via
--dump-virtual-files:Total output grows ~5.7×; test run time was unchanged. If the extra memory is a concern for very
large suites, gating the option on the runner being jsdom (rather than all Vitest builds) would
narrow it, since browser mode does not exhibit the bug — happy to adjust.
Other information
Verification. Verified by applying the equivalent change to an installed 21.2.19
@angular/build(building the CLI from source with Bazel was not available on this machine), so CIshould be treated as the authoritative run:
ng build(application build, flag unset)Regression test.
unit-test/tests/behavior/vitest-shared-chunk-init_spec.tsencodes theissue's four trigger conditions: a shared
constimported by two spec entries (shared chunk), acomponent reading it in a class-field initializer, an async test callback in that component's
spec (no
awaitneeded), and zone.js in the polyfills (thesetupApplicationTargetdefault).Earlier fixture attempts passed without the fix because their test callbacks were synchronous —
zone forces async downleveling, the spec then imports the
__asynchelper, and esbuild emits thatspec entry CommonJS-wrapped with the component module behind a lazy
__esminitializer, which isthe shape in which the cross-chunk read fails.
Since Bazel wasn't runnable on my machine, the fixture's exact file contents were validated
against an installed 21.2.19
@angular/buildin a freshng newworkspace: unpatched, thecontainer spec fails with the issue's
TypeError(1 failed / 2 passed); with this changeapplied, all pass and no
chunk-*.jsis emitted. Please treat the CI run of the harness test asauthoritative. One caveat encoded as a comment in the test: the failure is sensitive to inert
content (a top-level
console.login either module defuses it), so the fixtures intentionallymirror the reproduction byte-for-byte rather than a paraphrase.
🤖 Generated with Claude Code