Repository navigation
perf(@angular/build): overlap library asset resolution with bundling and typechecking - #34424
alan-agius4 wants to merge 2 commits into
Conversation
b0921fa to
b57c01f
Compare
…and typechecking - Run `buildState.collectAssets(options, modifiedFiles)` inside `Promise.all` alongside `bundleEntryPoints` and `diagnosePromise` so filesystem asset resolution overlaps with Rolldown bundling and worker-thread typecheck diagnostics. - Delete stale files with `deleteFilesFromDisk` before emitting updated files with `emitFilesToDisk` so case-only renames on case-insensitive filesystems do not remove newly emitted files. - Check `result.hasErrors` before adding `bundleOutput.warnings` so secondary bundler warnings are not logged when compilation fails. - Include `options.packageJsonPath` in the disallowed dependency error message in `verifyAllowedDependencies`.
b57c01f to
31619b8
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the library builder pipeline to optimize asset collection and file emission. Specifically, it parallelizes asset collection with bundling, defers file emission until after obsolete files are deleted, and refines warning logs to avoid outputting secondary bundler warnings when build errors occur. Additionally, dependency verification error messages now dynamically reference the correct package.json path instead of a hardcoded string, and corresponding tests have been updated to reflect these changes. There are no review comments, so no further feedback is provided.
| if (result.hasErrors) { | ||
| return result; | ||
| } | ||
| result.addWarnings(bundleOutput.warnings); |
There was a problem hiding this comment.
Question: I'm not following why we're moving warnings after the hasErrors check. Would we not want to include warnings when a fatal error exists? This seems useful for additional context / reducing the need for multiple builds to discover all the problems?
There was a problem hiding this comment.
Good catch. The thought here was originally to avoid noisy secondary bundler warnings from Rolldown when bundling code that has primary TypeScript compilation errors, similar to how we handle secondary bundler failures in the catch block above.
However, in the application builder we do keep and display bundler warnings alongside errors. Suppressing them here also hides legitimate warnings such as eval usage or packaging issues, which means having to do multiple build rounds to discover them as you pointed out.
Note that TypeScript and Angular compiler warnings were never suppressed by this check since those are collected earlier in diagnosePromise. I have reverted this change so bundler warnings are preserved alongside errors to match the application builder.
…ndling and typechecking
collectedAssets ?? buildState.collectAssets(options, modifiedFiles)insidePromise.allalongsidebundleEntryPointsanddiagnosePromise, overlappinglstatandtinyglobbyasset resolution with Rolldown's native Rust bundling threads (bundleAllEsm/bundleAllDts) and worker-thread diagnostics (diagnosePromise).deleteFilesFromDiskbefore emitting updated files withemitFilesToDiskso case-only renames on case-insensitive filesystems never delete newly emitted files.if (result.hasErrors) { return result; }beforeresult.addWarnings(bundleOutput.warnings)to suppress secondary bundler warnings when TypeScript/Angular compilation has errors.options.packageJsonPathin the disallowed dependency error message inverifyAllowedDependencies.