fix(vite-plugin-angular): signal-API parity in fast-compile JIT transform#2345
Merged
Conversation
The fast-compile JIT transform downleveled viewChild/viewChildren/
contentChild/contentChildren to ViewChild/ContentChild propDecorators
without isSignal: true, so Angular's runtime treated them as regular
decorator queries and never wired the underlying query state to the
signal returned by the call site. Reading a required signal query then
threw NG0951, while non-required queries silently returned undefined.
The transform also dropped the second positional options argument
(e.g. viewChild('ref', { read: ElementRef })), losing read/descendants
configuration entirely.
Match Angular's own JIT initializer transform: forward the predicate as
the first arg and emit { ...userOpts, isSignal: true } as the second.
Fixes #2344
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fast-compile JIT transform downleveled model() to two propDecorators
entries split across two keys — Input under the class field and Output
under a synthetic <name>Change key. Angular's runtime indexes outputs by
classPropertyName and reads instance[classPropertyName] for the emitter
(model exposes it via [SIGNAL] on the signal itself), so the synthetic
key pointed at a field that did not exist on the class and two-way
bindings silently lost the change event.
Also unconditionally emitted {isSignal: true} with no alias and no
required flag, so model.required<T>() skipped the unbound check at
runtime and model('x', {alias: 'foo'}) registered the wrong binding
name on both sides of the two-way binding.
Match Angular's transform: parse the options arg, emit both Input and
Output on the same propDecorators key, and derive the Change binding
name from the alias when one is provided.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fast-compile JIT transform forwarded the user's transform function through to the @input propDecorators entry. Angular's runtime JIT facade then wrapped that value into the directive metadata's transformFunction slot, so the transform ran twice on every binding assignment — once via the input signal (which always applies its own transform), and once via the runtime-derived transformFunction. Match Angular's official initializer transform: omit transform from the emitted decorator config entirely. Also skip shorthand options ({alias}, etc.) because the identifier wouldn't be in scope when the propDecorators static block evaluates at class top level. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The fast-compile JIT transform 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.
Read options from args[1] when the API is outputFromObservable to match
Angular. Also share alias extraction with the model branch via a helper
that skips shorthand properties safely and properly escapes embedded
quotes in alias string literals.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…corator is on the field The fast-compile JIT transform ran both the explicit decorator path and the signal-initializer path on the same member, producing duplicate propDecorators entries (two Input entries for an @Input-decorated input(), an extra synthetic Output for a model() that already has @output, etc.). Angular's transform bails on each branch when a matching decorator is already present. Track which FIELD_DECORATORS are explicitly applied to each member and skip the corresponding signal-downleveling branch (input/model -> Input, output/outputFromObservable/model -> Output, queries -> View/Content query decorators). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-API downleveling The earlier set of JIT signal-API bugs (#2344 and others) all shared a common root cause: existing tests assert string-substring patterns on the emitted code rather than evaluating it. So 'isSignal: true must appear somewhere' passed even when the emit was structurally wrong — wrong field key, dropped second argument, double-applied transform. Add a differential harness that: 1. Runs source through jitTransform. 2. Evaluates the emitted JS in a sandbox with stubbed Angular decorator factories that capture their args. 3. Normalizes the resulting X.propDecorators to the ReflectionCapabilities- visible shape (ngMetadataName + args). 4. Deep-equals against a reference oracle whose entries are pinned to the upstream Angular transforms (input_function.ts, model_function.ts, output_function.ts, query_functions.ts). Each gap fixed in this branch maps to a one-line toEqual assertion: the fixtures evaluate Analog's emit and compare it structurally instead of character-matching. Documents the runtime-equivalent shape divergences (no-alias inputs, no-alias outputs) in oracle comments rather than hiding them behind a fuzzy matcher. 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 refactors JIT signal metadata downleveling in Vite's Angular plugin to fix decorator coexistence and metadata correctness. It adds helper utilities to extract and escape string aliases, introduces explicit decorator tracking to prevent synthetic duplication, refactors Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR combines substantial implementation refactoring of metadata generation with a new ~470-line test harness that requires understanding the transform pipeline, runtime evaluation strategy, and expected metadata shapes. Multiple signal APIs are affected (input, model, output, queries), each with distinct metadata changes. The parity harness introduces a novel evaluation-based comparison pattern. Mixed complexity across files and dense logic density warrant careful review of alias extraction, string escaping, decorator coexistence logic, and test oracle alignment with Angular upstream behavior. 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/vite-plugin-angular/src/lib/compiler/jit-parity.spec.ts (1)
346-373: ⚡ Quick winAdd a quoted-alias parity case for outputs.
The new alias-escaping path is only exercised with safe strings here, so a regression in quote escaping would still pass this suite. A single quoted alias case for both
output()andoutputFromObservable()would pin the new behavior.Suggested coverage
it('output() with alias', () => { const meta = reflect(` import { Component, output } from '`@angular/core`'; `@Component`({ selector: 'x', template: '' }) export class X { ready = output({ alias: 'readyPub' }); } `); expect(meta).toEqual(ref.output('ready', { alias: 'readyPub' })); }); + + it('output() escapes quoted aliases', () => { + const meta = reflect(` + import { Component, output } from '`@angular/core`'; + `@Component`({ selector: 'x', template: '' }) + export class X { ready = output({ alias: "ready'Pub" }); } + `); + expect(meta).toEqual(ref.output('ready', { alias: "ready'Pub" })); + }); @@ it('outputFromObservable threads alias from args[1]', () => { const meta = reflect(` import { Component, outputFromObservable } from '`@angular/core`'; `@Component`({ selector: 'x', template: '' }) export class X { ready = outputFromObservable(null, { alias: 'readyPub' }); } `); expect(meta).toEqual(ref.output('ready', { alias: 'readyPub' })); }); + + it('outputFromObservable escapes quoted aliases', () => { + const meta = reflect(` + import { Component, outputFromObservable } from '`@angular/core`'; + `@Component`({ selector: 'x', template: '' }) + export class X { + ready = outputFromObservable(null, { alias: "ready'Pub" }); + } + `); + expect(meta).toEqual(ref.output('ready', { alias: "ready'Pub" })); + });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/jit-parity.spec.ts` around lines 346 - 373, Add tests that verify quoted-alias escaping for both output() and outputFromObservable(): extend the existing cases using reflect(...) and ref.output(...) to include an alias containing quotes (e.g. alias: '"readyPub"') for the output() test and a matching quoted-alias case for outputFromObservable() (including the two-argument form that currently threads alias from args[1]), and also add a quoted no-alias assertion variant for expectOutputNoAlias if applicable; target the same spec (jit-parity.spec.ts) near the existing output/outputFromObservable tests so the new cases exercise quote-escaping paths.
🤖 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.
Inline comments:
In `@packages/vite-plugin-angular/src/lib/compiler/jit-parity.spec.ts`:
- Around line 432-467: Add a test to cover `@Output` coexistence similar to the
`@Input/`@ViewChild cases: call reflect with code importing Component, Output,
output and a class like "export class X { `@Output`() ready = output(); }", then
assert that meta.ready has length 1, meta.ready[0].ngMetadataName is 'Output',
that the recorded decorator arg (e.g. const arg0 = meta.ready[0].args[0]) does
not have isSignal set, and (if applicable) that no synthetic "readyChange"
metadata was introduced; place this in the existing "decorator + signal
coexistence" describe block alongside the other cases.
In `@packages/vite-plugin-angular/src/lib/compiler/jit-transform.spec.ts`:
- Around line 398-399: The test uses optional chaining on regex matches (e.g.,
expect(propDecorators.match(/type:\s*Input/g)?.length).toBe(1)) which yields a
confusing TypeError when the regex doesn't match; change each occurrence (the
assertions referencing propDecorators.match at the three spots) to first assert
the match is not null/defined and then assert the match.length equals 1 (e.g.,
const m = propDecorators.match(...); expect(m).not.toBeNull();
expect(m!.length).toBe(1)), so failures produce clear assertion messages; apply
this pattern for the three instances currently using ?.length checks.
---
Nitpick comments:
In `@packages/vite-plugin-angular/src/lib/compiler/jit-parity.spec.ts`:
- Around line 346-373: Add tests that verify quoted-alias escaping for both
output() and outputFromObservable(): extend the existing cases using
reflect(...) and ref.output(...) to include an alias containing quotes (e.g.
alias: '"readyPub"') for the output() test and a matching quoted-alias case for
outputFromObservable() (including the two-argument form that currently threads
alias from args[1]), and also add a quoted no-alias assertion variant for
expectOutputNoAlias if applicable; target the same spec (jit-parity.spec.ts)
near the existing output/outputFromObservable tests so the new cases exercise
quote-escaping paths.
🪄 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: 5e6f76a3-5f23-42fd-9a59-3f3d5ec6878c
📒 Files selected for processing (3)
packages/vite-plugin-angular/src/lib/compiler/jit-metadata.tspackages/vite-plugin-angular/src/lib/compiler/jit-parity.spec.tspackages/vite-plugin-angular/src/lib/compiler/jit-transform.spec.ts
Comment on lines
+432
to
+467
| describe('decorator + signal coexistence', () => { | ||
| // Would have caught: duplicate metadata entries. | ||
| it('@Input on a field with input() — only the explicit decorator', () => { | ||
| const meta = reflect(` | ||
| import { Component, Input, input } from '@angular/core'; | ||
| @Component({ selector: 'x', template: '' }) | ||
| export class X { @Input() name = input(); } | ||
| `); | ||
| expect(meta.name).toHaveLength(1); | ||
| expect(meta.name[0].ngMetadataName).toBe('Input'); | ||
| const arg0 = meta.name[0].args[0] as any; | ||
| expect(arg0?.isSignal).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('@Input on a field with model() — only the explicit decorator, no synthetic Change', () => { | ||
| const meta = reflect(` | ||
| import { Component, Input, model } from '@angular/core'; | ||
| @Component({ selector: 'x', template: '' }) | ||
| export class X { @Input() value = model(0); } | ||
| `); | ||
| expect(meta.value).toHaveLength(1); | ||
| expect(meta.value[0].ngMetadataName).toBe('Input'); | ||
| expect(meta.valueChange).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('@ViewChild on a field with viewChild() — only the explicit decorator', () => { | ||
| const meta = reflect(` | ||
| import { Component, ViewChild, viewChild, ElementRef } from '@angular/core'; | ||
| @Component({ selector: 'x', template: '<div #r></div>' }) | ||
| export class X { @ViewChild('r') r = viewChild('r'); } | ||
| `); | ||
| expect(meta.r).toHaveLength(1); | ||
| expect(meta.r[0].ngMetadataName).toBe('ViewChild'); | ||
| const arg1 = meta.r[0].args[1] as any; | ||
| expect(arg1?.isSignal).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Cover explicit @Output coexistence too.
This matrix pins @Input and @ViewChild, but the new skip logic also handles @Output. Without a case like @Output() ready = output(), that branch can regress unnoticed.
Suggested coverage
it('`@ViewChild` on a field with viewChild() — only the explicit decorator', () => {
const meta = reflect(`
import { Component, ViewChild, viewChild, ElementRef } from '`@angular/core`';
`@Component`({ selector: 'x', template: '<div `#r`></div>' })
export class X { `@ViewChild`('r') r = viewChild('r'); }
`);
expect(meta.r).toHaveLength(1);
expect(meta.r[0].ngMetadataName).toBe('ViewChild');
const arg1 = meta.r[0].args[1] as any;
expect(arg1?.isSignal).toBeUndefined();
});
+
+ it('`@Output` on a field with output() — only the explicit decorator', () => {
+ const meta = reflect(`
+ import { Component, Output, output } from '`@angular/core`';
+ `@Component`({ selector: 'x', template: '' })
+ export class X { `@Output`() ready = output(); }
+ `);
+ expect(meta.ready).toHaveLength(1);
+ expect(meta.ready[0].ngMetadataName).toBe('Output');
+ });As per coding guidelines, packages/vite-plugin-angular/**: Prioritize Angular and Vite integration behavior, build compatibility, backward compatibility, stability, and targeted coverage for regressions, and **/*.{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/jit-parity.spec.ts` around
lines 432 - 467, Add a test to cover `@Output` coexistence similar to the
`@Input/`@ViewChild cases: call reflect with code importing Component, Output,
output and a class like "export class X { `@Output`() ready = output(); }", then
assert that meta.ready has length 1, meta.ready[0].ngMetadataName is 'Output',
that the recorded decorator arg (e.g. const arg0 = meta.ready[0].args[0]) does
not have isSignal set, and (if applicable) that no synthetic "readyChange"
metadata was introduced; place this in the existing "decorator + signal
coexistence" describe block alongside the other cases.
Comment on lines
+398
to
+399
| expect(propDecorators.match(/type:\s*Input/g)?.length).toBe(1); | ||
| expect(propDecorators).not.toContain('isSignal: true'); |
There was a problem hiding this comment.
Optional chaining before .toBe(1) can produce unclear test failures.
At lines 398, 412, and 426, the pattern .match(regex)?.length).toBe(1) will throw a TypeError ("Cannot read property 'toBe' of undefined") if the regex doesn't match, rather than producing a clear assertion failure message.
🔍 Proposed fix for clearer test failures
- expect(propDecorators.match(/type:\s*Input/g)?.length).toBe(1);
+ const inputMatches = propDecorators.match(/type:\s*Input/g);
+ expect(inputMatches).toBeDefined();
+ expect(inputMatches!.length).toBe(1);Apply similar changes to lines 412 and 426.
Also applies to: 412-412, 426-426
🤖 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/jit-transform.spec.ts` around
lines 398 - 399, The test uses optional chaining on regex matches (e.g.,
expect(propDecorators.match(/type:\s*Input/g)?.length).toBe(1)) which yields a
confusing TypeError when the regex doesn't match; change each occurrence (the
assertions referencing propDecorators.match at the three spots) to first assert
the match is not null/defined and then assert the match.length equals 1 (e.g.,
const m = propDecorators.match(...); expect(m).not.toBeNull();
expect(m!.length).toBe(1)), so failures produce clear assertion messages; apply
this pattern for the three instances currently using ?.length checks.
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
Reported in FastCompile throws NG0951 for viewChild.required analogjs/analog#2344. The investigation surfaced four additional gaps in the fast-compile JIT downleveling path that share the same root cause (the existing tests assert string-substring patterns on emitted code rather than evaluating it), so the PR fixes the reported bug plus the rest and adds a differential parity harness so future divergences are caught structurally.
Closes analogjs/analog#2344
Affected scope
vite-plugin-angularRecommended merge strategy for maintainer [optional]
Commit preservation note [optional]
The six commits are each focused on one specific gap or one piece of test infrastructure, with the parity harness landing last so it exercises the fixed transform rather than the buggy one. Each commit message stands on its own as a description of one bug and one fix. Rebase merge preserves that history for
git bisectand future audits; happy to switch to squash if maintainers prefer.What is the new behavior?
Fast-compile's JIT downleveling now matches Angular's official initializer-API transform (
@angular/compiler-cli/src/ngtsc/transform/jit/src/initializer_api_transforms/) for every signal API. The six commits, in order:emit isSignal on jit signal-query decorators— fixes analogjs/analog#2344.viewChild/viewChildren/contentChild/contentChildrennow emit{ ...userOpts, isSignal: true }as the second decorator argument, so the runtime JIT compiler wires them through the signal-query infrastructure rather than treating them as plain@ViewChild/@ContentChildassignments. Also preserves the second positional options argument ({ read: ElementRef },{ descendants: true }) which was being silently dropped.preserve model() alias, required, and field—model()now parses its options, honors.required, and places both@Inputand@Outputunder the same propDecorators key (the class field name) so Angular's runtime — which indexes outputs byclassPropertyNameand readsinstance[classPropertyName]for the emitter — actually finds the model signal's[SIGNAL]-exposed emitter. The Change binding name derives from the alias when one is given.drop transform from input() jit metadata— stops forwarding the user'stransform: fnoption to the@Inputdecorator config. The input signal already runs the transform internally; forwarding it caused Angular's runtime JIT facade to re-wrap it into the directive metadata'stransformFunctionslot, applying the transform twice. Most transforms are idempotent so this rarely surfaced as a visible bug.read outputFromObservable options from args[1]—outputFromObservable(stream, opts)'s options live atargs[1]becauseargs[0]is the source observable. The transform was readingargs[0]for bothoutputandoutputFromObservable, so aliases onoutputFromObservable(stream, { alias: 'foo' })were silently dropped. Shared alias extraction now goes through a single helper that also handles shorthand-property skip and quote escaping.skip signal downleveling when a matching decorator is on the field— when a field has both an explicit@Input/@Output/@ViewChildand a signal initializer, the signal-downleveling branch is now skipped. The explicit decorator wins, matching Angular's transform behavior (input_function.ts:42-48,model_function.ts:31-37,output_function.ts:34-40,query_functions.ts:54-58).differential parity harness for jit signal-API downleveling— addsjit-parity.spec.ts. The earlier bugs all shared a root cause: existing JIT-emit tests assertexpect(result).toContain('isSignal: true')and similar string-substring patterns, which passed even when the emit was structurally wrong (wrong field key, dropped argument, double-applied transform). The new harness:jitTransformX.propDecoratorsto theReflectionCapabilities-visible shapeEach gap fixed in this PR maps to a one-line
toEqualassertion in the parity suite.Test plan
nx format:check— all touched files pass; repo-wide check 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— 1134 passed | 23 skipped (1157 total). The five fix commits add 8 targeted tests acrossjit-transform.spec.ts; the harness commit adds 21 parity assertions across all signal APIs.pnpm buildorpnpm test— changes are scoped tovite-plugin-angularand the rest of the workspace doesn't depend on the touched files. Happy to run the full suite if maintainers prefer.#2344repro in a liveTestBed— the parity harness exercises the same metadata-shapeReflectionCapabilitiesreads at runtime, but a maintainer-side repro confirmation would be a useful additional signal.Does this PR introduce a breaking change?
fastCompileis opt-in (defaults tofalse), and every fix moves emitted metadata strictly toward Angular's official transform shape — code that worked before continues to work, code that was silently broken now functions correctly.Other information
The investigation behind this PR includes a write-up of why the bugs weren't caught earlier and a separate audit confirming the AOT path (
metadata.ts) is unaffected — it emitsisSignal: truecorrectly at lines 478, 503, 582 because that path goes through the real@angular/compiler-cli. The gaps were JIT-only.[optional] What gif best describes this PR or how it makes you feel?
A duck calmly swimming on the surface while its feet paddle furiously underneath the propDecorators block.