Skip to content

fix(vite-plugin-angular): emit compilation API diagnostics once with location info #2321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
tomer953 wants to merge 1 commit into
base: alpha
Choose a base branch
from
Draft

fix(vite-plugin-angular): emit compilation API diagnostics once with location info #2321

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 54 additions & 6 deletions packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,11 @@ export function compilationAPIPlugin(
let compilationLock = Promise.resolve();
let pendingCompilation: Promise<void> | null = null;
let initialCompilation = false;
let compilationDiagnostics: { errors: string[]; warnings: string[] } = {
errors: [],
warnings: [],
};
let diagnosticsEmitted = false;
let viteServer: ViteDevServer | undefined;

const isTest = process.env['NODE_ENV'] === 'test' || !!process.env['VITEST'];
Expand Down Expand Up @@ -460,8 +465,33 @@ export function compilationAPIPlugin(
: DiagnosticModes.All,
);

const errors = diagnostics.errors?.length ? diagnostics.errors : [];
const warnings = diagnostics.warnings?.length ? diagnostics.warnings : [];
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;
Comment on lines +468 to +494
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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:

🏁 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 3

Repository: 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 -80

Repository: 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 5

Repository: 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.


const templateUpdates = mapTemplateUpdatesToFiles(
compilationResult.templateUpdates,
Expand Down Expand Up @@ -491,10 +521,8 @@ export function compilationAPIPlugin(
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,
});
Comment on lines 521 to 528
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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.ts

Repository: analogjs/analog

Length of output: 645

🏁 Script executed:

sed -n '820,845p' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts

Repository: 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 3

Repository: analogjs/analog

Length of output: 261

🏁 Script executed:

rg -n '\berrors:\s*\[' packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts

Repository: 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 -60

Repository: 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 ts

Repository: 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.ts

Repository: 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 8

Repository: 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 1

Repository: 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.ts

Repository: 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.ts

Repository: 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 2

Repository: 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 ts

Repository: 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 ts

Repository: 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.ts

Repository: 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.ts

Repository: 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 1

Repository: 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.

Expand Down Expand Up @@ -620,6 +648,16 @@ export function compilationAPIPlugin(
pendingCompilation = null;
initialCompilation = true;
}

if (!diagnosticsEmitted) {
for (const warning of compilationDiagnostics.warnings) {
this.warn(warning);
}
if (compilationDiagnostics.errors.length > 0) {
this.error(compilationDiagnostics.errors.join('\n'));
}
diagnosticsEmitted = true;
}
},
async handleHotUpdate(ctx) {
if (isIgnoredHmrFile(ctx.file)) {
Expand Down Expand Up @@ -768,6 +806,16 @@ export function compilationAPIPlugin(
pendingCompilation = null;
}

if (!diagnosticsEmitted) {
for (const warning of compilationDiagnostics.warnings) {
this.warn(warning);
}
if (compilationDiagnostics.errors.length > 0) {
this.error(compilationDiagnostics.errors.join('\n'));
}
diagnosticsEmitted = true;
}
Comment on lines +809 to +817
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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:

🏁 Script executed:

# Check the file size first
wc -l packages/vite-plugin-angular/src/lib/compilation-api/compilation-api-plugin.ts

Repository: 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.ts

Repository: 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.


const typescriptResult = fileEmitter(id);
if (!typescriptResult) {
debugCompilationApi('transform skip (file not emitted)', { id });
Expand Down
Loading