Repository navigation
fix(@angular/build): improve library bundler resolution and watch shared chunk consistency - #34440
alan-agius4 wants to merge 1 commit into
Conversation
alan-agius4
added
action: review
![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 enhances the library bundler pipeline to maintain shared chunk consistency during watch mode rebuilds and improves path resolution in the memory file loader plugin, including handling trailing slashes, Windows drive-letter paths, and preferring .d.mts for .mjs specifiers. A high-severity issue was identified where partial rebuilds that only trigger ESM or DTS changes will fail to update bundleResults due to a strict check requiring both bundles to be present, which can lead to stale file cleanup issues or unnecessary full rebuilds.
…red chunk consistency - Strip trailing slashes when resolving relative directory imports in `createMemoryFileLoaderPlugin` so `./dir/` resolves `.../dir/index.js`. - Support `.cjs` and `.d.cts` extensions, and prioritize `.d.mts`/`.d.cts` before `.d.ts` when resolving explicit `.mjs`/`.cjs` specifiers in `.d.ts` mode. - Treat Windows drive-letter absolute paths (`/^[a-zA-Z]:\//`) as internal modules. - Fall back to on-disk declaration files in `.d.ts` mode for hand-authored ambient/declaration files not emitted into the in-memory `dtsFiles` map. - Re-bundle all entry points from in-memory outputs during partial watch rebuilds when previous bundle results contained shared chunks to prevent inlining shared code into a subset of entry points.
82efdc3 to
4e8c365
Compare
| /** | ||
| * Checks whether two sets share at least one common element. | ||
| */ | ||
| function hasSetIntersection(a: ReadonlySet<string>, b: ReadonlySet<string>): boolean { |
There was a problem hiding this comment.
| continue; | ||
| } | ||
| } else { | ||
| selectedNames ??= new Set(selectedEntryPoints.map((item) => item.name)); |
There was a problem hiding this comment.
Consider: Could we just initialize on line 137 instead of using ??=?
const selectedNames = new Set<string>();| const singleSelectedName = | ||
| selectedEntryPoints.length === 1 ? selectedEntryPoints[0].name : undefined; |
There was a problem hiding this comment.
Question: Why are we special casing singleSelectedName?
If I'm following this correctly, I think the below while loop does nothing in that case? Could we just return early or let this be naturally handled as an array that just happens to have one item in it?
| if (selectedEntryPoints.length >= allEntryPoints.length) { | ||
| return selectedEntryPoints; | ||
| } |
There was a problem hiding this comment.
Question: In what practical situation do we have more selected entry points than all entry points? Should that be an assertion error?
| * Expands the selected entry points to include any available entry points whose module IDs | ||
| * transitively overlap with the selected set. | ||
| */ | ||
| function expandOverlappingEntryPoints( |
There was a problem hiding this comment.
Consider: TBH, this function implementation has kinda lost me. IIUC, the intent is to figure out all entry points which share a module ID with the given selected entry points?
Is that fundamentally just an Object.groupBy call? Is there a way we can simplify to something closer to:
return Object.groupBy(selectedEntryPoints, getModuleIds).values();I suspect that wouldn't exactly work because each entry point has multiple module IDs, but I'm wondering if conceptually it's a cleaner direction?
| getPreviousModuleIds: (ep: NormalizedEntryPoint) => ReadonlySet<string> | undefined, | ||
| ): boolean { | ||
| for (const ep of bundledEntryPoints) { | ||
| const newIds = output.bundles.get(ep.bundleName)?.moduleIds; |
There was a problem hiding this comment.
Nit: We should always expect to get a bundle right? Would it be more clear to do output.bundles.get(ep.bundleName)!.moduleIds and then drop the subsequent if (!newIds) style checks? Similarly, do we really care about skipping .size === 0 cases or will that no-op on its own?
3 participants
Summary
Fixes module resolution issues in
createMemoryFileLoaderPluginand ensures shared chunk consistency during partial watch-mode rebuilds in the@angular/build:librarybundler pipeline:import './dir/') before probing candidate extensions so.../dir/index.jsis resolved instead of.../dir//index.js..mjs->.d.mtsdeclaration preference: In.d.tsbundling mode, prefer.d.mtswhen an explicit.mjsspecifier is used and both.d.tsand.d.mtsexist.C:/...) as internal modules inresolveIdrather than marking them as external.fesm2022/shared-*.mjsortypes/shared-*.d.ts), expand the Rolldown invocation to include all entry points present in the in-memoryesmFiles/dtsFilesmaps and updatebundleResultsfor all bundled entry points so shared chunk hashes and stale file cleanup remain consistent.