fix(@angular/build): disable code splitting for unit test builds - #33729
Conversation
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
clydin
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
This appears to be the cleanest approach to ensure resilience with Vitest's synthetic module runner.
Sorry, something went wrong.
|
This PR was merged into the repository. The changes were merged into the following branches: |
Sorry, something went wrong.
Fixes #33728
PR Checklist
Please check to confirm your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #33728
Under @angular/build:unit-test with the default (jsdom) runner, an export from a module that code
splitting hoisted into a shared chunk can be observed as undefined by an importing chunk — a
component reading an exported const in a class-field initializer fails with
TypeError: Cannot read properties of undefined (reading 'map'), in a test that never touches the
runner'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 __esm initializers; importing chunks then read the export as
a 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 async test callback + zone.js polyfills) isolated by single-variable toggles — is
in #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 never
downloaded by a browser, so splitting has nothing to optimize here. Three small changes:
the existing buildOptions.splitting = false precedent used for the polyfills bundle.
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 CI
should be treated as the authoritative run:
Regression test. unit-test/tests/behavior/vitest-shared-chunk-init_spec.ts encodes the
issue's four trigger conditions: a shared const imported by two spec entries (shared chunk), a
component reading it in a class-field initializer, an async test callback in that component's
spec (no await needed), and zone.js in the polyfills (the setupApplicationTarget default).
Earlier fixture attempts passed without the fix because their test callbacks were synchronous —
zone forces async downleveling, the spec then imports the __async helper, and esbuild emits that
spec entry CommonJS-wrapped with the component module behind a lazy __esm initializer, which is
the 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/build in a fresh ng new workspace: unpatched, the
container spec fails with the issue's TypeError (1 failed / 2 passed); with this change
applied, all pass and no chunk-*.js is emitted. Please treat the CI run of the harness test as
authoritative. One caveat encoded as a comment in the test: the failure is sensitive to inert
content (a top-level console.log in either module defuses it), so the fixtures intentionally
mirror the reproduction byte-for-byte rather than a paraphrase.
🤖 Generated with Claude Code