feat(class_metadata): inline templateUrl/styleUrls + default-on, matching ngc#299
Merged
Brooooooklyn merged 6 commits intoMay 27, 2026
Conversation
When emit_class_metadata is enabled, OXC was preserving the original
`templateUrl` and `styleUrls` literals in the setClassMetadata args.
TestBed's JIT recompilation path then trips on Angular's
`componentNeedsResolution` check ('Component is not resolved: templateUrl…').
Match Angular's reference behavior (transformDecoratorResources in
compiler-cli/src/ngtsc/annotations/component/src/resources.ts):
- Replace `templateUrl` in place with `template` carrying the inlined content.
- Drop `styleUrls` / `styleUrl` / `styles` and re-emit a single consolidated
`styles` array, filtering whitespace-only entries.
- Bail out unchanged when the source has no resource fields.
The new `build_decorator_metadata_array` takes optional `inlined_template`
and `inlined_styles` params; the component transform call site passes the
resolved template and `metadata.styles` (which resolve_styles already merges
with inline source styles). Other callers — ctor params, prop decorators, the
NAPI standalone API — pass None.
The Vite plugin gains an `emitClassMetadata?: boolean` option that forwards
through to `TransformOptions`.
Angular's reference compiler (`ngc`) unconditionally emits `setClassMetadata` calls, wrapped in `(typeof ngDevMode === "undefined" || ngDevMode) && …` so production bundles tree-shake them. OXC was defaulting this off at every layer, which silently broke TestBed/Vitest setups until users discovered the flag. Flip the default to `true` at all three layers so behavior matches `ngc` end to end: - Rust `TransformOptions::default()` (crate API) - NAPI binding `unwrap_or` (transformAngularFile JS entry point) - Vite plugin `?? true` (the `@oxc-angular/vite` user-facing option) The flag remains an escape hatch — passing `false` still elides the call. Test-driven: added `test/class-metadata-default.test.ts` covering default-on, ngDevMode wrapping, explicit-false, and explicit-true. Updated 14 integration snapshots to include the new (correctly-guarded) `setClassMetadata` block, and scoped `test_standalone_component_omits_standalone_field` to the `ɵɵdefineComponent` literal — the metadata block faithfully preserves the source decorator's `standalone: true` (matching ngc), which the test was inadvertently catching with a whole-file substring check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`transformDecoratorResources` in compiler-cli operates on a `Map<string,
ts.Expression>`. Its `templateUrl` → `template` substitution is:
metadata.delete('templateUrl');
metadata.set('template', …);
`Map.set` on a fresh key appends at the end; on an existing key it overwrites
in place. Two divergences in our port:
1. **Duplicate `template` keys** when source illegally contained both `template`
and `templateUrl`. Source `template` fell through the loop's `_` arm and the
`templateUrl` arm pushed a second `template:`, emitting invalid object syntax
in strict mode and differing from ngc's single-key output.
2. **In-place ordering** for `templateUrl` replacement. We kept `template` at
`templateUrl`'s original position; ngc's `delete` + `set` appends to the end
of the Map's insertion order. The e2e compare does whitespace-normalized
string equality on the decorators payload, so any component shaped like
`{ selector, templateUrl, encapsulation }` would silently miscompare.
Implementation:
- Drop all of `templateUrl` / `styleUrls` / `styleUrl` / `styles` in the pass
loop unconditionally (matches Angular's unconditional `metadata.delete`s).
- If `template` was in source AND `templateUrl` was in source, overwrite the
existing `template` entry in place during the loop (preserves position,
matches `Map.set` overwrite semantics).
- Otherwise, if `templateUrl` was in source, append `template` at the end
after the loop (matches `Map.set` on a fresh key).
- Styles continue to be appended at the end — that already matched.
Test-driven: added two RED tests covering each divergence; both fail against
the previous in-place implementation and pass under the new logic. Pulled the
template-entry construction into `build_template_entry` since it's now used
in two places.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three deeper-audit findings, all confirmed by reading Angular's source side-by-side and verified empirically by the e2e ngc-compare harness (now 759/759 fixtures matching across 23 categories). 1. **Decorator-name guard.** `inline_component_resources` ran on `decorator_idx == 0 && arg_idx == 0` regardless of the decorator's identifier. Hidden by upstream filtering at the @component call site, but visible through `build_decorator_metadata_array`'s use for constructor-param decorators: `@Inject({ templateUrl: '…', styleUrls: […] })` (legal TS, semantically nonsensical) had its keys silently stripped. Added `is_component_decorator` check to the rewrite gate, matching `transformDecoratorResources`' opening `if (dec.name !== 'Component') return dec;`. 2. **`resolve_template` precedence.** When source had BOTH inline `template` and `templateUrl`, OXC preferred the inline content. Angular's AOT compiler (`parseTemplateDeclaration` in `compiler-cli/.../component/src/resources.ts`) checks `component.has('templateUrl')` first and returns immediately — `templateUrl` wins, inline `template` is silently ignored. (Angular's JIT runtime diverges via `componentNeedsResolution`, preferring inline — irrelevant here since OXC is AOT-equivalent.) Flipped the order in `resolve_template` and updated its docstring with the source citation. Affects both the runtime template parse and the metadata inlining; only manifests in illegal "both present" components. 3. **Spread elements.** `@Component({ ...config, … })` survives untouched — the spread argument isn't statically evaluated, so resource fields inside it can leak past `componentNeedsResolution` at runtime. Angular's reference operates on a post-spread-resolution `Map`, so it doesn't hit this. Fixing it requires structural upstream work (pre-extraction spread resolution) beyond this branch's scope. Added a locking-in test so any future change has to consciously decide what to do with spreads. All three findings get RED tests written first; the fix flips them GREEN. Total: 21 class-metadata-resources tests pass (up from 17 in the previous commit), full Rust suite + JS suite + e2e ngc fixtures all 100%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Cosmetic line wrapping changes from `cargo fmt --check`; no behavior delta. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lugin The function-level doc for `build_decorator_metadata_array` said the rewrite "matches `@analogjs/vite-plugin-angular`'s behavior". That's misleading on two counts: - The real reference is `transformDecoratorResources` in Angular's own `compiler-cli/.../component/src/resources.ts` (which the deeper `inline_component_resources` doc cites correctly with line semantics). - Pointing at a third-party plugin in a generic library's API docs conflates the upstream source-of-truth with one downstream consumer. Updated the comment to direct readers to the actual source-cited spec right below it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d06374a8b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Brooooooklyn
approved these changes
May 27, 2026
Brooooooklyn
added a commit
that referenced
this pull request
May 27, 2026
After #299 turned `emit_class_metadata` on by default, the raw `${UNRESOLVED}-tag` template literal is intentionally preserved verbatim inside `ɵsetClassMetadata(...)` to mirror ngc — that's runtime metadata, not the compiled selector. The original test asserted on the full output and now panics on every CI run since d83445f landed on main. Narrow the assertion to the `ɵcmp` definition itself and additionally verify the compiled selector falls back to `ng-component`, matching the test's stated intent ("unresolved interpolation must not produce a partial/garbage selector"). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Brooooooklyn
added a commit
that referenced
this pull request
May 28, 2026
) * fix(decorator): hoist consts referenced by emitted Ivy definitions Closes #287. When `@Component` metadata such as `providers` references a top-level `const` declared *after* the class, the emitted `static ɵcmp` field evaluates `ɵɵProvidersFeature([…])` eagerly at class-init time. With the binding still in TDZ, this threw `ReferenceError` at module load. Match Angular's official compiler by hoisting referenced VariableDeclaration statements above the earliest class that needs them. Identifier collection walks decorator metadata but stops at function/arrow/class bodies, so lazy references like `useFactory: () => DEP` don't trigger unnecessary moves. Function declarations are JS-hoisted already, and class declarations are intentionally skipped to avoid clobbering the transform pipeline's existing edits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test: scope unresolved-interpolation assertion to ɵcmp selectors only After #299 turned `emit_class_metadata` on by default, the raw `${UNRESOLVED}-tag` template literal is intentionally preserved verbatim inside `ɵsetClassMetadata(...)` to mirror ngc — that's runtime metadata, not the compiled selector. The original test asserted on the full output and now panics on every CI run since d83445f landed on main. Narrow the assertion to the `ɵcmp` definition itself and additionally verify the compiled selector falls back to `ng-component`, matching the test's stated intent ("unresolved interpolation must not produce a partial/garbage selector"). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): transitive deps + deterministic dedup for shared statements Two PR-review bugs in the hoist pass: 1. **Transitive deps were dropped.** When `@Component({ providers: PROVIDERS })` was followed by `const PROVIDERS = [{ provide: TOKEN, ... }]; const TOKEN = ...;`, only `PROVIDERS` got hoisted. The hoisted `const PROVIDERS = [...]` then evaluated above the class and TDZ-threw on `TOKEN` — just moving the same `ReferenceError` one frame deeper. 2. **Multi-declarator dedup was nondeterministic.** The plan was keyed by binding *name*, so `const A = 1, B = 2;` referenced by two different classes produced two `HoistEntry` values sharing `stmt_start` but carrying different `insert_at` targets. The `emitted_stmts` dedup kept whichever HashMap iteration visited first — often the *later* class — leaving the earlier class in the TDZ. Fix: * Key the plan by `stmt_start` and merge collisions by taking MIN `insert_at` (no more nondeterministic dedup; multi-declarators collapse to one entry by construction). * Per-statement, collect the union of identifier references across every declarator initializer and feed them back into a BFS worklist — so hoisting `PROVIDERS` also schedules `TOKEN` (transitive closure). * Topologically sort the planned statements before emission so dependencies land *before* their dependents in the hoisted prelude (e.g. `const TOKEN` precedes `const PROVIDERS = [{ provide: TOKEN, ... }]`). DFS is iterative to avoid stack overflow on deep chains; cycles are broken silently because they can't yield a valid evaluation order anyway. Adds two integration tests that lock in both fixes: * `component_provider_aggregate_const_pulls_in_transitive_tdz_dep` * `component_shared_multideclarator_const_hoists_above_earliest_referencer` (the latter was flaky against the old code — 7/20 failure rate locally before this change, 30/30 passes after). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): boundary + function-call transitive TDZ deps Two PR #302 review fixes for the const-hoist: 1. Off-by-one at the class-body boundary (Cursor): the check `stmt_start <= class_body_end` skipped a const declared at exactly `class.body.span.end` (no whitespace between `}` and `const`). `class.body.span.end` is the exclusive end of the body, so a stmt starting there is the very next byte after the class and still needs hoisting. Switched to `<`. To keep the new delete from chewing into the `decls_after_class` insert at the same offset, the hoist delete now runs with a negative priority so it applies before that insert. 2. Transitive deps through function calls (Codex): a hoisted initializer that calls a top-level function (`const PROVIDERS = makeProviders()`) evaluates that function's body at module load, so any later-declared binding the body reads still TDZ-throws. Indexed top-level function declarations via `oxc_ast_visit::Visit` (skipping nested function / arrow / class expression bodies for the same lazy-evaluation reason as the existing expression walker), and chase identifiers through them in the BFS and in the topological-sort edge expansion. Added two regression tests: - `component_provider_const_immediately_after_class_brace_is_hoisted` - `component_provider_const_via_function_call_pulls_in_transitive_tdz_dep` Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(hoist): resolve bindings via oxc_semantic SymbolIds Replace the hand-rolled name-string indexing in `collect_hoist_edits` with SymbolId-based resolution backed by `oxc_semantic`. The AOT path in `transform_angular_file` now builds a `Semantic` and threads it into the hoister so every `IdentifierReference` resolves through the symbol table to the actual declaring binding. Previously, `binding_to_stmt: HashMap<&str, u32>` and `fn_body_idents: HashMap<&str, HashSet<&str>>` keyed everything by the identifier's spelling. If a nested scope shadowed a top-level binding with the same name, the walker couldn't tell them apart and might count a non-top-level reference as a TDZ-relevant hit on the top-level binding. After this refactor, every reference is resolved to a `SymbolId` and matched against the top-level binding set, so shadows are impossible. No observable behavior change is intended: all 344 integration tests (including the two regression tests from PR #302 `component_provider_const_immediately_after_class_brace_is_hoisted` and `component_provider_const_via_function_call_pulls_in_transitive_tdz_dep`) continue to pass. The hoist priorities, off-by-one boundary check, topological sort, and lazy-body skipping are all preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): lazy fn-value, optional chain, destructured bindings Three Codex review findings on PR #302: 1. Lazy fn-value over-hoisting: when a top-level function is referenced from decorator metadata as a *value* (e.g. `useFactory: makeFactory`) the BFS still chased its body refs and hoisted them above the class. Since Angular invokes such factories lazily, this could introduce a fresh TDZ that didn't exist in the source. Introduce an `eagerly_called` closure (direct callees in any top-level initializer or decorator metadata, transitively expanded through `fn_body_called_symbols`) and gate the BFS body-chase branch plus `expand_through_functions` on it. Regression test: `component_provider_useFactory_function_value_does_not_hoist_body_deps`. 2. `Expression::ChainExpression` swallowed by catch-all: optional chaining like `{ provide: TOKEN?.id, useValue: 1 }` recorded no reference to `TOKEN`. Add an explicit arm that dispatches each `ChainElement` variant (`CallExpression`, member variants, `TSNonNullExpression`) to the matching collection logic, also recording the inner call's direct callee for `f?.()`. Regression test: `component_provider_optional_chain_token_is_hoisted`. 3. Destructured top-level bindings not indexed: `collect_top_level_bindings` only handled `BindingPattern::BindingIdentifier`, so `const { TOKEN } = TOKENS;` never made it into `symbol_to_stmt`. Add `for_each_binding_identifier` recursive walker covering `ObjectPattern`/`ArrayPattern`/`AssignmentPattern` (plus rest elements and nested patterns). Regression test: `component_provider_destructured_top_level_token_is_hoisted`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): per-class eager-call, IIFE metadata, multi-decl class-ref guard Three bot-review findings on the post-refactor hoist: 1. Multi-declarator over-hoist (Codex). When a `const TOKEN = 'tok', BACKREF = TestComponent;` declarator list is hoisted because TOKEN is referenced in decorator metadata, the peer `BACKREF = TestComponent` moves above the class and introduces a new TDZ. Added a safe-skip: refuse to hoist a statement when any of its initializer symbols resolves to a top-level class declared at position >= `effective_start`. Indexed via a new `collect_top_level_class_positions` helper. (Split-hoist is intentionally out of scope; the safe-skip preserves the "no regressions" invariant — the user's existing TDZ on the directly-referenced symbol stays, but we don't introduce a new one.) 2. IIFE bodies in decorator metadata missed (Codex). `providers: (() => [{ provide: TOKEN }])()` runs the arrow body eagerly at class init, but the lazy-bodies rule was treating every arrow/function expression body as opaque. New `walk_iife_callee_body` detects an immediately- invoked function/arrow callee (peeling parens + TS wrappers) and walks the body via `FunctionBodyIdentVisitor`; the wider lazy rule still applies elsewhere. Symmetric handling for `NewExpression` and `ChainElement::CallExpression`. 3. Global `eagerly_called` bleeds across classes (Cursor). Previously seeded from every top-level initializer's call sites + every decorator's call sites, so `const X = foo()` in one part of the module would mark `foo` as eagerly-called for every other class too — even classes that only reference `foo` as a value (`useFactory: foo`). Replaced the global precomputation with a per-class closure inside the BFS: seeded only from THIS class's `decorator_called`, extended incrementally as the BFS plans bindings (adding each planned init's `init_called_symbols` and re-closing through `fn_body_called_symbols`). Functions popped before they became eagerly_called are deferred and belatedly chased when promotion happens. Topological sort uses the union of per-class sets so dependency edges still see every fn whose body fires at module load for some hoisted statement. Regression tests for each finding: - component_provider_multi_declarator_with_class_self_ref_skips_hoist - component_provider_iife_metadata_hoists_inner_token - component_provider_useFactory_value_does_not_chase_global_eager_caller All 350 integration tests pass; fmt clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): chase param default refs of eagerly called functions Parameter default expressions (`function f(x = TOKEN)` / `function f({ a = X } = {})`) evaluate at call time, before the body runs. When a hoisted initializer eagerly calls a top-level function, any later-declared binding read by a parameter default is just as TDZ-relevant as a body ref. The previous scan only walked the function body, so such defaults left their referenced bindings below the class and the hoisted call-site threw `ReferenceError: Cannot access ... before initialization`. Add a `for_each_pattern_default` helper that yields every nested `AssignmentPattern::right` inside a `BindingPattern`. Use it (alongside `FormalParameter::initializer` for top-level defaults) in `collect_top_level_bindings` for top-level function declarations and in `walk_iife_callee_body` for IIFE callee arrow/function expressions. The collected refs and direct callees join the same `fn_body_symbol_refs` / `fn_body_called_symbols` sets the body walk populates, so the existing BFS and eagerly-called closure pick them up transparently. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): chase destructuring defaults; walk IIFE bodies in eagerly-called fns `collect_top_level_bindings` now walks `AssignmentPattern.right` defaults nested in each declarator's binding pattern, so `const { TOKEN = FALLBACK } = {}` records `FALLBACK` as an init-time dep and keeps the dependency graph TDZ-correct. `FunctionBodyIdentVisitor` now detects IIFE callees in `visit_call_expression` / `visit_new_expression` and walks their bodies eagerly via `walk_iife_callee_body`, so IIFEs inside an eagerly-called top-level function no longer drop their identifier reads. Addresses Codex P2 #3311274924 and Cursor #3311313158 on PR #302. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): close class-TDZ guard transitively; walk assignment refs Resolves the two new inline bot comments on PR #302 and the follow-on adversarial findings the fix surfaced. * Codex P2 #3311493528 — the `stmt_references_later_class` guard now closes `init_called_symbols` through `fn_body_called_symbols` and checks each function's `fn_body_symbol_refs` for class refs, so an initializer that eagerly calls a function whose body reads a later class is no longer hoisted above that class. * Cursor Low #3311551145 — `collect_expr_symbols` gains explicit `AssignmentExpression` / `UpdateExpression` arms plus `collect_assignment_target_symbols` and siblings, so identifier refs on assignment-target lvalues (including pattern targets with defaults and member targets) flow into the dependency graph. * Cascade un-planning ("Step 2c"): when a planned statement's full dep closure reaches a top-level binding that isn't planned at an `insert_at` ≤ the dependent's `insert_at`, the dependent is dropped and the loop iterates to a fixed point — prevents leaving caller hoists stranded when a transitive dep was guard-skipped or planned only for a later class. * Function-valued bindings indexed: each per-declarator `const f = () => …` / `function() { … }` populates `fn_body_*` maps so the guard chases through them like a function declaration. Covers single- and multi-declarator forms. * Indirect call shapes: `fn.call(...)`, `fn.apply(...)`, and `fn.bind(...)()` recorded as eager invocations of `fn` at every call/new visit site. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): chase fn-valued binding bodies in guard, cascade, and topo Resolves the new inline bot comments on PR #302 and the follow-on adversarial findings the fix surfaced. * Codex P2 #3311913006 — when a popped symbol both has `symbol_to_stmt` and `fn_body_symbol_refs`, the BFS branch now plans the statement AND chases body refs (or defers them via `deferred_fns` when the binding isn't yet eagerly called). * Cursor Low #3311962888 — `topological_order` no longer takes the global `combined_eagerly_called`; per-S `stmt_eager_sets` and a shared `stmt_fn_valued_bindings` map drive the dep-edge construction, matching the cascade's reasoning. Adversarial follow-up: the safe-skip guard and cascade un-planning were blind to fn-valued bindings whose arrow body reads a class — for `@Component({providers: make()}) class C {} const make = () => C;` the planner hoisted `make` above `C` and Ivy's eager `make()` then hit TDZ on `C`. The fix lifts `stmt_fn_valued_bindings` to right after `collect_top_level_bindings` and folds each statement's eagerly-called fn-valued binding bodies into both the per-class safe-skip closure (via the BFS's `eagerly_called`) and the cascade closure (via `combined_eagerly_called`), keeping all three passes symmetric. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): index class constructors and walk eager class-expr parts * Codex P2 #3312108552 — top-level class declarations are now indexed into `fn_body_symbol_refs` / `fn_body_called_symbols` (with `include_constructor: true`) via a new `walk_class_eager_parts` helper. The helper covers the union of class-definition-time eager refs (super_class, computed keys, static field / accessor initializers, static blocks) and `new`-time eager refs (constructor body + parameter defaults, instance field / accessor initializers). The BFS body-chase branch then chases through `new ClassName()` callers in hoisted initializers — `const PROVIDERS = [new S()]` no longer hoists past `class TestComponent` when `S`'s constructor reads a later const. * Codex P2 #3312108558 — `collect_expr_symbols`'s previously-opaque `E::ClassExpression` arm now calls `walk_class_eager_parts` with `include_constructor: false`, so the eager parts of an inline class expression (super_class, computed keys, static initializers, static blocks) feed the dependency graph. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): walk classes inside fn bodies, branch callees, tag template tags * Codex P2 #3314767088 — `FunctionBodyIdentVisitor::visit_class` was a no-op, dropping inline `class extends TOKEN {}` defined inside an eagerly-called function body. It now calls `walk_class_eager_parts` with `include_constructor: true` so super_class, computed keys, static initializers/blocks, and (conservatively) constructor body refs feed `fn_body_symbol_refs` / `fn_body_called_symbols`. * Codex P2 #3314767091 — `record_direct_callee` only recognised bare identifiers under parens / TS wrappers. Restructured as an iterative worklist that also descends into both branches of `ConditionalExpression` / `LogicalExpression` and the last expression of a `SequenceExpression`, so callees like `(cond ? makeA : makeB)()` and `(makeA || makeB)()` enter the eager-call set. * Cursor Low #3314770575 — `FunctionBodyIdentVisitor` gained `visit_tagged_template_expression`, recording the tag via the same direct/indirect/bind callee helpers as `visit_call_expression` so `tag\`…\`` inside a top-level function body chases the tag's body refs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): record indirect/bind callees on tagged-template tags `collect_expr_symbols::E::TaggedTemplateExpression` only called `record_direct_callee`, while the call/new arms and the body visitor's override already invoke all three callee helpers. Decorator metadata like `make.bind(null)\`...\`` or `make.call(this)\`...\`` therefore recorded `make` as a value reference but never as an eager callee, so its body wasn't chased and a later-declared binding it reads stayed below the class. Now mirrors call/new: direct + indirect + bind. Closes Cursor Low #3314809112 and Codex P2 #3314810080. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): chase fn-valued binding bodies for pre-class bindings too The BFS branch for a `symbol_to_stmt` hit ran the fn-valued binding body-chase only AFTER the `stmt_start < class_body_end` early-continue, so a `const make = () => [{ provide: TOKEN }];` declared *before* the decorated class slipped past the chase entirely. `make`'s binding is indeed initialized at class-eval time, but the arrow body still fires when the decorator calls `make()` — and its later-declared reads stay in TDZ. Moved the body-chase block to BEFORE the early-continue so it runs regardless of stmt position; removed the now-duplicate post-plan copy. Closes Codex P2 #3314836115. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(hoist): drop stale 'moved from' comment in BFS Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(hoist): strip PR/issue bookkeeping references from comments Removes trailing `See PR #302 ...`, `Regression for/test for ... review #...`, `Round N follow-on review`, `Codex P2/P3 review Finding N`, etc. — the references that belong in PR descriptions and rot as the codebase moves. Substantive technical justifications kept; only the bookkeeping tails removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): descend through branch receivers; symmetric topo eager close * `record_indirect_callee` / `record_bind_callee` previously required the static-member receiver to be a bare identifier, missing shapes like `(cond ? makeA : makeB).call(null)` and `(cond ? makeA : makeB).bind(null)()`. Both now delegate the receiver descent to `record_direct_callee`, which already peels parens / TS wrappers and walks conditional / logical / sequence branches. * The topo-precompute eager set was closed under `fn_body_called_symbols` BEFORE folding in fn-valued binding symbols, while the cascade did the opposite order. Reordered the precompute to match the cascade so both passes expand through the same transitive callees and dependency edges through fn-valued bindings stay visible to the topological sort. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): chase named nested function bodies in body visitor `FunctionBodyIdentVisitor::visit_function` no-opped on every nested function, so a local declaration like `function outer() { function inner() { return TOKEN; } return inner(); }` called from an eagerly-evaluated body never contributed `TOKEN` to the eager surface — `inner` is a local symbol that's not indexed in any `fn_body_*` map, so `close_eagerly_called` can't chase through it. The visitor now walks the body and parameter defaults of any nested `Function` with `id: Some(_)`. Anonymous functions and arrow expressions remain lazy. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): chase locally-bound arrow/fn helpers when called eagerly `FunctionBodyIdentVisitor` skipped arrows entirely, so a local binding like `const inner = () => TOKEN;` inside an eagerly-called body left `inner` recorded as a callee but with no body to chase — `TOKEN` stayed below the class. Added a per-visitor `local_fn_bodies` map populated by a new `visit_variable_declarator` override that indexes arrow/function inits via a scratch walk. At each `CallExpression` / `NewExpression` / `TaggedTemplateExpression` we resolve every reachable callee identifier (through parens, TS wrappers, conditional / logical / sequence branches, `.call` / `.apply` receivers, and the inner call of `.bind(...)()`) and fold the indexed body's refs / callees into the eager surface, with a `folded` guard for self- and mutually-recursive arrows. Value-passed arrows that are never invoked inside the body (`useFactory: lazy`) stay lazy because the fold only fires at call sites. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(hoist): index nested fn declarations lazily; fold only at call sites The prior round made `FunctionBodyIdentVisitor::visit_function` walk named nested function bodies unconditionally, which broke laziness — an uncalled `function unused() { return TOKEN; }` declared inside an eagerly-called helper folded `TOKEN` into the eager surface and could hoist it above the class for no reason. Mirror the local-arrow path: `visit_function` now indexes the function into `local_fn_bodies` and defers the fold to call sites. A new `visit_function_body` / `visit_block_statement` pre-pass indexes hoisted function declarations before source-order walking so a call that textually precedes its declaration still resolves. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Brooooooklyn
pushed a commit
that referenced
this pull request
May 28, 2026
…d all decorated class kinds (#303) * feat(class_metadata): emit initializer-API prop decorators for JIT/TestBed Signal-based members (`input()`/`input.required()`/`output()`/`model()`/ `viewChild()`/`viewChildren()`/`contentChild()`/`contentChildren()`) live only in the AOT `ɵcmp`, which `TestBed.overrideComponent` discards before recompiling via JIT. The JIT recompile reconstructs inputs/outputs/queries from decorator and prop metadata reflected off the class, so signal members were silently dropped on recompile — `setInput`/router-binding then failed with NG0315/NG0303/NG0950. Synthesize the equivalent `@Input`/`@Output`/query prop decorators into `setClassMetadata`, mirroring `@angular/compiler-cli`'s `initializer_api_transforms` (applied by the Angular CLI in test builds). The decorator type is referenced via the `@angular/core` namespace import (`i0.Input`), exactly as ngc emits it; output verified byte-for-byte against ngc 21.2 across all initializer-API member kinds. * fix(class_metadata): drop unresolvable template-literal config fields from setClassMetadata A `@Component` config field whose value is a template literal with an unresolvable `${…}` interpolation (e.g. ``selector: `${UNRESOLVED}-tag` ``) was copied verbatim into `ɵsetClassMetadata`, leaking the raw `${…}` text. The AOT `ɵcmp` path already drops such fields (#300); apply the same fold-or-drop on the `setClassMetadata` decorator-args path. Expose `resolve_template_literal` as `pub(crate)` and thread the file-scope string consts into `build_decorator_metadata_array`. Resolvable interpolations (`${KNOWN_CONST}`) still fold and are left untouched; only unresolvable fields are dropped (post-conversion, so key/quoting/ordering fidelity is preserved). Fixes the pre-existing `component_template_literal_unresolved_identifier_drops_field` test, which default-on `emit_class_metadata` (#299) had turned red. * fix(directive): recognize as-cast/parenthesized and namespaced-required initializer APIs Match ngc's `tryParseInitializerApi`, which unwraps `as`/parenthesized expressions and resolves namespaced calls: - `x = input(0) as any` / `x = (input(0))` — unwrap `TSAsExpression`/ `ParenthesizedExpression` before matching the initializer call (applies to input/output/model/query detectors, so both the AOT ɵcmp and setClassMetadata paths recognize them). - `core.input.required()` / `core.model.required()` — recognize the namespaced `<ns>.<fn>.required()` form, not just `<fn>.required()`. Output verified byte-for-byte against @angular/compiler-cli 21.2.14. * feat(directive): emit setClassMetadata for directives Directives previously emitted no `setClassMetadata`, so `TestBed.overrideDirective` could not reflect their decorators and signal inputs/outputs/queries were lost on recompile. Emit it for the `@Directive` AOT branch, mirroring the `@Component` path: the `@Directive` decorator metadata, `ctorParameters` (reflected from the class), and prop decorators (real `@Input`/`@Output`/query decorators plus synthesized initializer-API ones). Matches ngc, which emits directive class metadata. Constructor-token types are reflected from the AST (not namespace-prefixed) since directive metadata carries a different dependency representation than `build_ctor_params_metadata` consumes — the common no-constructor directive emits `null` ctorParameters, exactly like ngc. Updates two host-directive snapshots that now include the directive `setClassMetadata`. * feat(class_metadata): emit setClassMetadata for pipes, injectables, and NgModules Extends directive metadata support to the remaining decorated class kinds so TestBed.overridePipe/overrideModule and reflection work, matching ngc which emits setClassMetadata for every decorated class. Each emits the decorator metadata, ctorParameters (reflected from the class — imported token types are namespace- prefixed via the import map, e.g. `() => [{ type: i0.Injector }]`), and prop decorators. Extracts the shared `build_set_class_metadata_decls` helper (used by the @Directive/@Pipe/@Injectable/@NgModule branches), replacing the inline directive block. Output verified byte-for-byte against @angular/compiler-cli 21.2.14 for each kind, including constructor-DI cases. * test(class_metadata): cover full classic decorator set in setClassMetadata Add regression guards for the classic (non-signal) decorators in setClassMetadata, on both @component and @directive: plain/aliased/config @input, plain/aliased @output, @ViewChild/@ViewChildren, @ContentChild/@ContentChildren (with read/ descendants), @HostBinding, and @HostListener (with args) — plus a mixed classic+signal class. Expected shapes verified byte-for-byte against @angular/compiler-cli 21.2.14. * refactor(class_metadata): require a locator when synthesizing query decorators A signal query with no locator (`viewChild()`) is invalid (ngc errors); skip synthesis instead of emitting a malformed decorator that treats the options object as the predicate. No behavior change for valid queries. Also drops a stale duplicate comment. * style: cargo fmt
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
2 participants
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.
Summary
Brings OXC's
setClassMetadataemission into byte-exact parity with Angular'sngc, so TestBed JIT recompilation works without users having to discover and flip a flag.Three threads of work:
Inline
templateUrl/styleUrls/styleUrlinto the metadata args (d1d3ac8). Pre-existing emission preserved the raw resource literals, so Angular'scomponentNeedsResolution(metadata)check tripped at TestBed time ('Component is not resolved: templateUrl…').Default
emit_class_metadatatotrueat all three layers (Rust crate, NAPI binding, Vite plugin) —ngcalways emits the call wrapped in(typeof ngDevMode === "undefined" || ngDevMode) && …, so production bundles tree-shake it. Off-by-default silently broke TestBed/Vitest setups.Match
ngc's exact semantics, found via deeper audit:Map.delete('templateUrl')+Map.set('template', …)ordering (append at end, not in-place) — the e2e compare does string-equality of the decorators payload so the in-place divergence would have silently miscompared.templateandtemplateUrl.Component(matches Angular'sif (dec.name !== 'Component') return dec;) — without this,@Inject({ templateUrl: '…' })had its keys silently stripped.resolve_templateprecedence:templateUrlwins over inlinetemplate(matchesparseTemplateDeclaration); OXC was preferring inline, which is JIT's behavior, not AOT.The Rust port now reads as a near-line-by-line mirror of
transformDecoratorResources, with citations to the upstream source in the doc comments.Empirical ngc parity
All 686 non-skipped fixtures across 22 categories of the e2e ngc-compare suite match byte-for-byte: animations, bindings, class-metadata, control-flow, defer, edge-cases, host-bindings, host-directives, i18n, pipes, providers, regressions, schemas, styles, templates, etc.
Test plan
cargo check --all-featurescargo test(21 binaries, 2685+ tests, all ok)cargo fmt --all -- --checkcargo run -p oxc_angular_conformance && git diff --exit-code(1252/1252, git-clean)pnpm build-dev+pnpm --filter ./napi/angular-compiler build:tspnpm test(193/193 vitest)pnpm check(oxfmt + oxlint, clean)pnpm test:e2e(34/34 playwright)pnpm --filter @oxc-angular/compare compare --fixtures(686/686 against ngc 21.2.14)Notes for reviewers
setClassMetadataby default. Production strips it viangDevMode. Anyone who actively wants the old behavior passesemitClassMetadata: false.setClassMetadatablock.test_standalone_component_omits_standalone_field) to scope itscontains("standalone:true")check to theɵɵdefineComponentliteral — the metadata block faithfully preserves the sourcestandalone: true, which is what ngc does.@Component({ ...config, … })) pass through unchanged; fixing requires structural pre-extraction spread resolution upstream of metadata.