fix(vite-plugin-angular): signal-API parity in fast-compile AOT transform#2346
Merged
Conversation
The fast-compile AOT path's model branch built the input descriptor with
{classPropertyName, bindingPropertyName, isSignal: true} only, dropping
the required flag returned by getCallApi. The downstream merge in
compile.ts then read `sigDesc.required || false` and registered
model.required() as a non-required input.
The signal itself still throws at runtime when read uninitialized, but
the template type-checker (which consumes the partial-mode
isRequired metadata in .d.ts declarations) never flagged the missing
binding at compile time. Consumers only discovered unbound required
models at runtime when the model signal was first read.
Set `required` on the model input descriptor so partial-mode output
includes isRequired: true and the template type-checker rejects parent
templates that omit the binding.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
detectSignals captured the user's transform function as a WrappedNodeExpr and placed it on the input descriptor. compile.ts then forwarded it as transformFunction on the R3DirectiveMetadata. Angular's emitter sets HasDecoratorInputTransform on the input flags and pushes the expression into the directive definition's input array, so the runtime input setter applies it a second time on every write — on top of the transform the input signal already runs internally. Most built-in transforms (numberAttribute, booleanAttribute) are idempotent so this rarely surfaced, but user-defined transforms like `v => v + 1` produced wrong results on the second pass. Emit `transform: null` for all signal inputs so the directive metadata matches Angular's reference (input_function.ts:74) and the runtime relies on the signal's own internal transform. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…[1] in aot
detectSignals read options from args[0] for both output and
outputFromObservable. The first positional argument of
outputFromObservable is the source observable, not the options object,
so aliases on outputFromObservable(stream, { alias: 'foo' }) silently
vanished and the output was registered under the class property name.
Templates binding (foo) silently failed to match.
Read options from args[1] when the API is outputFromObservable to match
upstream output_function.ts:81.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ecorator is on the field detectSignals ran independently of detectFieldDecorators, and compile.ts merged the two results with last-wins for inputs/outputs and array-concat for queries. When a field carried both an explicit @Input/@Output/ @ViewChild and a signal initializer: - Inputs/outputs: the signal entry silently overwrote the decorator's alias/required/transform. - Queries: the same query was registered twice; the runtime collected both and clobbered `this.field` in registration order, and template ɵɵviewQuery/ɵɵcontentQuery instructions fired duplicated. Mirror Angular's transform behavior (input_function.ts:42-48 et al.) and the JIT fix from the prior PR: skip the signal-synthesis branch when an overlapping FIELD_DECORATORS entry is present on the same member. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-API metadata Companion to jit-parity.spec.ts for the fast-compile AOT path. Catches the same five gap classes by capturing the R3DirectiveMetadata object Analog hands to Angular's compiler and deep-equaling against a reference oracle derived from the upstream initializer-API transforms. Partial mode is used (not full mode) because its ɵɵngDeclareComponent argument exposes isRequired, transformFunction, query descriptors, and the outputs map as structured object literals — full-mode ɵɵdefineComponent packs inputs into positional arrays that lose required and transformFunction. Partial mode is therefore the only emit path where every AOT gap fixed by this branch is observable. The harness: 1. Runs source through compilePartialCode. 2. Replaces the @angular/core imports with stubs that capture each ɵɵngDeclareComponent / ɵɵngDeclareDirective argument. 3. Normalizes the captured metadata to a structural shape (unwrapping absent-as-default fields like contentChildren's descendants). 4. Deep-equals against an oracle pinned to upstream source files. 18 cases cover input/model/output/outputFromObservable/queries and decorator-coexistence — each AOT gap fixed earlier in the branch maps to a focused toEqual assertion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
✅ Deploy Preview for analog-docs 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. |
✅ Deploy Preview for analog-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR extends the Angular signal API metadata extraction to respect existing field decorators and adds comprehensive test coverage. The core implementation in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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.
🧹 Nitpick comments (2)
packages/vite-plugin-angular/src/lib/compiler/partial.spec.ts (1)
158-215: ⚡ Quick winRun
expectCompilesin these new partial-mode cases.These tests only match substrings right now. If the partial output becomes malformed while still containing the expected metadata literals, they'll still pass.
Minimal hardening
it('emits isRequired: true for input.required() and false for input()', () => { const result = compilePartial( ` import { Component, input } from '`@angular/core`'; `@Component`({ selector: 'x', template: '' }) export class X { a = input<string>(); b = input.required<number>(); } `, 'inputs.ts', ); + expectCompiles(result); expect(result).toMatch(/a:\s*\{[^}]*isRequired:\s*false/); expect(result).toMatch(/b:\s*\{[^}]*isRequired:\s*true/); }); @@ const result = compilePartial( ` import { Component, input, numberAttribute } from '`@angular/core`'; `@Component`({ selector: 'x', template: '' }) export class X { size = input(0, { transform: numberAttribute }); } `, 'transform.ts', ); + expectCompiles(result); expect(result).toMatch(/size:\s*\{[^}]*transformFunction:\s*null/); expect(result).not.toMatch( /size:\s*\{[^}]*transformFunction:\s*numberAttribute/, ); @@ const result = compilePartial( ` import { Component, model } from '`@angular/core`'; `@Component`({ selector: 'x', template: '' }) export class X { c = model<string>(); d = model.required<number>(); } `, 'models.ts', ); + expectCompiles(result); expect(result).toMatch(/c:\s*\{[^}]*isRequired:\s*false/); expect(result).toMatch(/d:\s*\{[^}]*isRequired:\s*true/); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vite-plugin-angular/src/lib/compiler/partial.spec.ts` around lines 158 - 215, The tests in the "signal API metadata" suite only assert substrings from compilePartial output; call expectCompiles on the partial output to ensure the partial-mode compilation actually succeeded. For each it block that calls compilePartial (the ones using compilePartial in 'inputs.ts', 'transform.ts', and 'models.ts'), add a call like expectCompiles(result) immediately after const result = compilePartial(...); and before the regex assertions so that compilePartial and its result are validated end-to-end.packages/vite-plugin-angular/src/lib/compiler/aot-parity.spec.ts (1)
415-451: ⚡ Quick winAdd parity cases for the new
@Outputand content-query suppression paths.
detectSignals()now skips synthesis when the field already has@Output,@ContentChild, or@ContentChildren, but this harness only exercises the@Inputand@ViewChildbranches. A regression in those new paths would still slip through the parity suite.Suggested coverage additions
describe('decorator + signal coexistence', () => { + it('`@Output` on a field with output() — decorator alias wins', () => { + const meta = reflectAot(` + import { Component, Output, output } from '`@angular/core`'; + `@Component`({ selector: 'x', template: '' }) + export class X { `@Output`('decoratorPub') ready = output({ alias: 'signalPub' }); } + `); + expect(meta.outputs).toEqual({ ready: 'decoratorPub' }); + }); + + it('`@ContentChild` on a field with contentChild() — only one query entry', () => { + const meta = reflectAot(` + import { Component, ContentChild, contentChild } from '`@angular/core`'; + `@Component`({ selector: 'x', template: '<ng-content></ng-content>' }) + export class X { `@ContentChild`('slot') slot = contentChild('slot'); } + `); + expect(meta.queries).toHaveLength(1); + expect(meta.queries[0].isSignal).toBe(false); + }); + // Would have caught: AOT duplicate query registration. it('`@ViewChild` on a field with viewChild() — only one query entry', () => {As per coding guidelines
**/*.{test,spec}.{ts,tsx}: Include tests which validate behavior for any new functionality added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vite-plugin-angular/src/lib/compiler/aot-parity.spec.ts` around lines 415 - 451, Add tests mirroring the existing decorator+signal cases for the new suppression paths: write reflectAot-based specs that assert when a field has an explicit `@Output`() with output() the decorator entry wins (meta.outputs.<name>.isSignal is false and no extra synthetic change output is created), and when a field has `@ContentChild`() or `@ContentChildren`() with contentChild()/contentChildren() the decorator-derived content query entries (meta.contentQueries or equivalent) are present and not marked as signal-derived; place these alongside the existing decorator+signal coexistence tests and reuse the same pattern/assertions as the `@Input` and `@ViewChild` tests to ensure detectSignals() skipping behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/vite-plugin-angular/src/lib/compiler/aot-parity.spec.ts`:
- Around line 415-451: Add tests mirroring the existing decorator+signal cases
for the new suppression paths: write reflectAot-based specs that assert when a
field has an explicit `@Output`() with output() the decorator entry wins
(meta.outputs.<name>.isSignal is false and no extra synthetic change output is
created), and when a field has `@ContentChild`() or `@ContentChildren`() with
contentChild()/contentChildren() the decorator-derived content query entries
(meta.contentQueries or equivalent) are present and not marked as
signal-derived; place these alongside the existing decorator+signal coexistence
tests and reuse the same pattern/assertions as the `@Input` and `@ViewChild` tests
to ensure detectSignals() skipping behavior is covered.
In `@packages/vite-plugin-angular/src/lib/compiler/partial.spec.ts`:
- Around line 158-215: The tests in the "signal API metadata" suite only assert
substrings from compilePartial output; call expectCompiles on the partial output
to ensure the partial-mode compilation actually succeeded. For each it block
that calls compilePartial (the ones using compilePartial in 'inputs.ts',
'transform.ts', and 'models.ts'), add a call like expectCompiles(result)
immediately after const result = compilePartial(...); and before the regex
assertions so that compilePartial and its result are validated end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c08937f-f3bf-4499-9488-96359005000e
📒 Files selected for processing (4)
packages/vite-plugin-angular/src/lib/compiler/aot-parity.spec.tspackages/vite-plugin-angular/src/lib/compiler/component.spec.tspackages/vite-plugin-angular/src/lib/compiler/metadata.tspackages/vite-plugin-angular/src/lib/compiler/partial.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
Follow-up to signal-API parity in fast-compile JIT transform analogjs/analog#2345. The post-merge audit on that PR identified four parallel gaps in the fast-compile AOT path (
metadata.ts+compile.ts) plus the differential-test infrastructure missing from the AOT side. This PR closes them with the same per-gap commit structure and adds an AOT parity harness alongside the JIT one.Closes #
Affected scope
vite-plugin-angularRecommended merge strategy for maintainer [optional]
Commit preservation note [optional]
Five focused commits — four fixes plus the parity harness — each independently reviewable and bisectable. Same rationale as #2345: each commit documents one specific gap and stands alone. Happy to switch to squash if maintainers prefer.
What is the new behavior?
The fast-compile AOT path's
detectSignals(inpackages/vite-plugin-angular/src/lib/compiler/metadata.ts) and the downstream merge incompile.tsnow match Angular's official initializer-API transforms (@angular/compiler-cli/src/ngtsc/annotations/directive/src/{input,model,output,query}_function.ts) for every signal API. The five commits, in order:propagate required through aot model metadata— the model branch built the input descriptor with{classPropertyName, bindingPropertyName, isSignal: true}only, dropping therequiredflag returned bygetCallApi.compile.ts:684then readsigDesc.required || falseand registeredmodel.required()as a non-required input.Important nuance: this is less severe than a naive reading of the audit would suggest. The model signal itself enforces required-ness at runtime —
model.required()constructs a signal that throws on uninitialized read, so an unbound required model still throws eventually. The actual user-facing breakage is template type-checker silence at compile time: withoutisRequired: truein the partial-mode declaration metadata, the template type-checker doesn't flag parent templates that omit the binding, and the consumer only discovers the bug at runtime when the signal is first read. The fix restores the compile-time gate — which is what makes required inputs useful in the first place.drop transform from aot input metadata—detectSignalscaptured the user'stransform: fnas aWrappedNodeExprand placed it on the input descriptor.compile.tsforwarded it astransformFunctionon theR3DirectiveMetadata. Angular's emitter then setHasDecoratorInputTransformon the input flags and pushed the expression into the directive definition's input array, so the runtime input setter applied the transform a second time on every write — on top of the transform the input signal already runs internally. Idempotent transforms (numberAttribute,booleanAttribute) rarely surfaced this; user-defined transforms likev => v + 1produced wrong results on the second pass.read outputFromObservable options from args[1] in aot—detectSignalsread options fromargs[0]for bothoutputandoutputFromObservable. The first positional argument ofoutputFromObservableis the source observable, not the options object, so aliases onoutputFromObservable(stream, { alias: 'foo' })silently vanished and the output registered under the class property name. Templates binding(foo)="..."silently failed to match.skip aot signal synthesis when a matching decorator is on the field—detectSignalsran independently ofdetectFieldDecoratorsandcompile.tsmerged the two with last-wins for inputs/outputs (line 670-687) and array-concat for queries (line 721-722). When a field carried both an explicit@Input/@Output/@ViewChildand a signal initializer:alias/required/transform.this.fieldin registration order, and the template'sɵɵviewQuery/ɵɵcontentQueryinstructions fired duplicated.differential parity harness for aot signal-API metadata— companion tojit-parity.spec.tsintroduced in fix(vite-plugin-angular): signal-API parity in fast-compile JIT transform #2345. Catches the same five gap classes by capturing theR3DirectiveMetadataobject Analog hands to Angular's compiler and deep-equaling against a reference oracle derived from the upstream initializer-API transforms.Partial mode is used (not full mode) because its
ɵɵngDeclareComponentargument exposesisRequired,transformFunction, query descriptors, and the outputs map as structured object literals. Full-modeɵɵdefineComponentpacks inputs into positional arrays that loserequiredandtransformFunction(Angular only carries those in.d.tstype metadata or the partial declaration), so partial mode is the only emit path where every AOT gap is observable.18 cases cover input/model/output/outputFromObservable/queries and decorator-coexistence — each AOT gap fixed earlier in the branch maps to a focused
toEqualassertion.Test plan
nx format:check— all touched files pass; repo-wide check still fails only on pre-existing unformatted files insidepackages/**/dist/(untracked build output).nx build vite-plugin-angular— clean build, no type errors.nx test vite-plugin-angular— 1152 passed | 23 skipped (1175 total). Each fix commit adds focused unit tests inpartial.spec.ts(fixes 1 and 2) orcomponent.spec.ts(fixes 3 and 4); the harness commit adds 18 differential parity assertions.pnpm buildorpnpm test— changes are scoped tovite-plugin-angular.compilePartialCode, which is what produces.d.tsdeclarations and what the template type-checker reads.Does this PR introduce a breaking change?
fastCompileis opt-in (defaults tofalse), and every fix moves the emitted AOT metadata strictly toward Angular's official transform shape. Code that worked before continues to work; code that was silently broken now functions correctly (or, in themodel.requiredcase, fails fast at template-typecheck time rather than late at runtime, which is the desired behavior).Other information
See the inline discussion on #2345 for the post-merge audit that motivated this PR. With this branch landed, both fast-compile codepaths (JIT for tests, AOT for application/library builds) emit Angular-equivalent metadata for every signal initializer API, and both have differential parity harnesses preventing regression.
One ancillary finding from the audit that's not addressed by this PR:
metadata.ts:581hardcodesemitFlags: 0on signal queries, but Angular'sR3QueryMetadatareadsemitDistinctChangesOnlyfrom there and signal queries should default totrue(matching upstream). This causes multi-result signal queries to re-emit on every change even when the result list hasn't changed — a minor performance issue, not a correctness bug. Tracked as a separate concern.[optional] What gif best describes this PR or how it makes you feel?
That cleaning-the-bathroom-corners-with-a-toothbrush gif where each tile gets the same systematic attention — except both bathrooms now have parity tests guarding the grout lines.