Skip to content

Commit 2da2fc2

Browse files
Brooooooklynclaude
andcommitted
fix(compiler): recognise Angular class decorators by their @angular/core import, like ngtsc
`@Component`, `@Directive`, `@Pipe`, `@Injectable` and `@NgModule` were matched by name. Like ngtsc's `findAngularDecorator`, a class decorator is now Angular's only when it's imported from `@angular/core`: by name under any alias (`import {Component as Cmp}`), or through a namespace import (`@ng.Component()`). This is the rule #504 uses for member decorators, and the same check (`angular_core_decorator`). Checked against @angular/compiler-cli 22.1.7 (44 probes in tests/fixtures/class_decorators_ngtsc.json, 40 compared): - Another library's, a local, an undeclared or a default-imported `@Component` (and the other four) is left on the class, which isn't compiled, and its `inputs:` aren't checked. - One re-exported through another module (`import {Component} from './shared'`) is left alone too: ngtsc follows the re-export, oxc can't. - An aliased one (`@Cmp`, `@D`, `@P`, `@M`, `@Inj`) and a namespaced `@ng.Injectable` are now compiled; `setClassMetadata` lists them as written (`type: Cmp`). An aliased `@Component` keeps `templateUrl` in `setClassMetadata`, as ngtsc does. - `import {Injectable as Component}` compiles as an `@Injectable`. This holds for AOT compilation and decorator removal, setClassMetadata, the `@Service` collision check, JIT, the hoisting of declarations a decorator references, NAPI `extractComponentMetadataSync` / `extractComponentUrls` / the pipe and class-metadata APIs, and the Vite plugin: its quick decorator check lets `@Cmp(...)` / `@ng.Component(...)` files through, and the HMR template/styles diff only blanks the components the compiler compiled. The public `extract_pipe_metadata`, `extract_injectable_metadata`, `extract_ng_module_metadata` and `find_*_decorator_span`, which don't get the file's imports, still match by name (`extract_pipe_metadata_in` takes them). Not matched to ngtsc, on purpose: an aliased `@Injectable` gets a factory (ngtsc's `needsFactory` compares the written name with 'Injectable', so its `ɵprov` points at a missing `ɵfac`). 117 unit-test sources that used `@Component` / `@Directive` without importing it now import it from `@angular/core`. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 1d7937a commit 2da2fc2

22 files changed

Lines changed: 1479 additions & 337 deletions

‎crates/oxc_angular_compiler/src/component/decorator.rs‎

Lines changed: 98 additions & 34 deletions
Large diffs are not rendered by default.

‎crates/oxc_angular_compiler/src/component/hoist.rs‎

Lines changed: 30 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ use oxc_semantic::Semantic;
4444
use oxc_span::GetSpan;
4545
use oxc_syntax::symbol::SymbolId;
4646

47+
use crate::directive::StringConsts;
4748
use crate::optimizer::Edit;
4849

4950
use super::transform::{ImportMap, is_angular_core_export, is_angular_core_namespace};
@@ -98,6 +99,7 @@ pub fn collect_hoist_edits<'a>(
9899
source: &str,
99100
semantic: &Semantic<'a>,
100101
import_map: &ImportMap<'a>,
102+
consts: &StringConsts<'_>,
101103
) -> Vec<Edit> {
102104
// Step 1: index top-level bindings (keyed by SymbolId).
103105
// - `symbol_to_stmt`: binding SymbolId → containing statement's `start`.
@@ -162,7 +164,7 @@ pub fn collect_hoist_edits<'a>(
162164
let mut classes: Vec<(&Class<'a>, u32, HashSet<SymbolId>, HashSet<SymbolId>)> = Vec::new();
163165
for stmt in &program.body {
164166
let Some((class, stmt_start_pos)) = class_of(stmt) else { continue };
165-
if !has_hoistable_angular_decorator(class, import_map) {
167+
if !has_hoistable_angular_decorator(class, import_map, consts) {
166168
continue;
167169
}
168170
let mut direct: HashSet<SymbolId> = HashSet::new();
@@ -887,81 +889,58 @@ fn class_of<'a, 'src>(stmt: &'src Statement<'a>) -> Option<(&'src Class<'a>, u32
887889
}
888890
}
889891

890-
/// Does this class carry any decorator that Angular's compiler emits eager
891-
/// definitions for? We don't try to be precise here — any of the well-known
892-
/// Angular decorators makes the class a candidate.
893-
///
894-
/// Name-only and import-agnostic: used by the cheap pre-check
895-
/// [`program_has_angular_decorated_class`], where a false positive only costs
896-
/// a wasted scan. The actual hoist filter uses the import-aware
897-
/// [`has_hoistable_angular_decorator`].
898-
fn has_angular_decorator(class: &Class<'_>) -> bool {
899-
class.decorators.iter().any(|d| {
900-
let callee = match &d.expression {
901-
Expression::CallExpression(call) => &call.callee,
902-
expr => expr,
903-
};
904-
let name = match callee {
905-
Expression::Identifier(id) => id.name.as_str(),
906-
Expression::StaticMemberExpression(member) => member.property.name.as_str(),
907-
_ => return false,
908-
};
909-
matches!(name, "Component" | "Directive" | "Pipe" | "NgModule" | "Injectable" | "Service")
910-
})
911-
}
912-
913892
/// Whether this class carries an Angular decorator that warrants hoisting its
914-
/// referenced declarations.
915-
///
916-
/// Like [`has_angular_decorator`], but verifies `@Service` resolves to
917-
/// `@angular/core` before treating it as Angular. `Service` is a common name
918-
/// in non-Angular code, and hoisting a third-party `@Service` class's
919-
/// referenced declarations would reorder statements and change that class's
920-
/// runtime evaluation semantics. The other decorator names predate this check
921-
/// and stay name-only — over-triggering there is the long-standing behavior and
922-
/// only ever hoists a TDZ-safe declaration earlier.
923-
fn has_hoistable_angular_decorator<'a>(class: &Class<'a>, import_map: &ImportMap<'a>) -> bool {
893+
/// referenced declarations: one the compiler compiles the class for, imported
894+
/// from `@angular/core` (see [`crate::directive::angular_class_decorator`]),
895+
/// or an `@Service` that resolves to `@angular/core`. A same-named decorator
896+
/// from another library (`Service` is a common name in non-Angular code)
897+
/// leaves the class alone, and hoisting its referenced declarations would
898+
/// reorder statements and change that class's runtime evaluation semantics.
899+
fn has_hoistable_angular_decorator<'a>(
900+
class: &Class<'a>,
901+
import_map: &ImportMap<'a>,
902+
consts: &StringConsts<'_>,
903+
) -> bool {
924904
class.decorators.iter().any(|d| {
905+
if crate::directive::angular_class_decorator(d, consts).is_some() {
906+
return true;
907+
}
925908
let callee = match &d.expression {
926909
Expression::CallExpression(call) => &call.callee,
927910
expr => expr,
928911
};
929912
match callee {
930913
Expression::Identifier(id) => {
931-
let name = id.name.as_str();
932-
if name == "Service" {
933-
is_angular_core_export(import_map, name, "Service")
934-
} else {
935-
matches!(name, "Component" | "Directive" | "Pipe" | "NgModule" | "Injectable")
936-
}
914+
id.name == "Service" && is_angular_core_export(import_map, &id.name, "Service")
937915
}
916+
// `@ns.Service()` — accept only when `ns` is a namespace import
917+
// from `@angular/core`.
938918
Expression::StaticMemberExpression(member) => {
939-
let name = member.property.name.as_str();
940-
if name == "Service" {
941-
// `@ns.Service()` — accept only when `ns` is a namespace
942-
// import from `@angular/core`.
943-
matches!(&member.object, Expression::Identifier(ns)
919+
member.property.name == "Service"
920+
&& matches!(&member.object, Expression::Identifier(ns)
944921
if is_angular_core_namespace(import_map, ns.name.as_str()))
945-
} else {
946-
matches!(name, "Component" | "Directive" | "Pipe" | "NgModule" | "Injectable")
947-
}
948922
}
949923
_ => false,
950924
}
951925
})
952926
}
953927

954928
/// Cheap pre-check: does `program` contain any top-level class statement
955-
/// carrying one of the Angular decorators recognized by [`has_angular_decorator`]?
929+
/// carrying one of the Angular decorators recognized by
930+
/// [`has_hoistable_angular_decorator`]?
956931
///
957932
/// Used by the AOT transform pipeline to skip the `Semantic` build and the
958933
/// full hoist scan for files with no decorated classes (plain TS helpers,
959934
/// type-only modules, classes with no Angular decorator, …). This walks
960935
/// `program.body` only and never descends into class bodies or expressions,
961936
/// so it's O(top-level statements) with a tiny per-statement cost.
962-
pub(crate) fn program_has_angular_decorated_class(program: &Program<'_>) -> bool {
937+
pub(crate) fn program_has_angular_decorated_class<'a>(
938+
program: &Program<'a>,
939+
import_map: &ImportMap<'a>,
940+
consts: &StringConsts<'_>,
941+
) -> bool {
963942
program.body.iter().any(|stmt| match class_of(stmt) {
964-
Some((class, _)) => has_angular_decorator(class),
943+
Some((class, _)) => has_hoistable_angular_decorator(class, import_map, consts),
965944
None => false,
966945
})
967946
}

0 commit comments

Comments
 (0)