fix(vite-plugin-angular): emit compilation API diagnostics once with location info#2321
Conversation
✅ Deploy Preview for analog-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for analog-blog ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change fixes diagnostic duplication in the Angular compilation API plugin. Previously, global compilation diagnostics were attached to every emitted file, creating an N×M explosion (e.g., 485 warnings × 85 files = 41,087 total warnings). The fix moves diagnostics to centralized storage in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes The change is localized to a single file with straightforward diagnostic handling logic, but requires careful verification of state management: the 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
scope:vite-plugin-angular
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts (1)
645-660: Duplicated emission block — extract a tiny helper. 🧰The exact same
if (!diagnosticsEmitted) { ... }block lives here and at lines 809–817 insidetransform. With the fix above (flip-flag-first + singlethis.errorcall), it’s a one-liner worth deduplicating so the buildStart and HMR paths stay in lockstep — easy place for the two to drift otherwise.♻️ Sketch
+ function emitPendingDiagnostics(ctx: { warn: (m: string) => void; error: (m: string) => never }) { + if (diagnosticsEmitted) return; + diagnosticsEmitted = true; + for (const warning of compilationDiagnostics.warnings) ctx.warn(warning); + if (compilationDiagnostics.errors.length > 0) { + ctx.error(compilationDiagnostics.errors.join('\n')); + } + }Then both call sites become
emitPendingDiagnostics(this);.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts` around lines 645 - 660, Extract the duplicated diagnostics emission block into a small helper function (e.g., emitPendingDiagnostics) and call it from both buildStart and transform to keep behavior consistent; specifically, move the logic that checks diagnosticsEmitted and loops over compilationDiagnostics.warnings/errors (and sets diagnosticsEmitted = true) into emitPendingDiagnostics(pluginContext) and replace the inline blocks in buildStart and transform with a single call like emitPendingDiagnostics(this) so both paths share the same implementation and state updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts`:
- Around line 809-817: The diagnostics flag is set after emitting diagnostics
but PluginContext.error() throws and aborts the hook, so move diagnosticsEmitted
= true to just before emitting loops in both transform and buildStart to ensure
it is marked even if an error is thrown; instead of calling this.error() inside
the loop, collect errors from compilationDiagnostics.errors into a single
aggregated emission (or at least call this.error() once with a combined message)
while still iterating warnings via this.warn() from
compilationDiagnostics.warnings; also remove the dead checks and references to
typescriptResult.warnings and typescriptResult.errors (they are always empty) to
eliminate dead code.
- Around line 468-494: When building compilationDiagnostics (in the
diagnostics.errors and diagnostics.warnings mapping blocks) adjust the column
formatting to be 1-based for editor clickability: when composing the prefix from
loc.column use loc.column + 1 instead of loc.column (keeping the existing
null/undefined checks), so the prefix string uses `${loc.column + 1}` where
applicable; update both the errors and warnings mapping where loc and prefix are
calculated.
- Around line 521-528: Remove the per-file dead fields and their readers: delete
the errors and warnings properties from the EmitFileResult type in
packages/vite-plugin-angular/src/lib/models.ts, remove the errors: [] and
warnings: [] entries when constructing the map value in
compilation-api-plugin.ts (the outputFiles.set call that builds the file
object), and delete the corresponding branches that read those arrays in
compilation-api-plugin.ts and angular-vite-plugin.ts (the reader logic around
lines referenced in the review that inspect per-file errors/warnings). Ensure
all references to those properties are removed or refactored to use
compilationDiagnostics instead.
---
Nitpick comments:
In
`@packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts`:
- Around line 645-660: Extract the duplicated diagnostics emission block into a
small helper function (e.g., emitPendingDiagnostics) and call it from both
buildStart and transform to keep behavior consistent; specifically, move the
logic that checks diagnosticsEmitted and loops over
compilationDiagnostics.warnings/errors (and sets diagnosticsEmitted = true) into
emitPendingDiagnostics(pluginContext) and replace the inline blocks in
buildStart and transform with a single call like emitPendingDiagnostics(this) so
both paths share the same implementation and state updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d23422c-ef8c-477b-97e8-7c43d951f650
📒 Files selected for processing (1)
packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts
| compilationDiagnostics = { | ||
| errors: (diagnostics.errors || []).map( | ||
| (d: { | ||
| text?: string; | ||
| location?: { file?: string; line?: number; column?: number } | null; | ||
| }) => { | ||
| const loc = d.location; | ||
| const prefix = loc?.file | ||
| ? `${loc.file}${loc.line != null ? `:${loc.line}` : ''}${loc.column != null ? `:${loc.column}` : ''} - ` | ||
| : ''; | ||
| return `${prefix}${d.text || ''}`; | ||
| }, | ||
| ), | ||
| warnings: (diagnostics.warnings || []).map( | ||
| (d: { | ||
| text?: string; | ||
| location?: { file?: string; line?: number; column?: number } | null; | ||
| }) => { | ||
| const loc = d.location; | ||
| const prefix = loc?.file | ||
| ? `${loc.file}${loc.line != null ? `:${loc.line}` : ''}${loc.column != null ? `:${loc.column}` : ''} - ` | ||
| : ''; | ||
| return `${prefix}${d.text || ''}`; | ||
| }, | ||
| ), | ||
| }; | ||
| diagnosticsEmitted = false; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
esbuild PartialMessage Location line column 1-based 0-based convention
💡 Result:
In esbuild, the PartialMessage interface includes an optional location field of type Partial | null. The Location interface specifies that line numbers are 1-based and column numbers are 0-based (in bytes). This convention is consistently documented across official esbuild sources, including the source code types.ts and plugin API documentation.
Citations:
- 1: https://github.com/evanw/esbuild/blob/main/lib/shared/types.ts
- 2: https://esbuild.github.io/plugins/
- 3: https://github.com/evanw/esbuild/blob/88821b7e7d46737f633120f91c65f662eace0bcf/lib/shared/types.ts
🏁 Script executed:
#!/bin/bash
# Inspect how `@angular/build` surfaces location indices in diagnoseFiles output.
fd -t f -e d.ts . node_modules/@angular/build 2>/dev/null | xargs rg -nP -C3 '\bLocation\b|\bPartialMessage\b|line\??:\s*number|column\??:\s*number' 2>/dev/null | head -100
rg -nP -C3 'diagnoseFiles|PartialMessage' --type=ts -g '!**/node_modules/**'Repository: analogjs/analog
Length of output: 11822
🏁 Script executed:
# Search for `@angular/build` package imports and usage in the codebase
rg -nP '@angular/build' --type=ts -A 2 -B 2 | head -50
# Look for the type definition of what diagnoseFiles returns
rg -nP 'diagnoseFiles|PartialMessage|esbuild' packages/vite-plugin-angular/src/lib/compilation-api --type=ts -B 3 -A 3Repository: analogjs/analog
Length of output: 7669
🏁 Script executed:
# Check if there's any transformation of diagnostic location data
rg -nP 'location|\.line|\.column' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts -B 2 -A 2 | head -80Repository: analogjs/analog
Length of output: 900
🏁 Script executed:
# Check if there's any indication in package.json or imports about Angular build version
cat package.json | grep -A 5 -B 5 '@angular/build'
# Search for any test or mock data showing diagnostic format
rg -nP 'diagnoseFiles.*mockResolved|\.errors\[|\.warnings\[' packages/vite-plugin-angular --type=ts -B 2 -A 5Repository: analogjs/analog
Length of output: 2389
🏁 Script executed:
# Look for any mention of location index format in Angular or test fixtures
rg -nP 'line.*column|PartialMessage' packages/vite-plugin-angular --type=ts -B 1 -A 1 | head -40
# Check if there's documentation or comments about the format
cat packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts | sed -n '460,495p'Repository: analogjs/analog
Length of output: 4277
Column indices are 0-based in esbuild but 1-based for editor IDE clickability — apply column + 1 when formatting.
esbuild's PartialMessage.location uses 1-based line but 0-based column (in bytes). Most editors expect file:line:column with both 1-based, so the formatter should emit column + 1 to land on the correct caret position.
The column values (including 0) are correctly included by the conditional; no values are silently dropped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts`
around lines 468 - 494, When building compilationDiagnostics (in the
diagnostics.errors and diagnostics.warnings mapping blocks) adjust the column
formatting to be 1-based for editor clickability: when composing the prefix from
loc.column use loc.column + 1 instead of loc.column (keeping the existing
null/undefined checks), so the prefix string uses `${loc.column + 1}` where
applicable; update both the errors and warnings mapping where loc and prefix are
calculated.
| outputFiles.set(normalizedFilename, { | ||
| content: file.contents, | ||
| dependencies: [], | ||
| errors: errors.map((error: { text?: string }) => error.text || ''), | ||
| warnings: warnings.map( | ||
| (warning: { text?: string }) => warning.text || '', | ||
| ), | ||
| errors: [], | ||
| warnings: [], | ||
| hmrUpdateCode: templateUpdate?.code, | ||
| hmrEligible: !!templateUpdate?.code, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm there are no remaining producers/consumers of per-file errors/warnings beyond this file.
rg -nP -C2 '\b(errors|warnings)\s*:\s*\[\s*\]' --type=ts
rg -nP -C2 '\btypescriptResult\.(errors|warnings)\b' --type=ts
fd -t f 'models.ts' packages/vite-plugin-angular --exec rg -nP -C2 '(errors|warnings)\??:\s*string\[\]' {}Repository: analogjs/analog
Length of output: 6723
🏁 Script executed:
fd -t f 'models.ts' packages/vite-plugin-angular --exec cat -n {} \;Repository: analogjs/analog
Length of output: 475
🏁 Script executed:
sed -n '520,540p' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.tsRepository: analogjs/analog
Length of output: 645
🏁 Script executed:
sed -n '820,845p' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.tsRepository: analogjs/analog
Length of output: 1078
🏁 Script executed:
rg -n 'typescriptResult\s*=|function.*typescriptResult|const.*typescriptResult' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts -A 3Repository: analogjs/analog
Length of output: 261
🏁 Script executed:
rg -n '\berrors:\s*\[' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.tsRepository: analogjs/analog
Length of output: 103
🏁 Script executed:
rg 'fileEmitter' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts -B 2 -A 10 | head -60Repository: analogjs/analog
Length of output: 1303
🏁 Script executed:
# Search for where errors/warnings might be populated in EmitFileResult
rg -n 'errors\s*:\s*(?!\[\])' packages/vite-plugin-angular/src/lib/compilation-api/ -t tsRepository: analogjs/analog
Length of output: 297
🏁 Script executed:
rg -n 'errors\s*:' packages/vite-plugin-angular/src/lib/compilation-api/ -t ts | grep -v 'errors: \[\]'Repository: analogjs/analog
Length of output: 507
🏁 Script executed:
rg -n 'warnings\s*:' packages/vite-plugin-angular/src/lib/compilation-api/ -t ts | grep -v 'warnings: \[\]'Repository: analogjs/analog
Length of output: 350
🏁 Script executed:
# Check what the actual type of fileEmitter is and where outputFiles entries come from
sed -n '515,530p' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.tsRepository: analogjs/analog
Length of output: 491
🏁 Script executed:
# Check all places where outputFiles.set is called
rg -n 'outputFiles\.set' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts -B 2 -A 8Repository: analogjs/analog
Length of output: 362
🏁 Script executed:
# Verify if there are any other EmitFileResult creations with non-empty errors/warnings
rg -n '(errors|warnings):\s*\(' packages/vite-plugin-angular/src/lib/compilation-api/ -t ts -A 1Repository: analogjs/analog
Length of output: 518
🏁 Script executed:
# Check angular-vite-plugin.ts lines 1428-1436 to see if it also reads from the same source
sed -n '1420,1450p' packages/vite-plugin-angular/src/lib/angular-vite-plugin.tsRepository: analogjs/analog
Length of output: 1077
🏁 Script executed:
# Understand the full context: what does fileEmitter return and why are these fields always empty?
sed -n '465,490p' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.tsRepository: analogjs/analog
Length of output: 1011
🏁 Script executed:
# Final check: ensure no other code path populates per-file errors/warnings with actual values
rg -n '\berrors\s*:\s*\[.*\]' packages/vite-plugin-angular/src/lib/ -t ts | grep -v 'errors: \[\]'Repository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Check if EmitFileResult is used elsewhere and whether removing these fields would break anything
rg -n 'EmitFileResult' packages/vite-plugin-angular/src/lib/ -B 2 -A 2Repository: analogjs/analog
Length of output: 3833
🏁 Script executed:
# Final verification: any other readers of errors/warnings from EmitFileResult
rg 'typescriptResult\.(errors|warnings)' packages/vite-plugin-angular/src/lib/ -t tsRepository: analogjs/analog
Length of output: 1508
🏁 Script executed:
# Check if there's any destructuring or spread that might read these fields
rg '\.\.\.*typescriptResult' packages/vite-plugin-angular/src/lib/ -t tsRepository: analogjs/analog
Length of output: 41
🏁 Script executed:
# Check for any comments or TODOs around the EmitFileResult definition or per-file diagnostics
sed -n '1,15p' packages/vite-plugin-angular/src/lib/models.tsRepository: analogjs/analog
Length of output: 391
🏁 Script executed:
# Check context around where outputFiles entries are created to understand the design
sed -n '500,530p' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.tsRepository: analogjs/analog
Length of output: 996
🏁 Script executed:
# Look for any git history or comments explaining the design around compilationDiagnostics vs per-file diagnostics
rg -n 'compilationDiagnostics|per-file' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts -B 1 -A 1Repository: analogjs/analog
Length of output: 811
Per-file errors/warnings are dead code — remove them. 🧹
The errors: [] and warnings: [] fields on every emitted file entry (lines 524–525) are write-only. Diagnostics are centralized in compilationDiagnostics, and the reader code at lines 831–837 will never find a non-empty array. Similar dead readers exist in angular-vite-plugin.ts (lines 1428–1436, 1443–1444).
Remove the fields from EmitFileResult in packages/vite-plugin-angular/src/lib/models.ts and delete the corresponding reader branches from both compilation-api-plugin.ts and angular-vite-plugin.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts`
around lines 521 - 528, Remove the per-file dead fields and their readers:
delete the errors and warnings properties from the EmitFileResult type in
packages/vite-plugin-angular/src/lib/models.ts, remove the errors: [] and
warnings: [] entries when constructing the map value in
compilation-api-plugin.ts (the outputFiles.set call that builds the file
object), and delete the corresponding branches that read those arrays in
compilation-api-plugin.ts and angular-vite-plugin.ts (the reader logic around
lines referenced in the review that inspect per-file errors/warnings). Ensure
all references to those properties are removed or refactored to use
compilationDiagnostics instead.
| if (!diagnosticsEmitted) { | ||
| for (const warning of compilationDiagnostics.warnings) { | ||
| this.warn(warning); | ||
| } | ||
| for (const error of compilationDiagnostics.errors) { | ||
| this.error(error); | ||
| } | ||
| diagnosticsEmitted = true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rollup/Vite plugin context, does PluginContext.error() throw and abort the hook? What is the recommended way to surface multiple compilation errors from a single hook?
💡 Result:
Yes. In Rollup/Vite plugin hooks, calling PluginContext.error(...) throws (i.e., it aborts the hook immediately by throwing an error). Recommended way to surface multiple compilation errors from a single hook: - Collect all diagnostics inside the hook. - Throw a single error that represents the set, e.g. an AggregateError (or an error with a .errors array), so Vite/Rollup fail the hook once while still preserving multiple underlying errors. - In Vite, when you throw an AggregateError from a hook, you may need to read/iterate the nested errors (Vite may display only the top-level AggregateError message in some cases). Rationale / evidence: - Rollup’s PluginContext.error(error_) is implemented to return error(logPluginError(...)) where the return type is never, and in practice it throws an error and stops hook execution. - Vite/Rollup hook execution is designed such that throwing ends the current hook action. - For multiple errors, Vite users have thrown an AggregateError from a single buildApp hook and reported the behavior (Vite printed only the AggregateError message, so you should ensure the underlying errors are accessible via the thrown error’s .errors/.message).
Citations:
- 1: https://github.com/rollup/rollup/blob/fe50d862804c1f157f0ae378a3c555b8b3ef6ce7/src/utils/PluginContext.ts
- 2: https://github.com/rollup/rollup/blob/fe50d862804c1f157f0ae378a3c555b8b3ef6ce7/src/utils/hookActions.ts
- 3: Cannot show AggreateError nicely when there is a build error vitejs/vite#21666
- 4: Using
this.erroroutside the transformation context rollup/rollup#4424
🏁 Script executed:
# Check the file size first
wc -l packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.tsRepository: analogjs/analog
Length of output: 142
🏁 Script executed:
# Read the critical sections to verify the issue
cat -n packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts | sed -n '450,530p'Repository: analogjs/analog
Length of output: 3221
🏁 Script executed:
# Read the buildStart emission section
cat -n packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts | sed -n '640,670p'Repository: analogjs/analog
Length of output: 1184
🏁 Script executed:
# Read the transform emission section
cat -n packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts | sed -n '800,840p'Repository: analogjs/analog
Length of output: 1610
🏁 Script executed:
# Check where diagnosticsEmitted is declared
rg -n "diagnosticsEmitted" packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.tsRepository: analogjs/analog
Length of output: 289
Move diagnosticsEmitted = true before the loops to prevent re-emission when this.error() throws.
PluginContext.error() throws and aborts the hook immediately. In both buildStart (lines 652–660) and transform (lines 809–817), if an error exists in the loop, the throw prevents diagnosticsEmitted = true from executing. In buildStart this is one-shot; in transform, every subsequent file's hook invocation sees the flag still false and re-throws the same first error — reintroducing the N×M duplication this PR aimed to fix. Set the flag before entering the loops, and collect all errors to emit in a single call so users see the full error set instead of aborting on the first one.
Suggested approach (both 652–660 and 809–817)
if (!diagnosticsEmitted) {
+ diagnosticsEmitted = true;
for (const warning of compilationDiagnostics.warnings) {
this.warn(warning);
}
- for (const error of compilationDiagnostics.errors) {
- this.error(error);
+ if (compilationDiagnostics.errors.length > 0) {
+ this.error(compilationDiagnostics.errors.join('\n'));
}
- diagnosticsEmitted = true;
}Also remove lines 831–837: typescriptResult.warnings and typescriptResult.errors are always empty arrays (per lines 524–525), so these checks are dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts`
around lines 809 - 817, The diagnostics flag is set after emitting diagnostics
but PluginContext.error() throws and aborts the hook, so move diagnosticsEmitted
= true to just before emitting loops in both transform and buildStart to ensure
it is marked even if an error is thrown; instead of calling this.error() inside
the loop, collect errors from compilationDiagnostics.errors into a single
aggregated emission (or at least call this.error() once with a combined message)
while still iterating warnings via this.warn() from
compilationDiagnostics.warnings; also remove the dead checks and references to
typescriptResult.warnings and typescriptResult.errors (they are always empty) to
eliminate dead code.
…location info Previously, performAngularCompilation() attached all global diagnostics to every emitted file in the outputFiles map. The transform hook then emitted each file's warnings via this.warn(), causing an N×M explosion (e.g. 485 warnings × 85 files = 41,087 total warnings). Additionally, the PartialMessage objects from diagnoseFiles() contain rich location info (file, line, column) but only warning.text was kept, discarding all location context and making it impossible to locate the source of each diagnostic. This fix: - Stores diagnostics at the module level instead of per-file - Emits warnings individually and errors as a single combined message in buildStart (initial build) and in the transform hook (HMR recompilations) using a diagnosticsEmitted flag - Includes file:line:column location info in diagnostic messages Closes analogjs#2317 Signed-off-by: tomer953 <tomer953@gmail.com>
tomer953
force-pushed
the
fix/vite-plugin-angular-compilation-api-diagnostics
branch
from
7338612 to
365e55b
Compare
1 participant
Previously, performAngularCompilation() attached all global diagnostics to every emitted file in the outputFiles map. The transform hook then emitted each file's warnings via this.warn(), causing an N×M explosion (e.g. 485 warnings × 85 files = 41,087 total warnings).
Additionally, the PartialMessage objects from diagnoseFiles() contain rich location info (file, line, column) but only warning.text was kept, discarding all location context and making it impossible to locate the source of each diagnostic.
This fix:
PR Checklist
Closes #2317
Affected scope
Recommended merge strategy for maintainer [optional]
Commit preservation note [optional]
What is the new behavior?
Test plan
nx format:checkpnpm buildpnpm testDoes this PR introduce a breaking change?
Other information
[optional] What gif best describes this PR or how it makes you feel?