Skip to content

Commit c1acbde

Browse files
Brooooooklynclaude
andcommitted
fix: pipe-only imports should not prevent DomOnly mode
Angular's ngtsc (handler.ts:1326-1339) only counts MetaKind.Directive and MetaKind.NgModule as directive dependencies — NOT MetaKind.Pipe. The previous `has_any_import_elements` treated ALL non-empty imports as directive dependencies, causing standalone components with only pipe imports (AsyncPipe, SlicePipe, etc.) to incorrectly use Full mode. Replace with `has_any_non_pipe_import_elements` that uses a naming convention heuristic: identifiers ending in "Pipe" are treated as pipes and do not count as directive dependencies. Non-identifier elements (spread, calls) and non-array expressions conservatively fall back to Full mode. Fixes 11 fixture test failures (100% fixture match rate). No regression in ClickUp comparison (168 mismatched, 97.1%). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent c86d6f3 commit c1acbde

2 files changed

Lines changed: 277 additions & 17 deletions

File tree

crates/oxc_angular_compiler/src/component/decorator.rs

Lines changed: 181 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -132,12 +132,13 @@ pub fn extract_component_metadata<'a>(
132132
metadata.imports = extract_identifier_array(allocator, &prop.value);
133133
// 2. The raw expression to pass to ɵɵgetComponentDepsFactory in RuntimeResolved mode
134134
metadata.raw_imports = convert_oxc_expression(allocator, &prop.value);
135-
// 3. Determine if the imports array has any elements (directive dependencies).
136-
// Angular uses: meta.isStandalone && !meta.hasDirectiveDependencies → DomOnly
137-
// Without type info, we conservatively assume any non-empty import
138-
// could be a directive (not just a pipe).
135+
// 3. Determine if the imports array has any non-pipe elements (directive deps).
136+
// Angular's ngtsc (handler.ts:1326-1339) only counts MetaKind.Directive
137+
// and MetaKind.NgModule — NOT MetaKind.Pipe. Without type info, we use
138+
// a naming convention heuristic: identifiers ending in "Pipe" are pipes.
139139
// See: angular/packages/compiler/src/render3/view/compiler.ts:229-232
140-
metadata.has_directive_dependencies = has_any_import_elements(&prop.value);
140+
metadata.has_directive_dependencies =
141+
has_any_non_pipe_import_elements(&prop.value);
141142
}
142143
"exportAs" => {
143144
// exportAs can be comma-separated: "foo, bar"
@@ -398,24 +399,45 @@ fn extract_string_array<'a>(
398399
Some(result)
399400
}
400401

401-
/// Check if an imports expression has any elements that could be directive dependencies.
402+
/// Check if an imports expression has any non-pipe elements (directive dependencies).
402403
///
403-
/// Returns `true` if:
404-
/// - The expression is a non-empty array literal (may contain directives)
405-
/// - The expression is not an array literal (e.g., a variable reference that could contain anything)
404+
/// Angular's ngtsc (handler.ts:1326-1339) only counts `MetaKind.Directive` and
405+
/// `MetaKind.NgModule` as directive dependencies — NOT `MetaKind.Pipe`.
406406
///
407-
/// Returns `false` only for empty array literals (`imports: []`).
407+
/// Since OXC is a single-file compiler without type information, we use a naming
408+
/// convention heuristic: identifiers ending in "Pipe" are assumed to be pipes
409+
/// and do NOT count as directive dependencies. This matches the universal Angular
410+
/// convention where all pipe classes are named `*Pipe` (AsyncPipe, DatePipe, etc.).
408411
///
409-
/// This is a conservative check: without type info, any non-empty import
410-
/// could be a directive. Angular's ngtsc has full type info to distinguish
411-
/// directives from pipes, but Oxc uses this heuristic.
412-
fn has_any_import_elements(expr: &Expression<'_>) -> bool {
413-
match expr {
414-
Expression::ArrayExpression(arr) => !arr.elements.is_empty(),
412+
/// Returns `true` (has directive dependencies) if:
413+
/// - The expression is not an array literal (e.g., variable reference — conservatively
414+
/// assumed to potentially contain directives)
415+
/// - The array contains any non-identifier element (spread, function call, etc.)
416+
/// - The array contains any identifier that does NOT end in "Pipe"
417+
///
418+
/// Returns `false` if:
419+
/// - The expression is an empty array literal (`imports: []`)
420+
/// - ALL elements in the array are identifiers ending in "Pipe"
421+
fn has_any_non_pipe_import_elements(expr: &Expression<'_>) -> bool {
422+
let Expression::ArrayExpression(arr) = expr else {
415423
// Not an array literal (e.g., variable reference like `imports: MY_IMPORTS`)
416424
// Conservatively assume it may contain directives
417-
_ => true,
425+
return true;
426+
};
427+
for element in &arr.elements {
428+
match element {
429+
ArrayExpressionElement::Identifier(id) => {
430+
if !id.name.ends_with("Pipe") {
431+
return true;
432+
}
433+
}
434+
// Non-identifier elements (spread, call expressions, etc.)
435+
// conservatively treated as potential directives
436+
_ => return true,
437+
}
418438
}
439+
// All elements are identifiers ending in "Pipe", or the array is empty
440+
false
419441
}
420442

421443
/// Extract an array of identifiers (for imports).
@@ -3261,4 +3283,146 @@ mod tests {
32613283
);
32623284
});
32633285
}
3286+
3287+
// =========================================================================
3288+
// Directive dependency detection tests (pipe vs directive imports)
3289+
// =========================================================================
3290+
//
3291+
// Angular's ngtsc (handler.ts:1326-1339) only counts MetaKind.Directive
3292+
// and MetaKind.NgModule as directive dependencies — NOT MetaKind.Pipe.
3293+
// Since OXC is a single-file compiler, we use a naming convention heuristic:
3294+
// identifiers ending in "Pipe" are assumed to be pipes.
3295+
3296+
#[test]
3297+
fn test_pipe_only_imports_no_directive_dependencies() {
3298+
let code = r#"
3299+
@Component({
3300+
selector: 'app-test',
3301+
standalone: true,
3302+
imports: [AsyncPipe],
3303+
template: ''
3304+
})
3305+
class TestComponent {}
3306+
"#;
3307+
assert_metadata(code, |meta| {
3308+
assert!(
3309+
!meta.has_directive_dependencies,
3310+
"Pipe-only imports should not set has_directive_dependencies"
3311+
);
3312+
});
3313+
}
3314+
3315+
#[test]
3316+
fn test_multiple_pipe_imports_no_directive_dependencies() {
3317+
let code = r#"
3318+
@Component({
3319+
selector: 'app-test',
3320+
standalone: true,
3321+
imports: [AsyncPipe, DatePipe, SlicePipe, KeyValuePipe],
3322+
template: ''
3323+
})
3324+
class TestComponent {}
3325+
"#;
3326+
assert_metadata(code, |meta| {
3327+
assert!(
3328+
!meta.has_directive_dependencies,
3329+
"Multiple pipe-only imports should not set has_directive_dependencies"
3330+
);
3331+
});
3332+
}
3333+
3334+
#[test]
3335+
fn test_mixed_pipe_and_directive_imports_has_directive_dependencies() {
3336+
let code = r#"
3337+
@Component({
3338+
selector: 'app-test',
3339+
standalone: true,
3340+
imports: [AsyncPipe, HighlightDirective],
3341+
template: ''
3342+
})
3343+
class TestComponent {}
3344+
"#;
3345+
assert_metadata(code, |meta| {
3346+
assert!(
3347+
meta.has_directive_dependencies,
3348+
"Mixed imports with non-pipe should set has_directive_dependencies"
3349+
);
3350+
});
3351+
}
3352+
3353+
#[test]
3354+
fn test_directive_only_imports_has_directive_dependencies() {
3355+
let code = r#"
3356+
@Component({
3357+
selector: 'app-test',
3358+
standalone: true,
3359+
imports: [HighlightDirective, RouterModule],
3360+
template: ''
3361+
})
3362+
class TestComponent {}
3363+
"#;
3364+
assert_metadata(code, |meta| {
3365+
assert!(
3366+
meta.has_directive_dependencies,
3367+
"Directive-only imports should set has_directive_dependencies"
3368+
);
3369+
});
3370+
}
3371+
3372+
#[test]
3373+
fn test_empty_imports_no_directive_dependencies() {
3374+
let code = r#"
3375+
@Component({
3376+
selector: 'app-test',
3377+
standalone: true,
3378+
imports: [],
3379+
template: ''
3380+
})
3381+
class TestComponent {}
3382+
"#;
3383+
assert_metadata(code, |meta| {
3384+
assert!(
3385+
!meta.has_directive_dependencies,
3386+
"Empty imports should not set has_directive_dependencies"
3387+
);
3388+
});
3389+
}
3390+
3391+
#[test]
3392+
fn test_variable_imports_has_directive_dependencies() {
3393+
let code = r#"
3394+
@Component({
3395+
selector: 'app-test',
3396+
standalone: true,
3397+
imports: MY_IMPORTS,
3398+
template: ''
3399+
})
3400+
class TestComponent {}
3401+
"#;
3402+
assert_metadata(code, |meta| {
3403+
assert!(
3404+
meta.has_directive_dependencies,
3405+
"Variable imports should conservatively set has_directive_dependencies"
3406+
);
3407+
});
3408+
}
3409+
3410+
#[test]
3411+
fn test_spread_in_imports_has_directive_dependencies() {
3412+
let code = r#"
3413+
@Component({
3414+
selector: 'app-test',
3415+
standalone: true,
3416+
imports: [...SHARED_IMPORTS, AsyncPipe],
3417+
template: ''
3418+
})
3419+
class TestComponent {}
3420+
"#;
3421+
assert_metadata(code, |meta| {
3422+
assert!(
3423+
meta.has_directive_dependencies,
3424+
"Spread in imports should conservatively set has_directive_dependencies"
3425+
);
3426+
});
3427+
}
32643428
}

crates/oxc_angular_compiler/tests/integration_test.rs

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3441,6 +3441,102 @@ export class TestComponent {}
34413441
);
34423442
}
34433443

3444+
/// Test that standalone components with ONLY pipe imports use DomOnly mode.
3445+
///
3446+
/// Angular's ngtsc (handler.ts:1326-1339) only counts MetaKind.Directive and
3447+
/// MetaKind.NgModule as directive dependencies — NOT MetaKind.Pipe. Since OXC
3448+
/// is a single-file compiler, we use the naming convention (ending in "Pipe")
3449+
/// to identify pipes.
3450+
#[test]
3451+
fn test_dom_only_mode_used_for_standalone_with_pipe_only_imports() {
3452+
let allocator = Allocator::default();
3453+
let source = r#"
3454+
import { Component } from '@angular/core';
3455+
import { AsyncPipe } from '@angular/common';
3456+
3457+
@Component({
3458+
selector: 'app-test',
3459+
standalone: true,
3460+
imports: [AsyncPipe],
3461+
template: `<div>{{ data$ | async }}</div>`
3462+
})
3463+
export class TestComponent {
3464+
data$ = null;
3465+
}
3466+
"#;
3467+
3468+
let options = ComponentTransformOptions { use_dom_only_mode: true, ..Default::default() };
3469+
let result = transform_angular_file(&allocator, "test.ts", source, &options, None);
3470+
3471+
assert!(
3472+
result.code.contains("ɵɵdomElementStart"),
3473+
"Standalone with pipe-only imports should use ɵɵdomElementStart (DomOnly). Output:\n{}",
3474+
result.code
3475+
);
3476+
assert!(
3477+
!result.code.contains("ɵɵelementStart"),
3478+
"Standalone with pipe-only imports should NOT use ɵɵelementStart. Output:\n{}",
3479+
result.code
3480+
);
3481+
}
3482+
3483+
/// Test that multiple pipe-only imports also use DomOnly mode.
3484+
#[test]
3485+
fn test_dom_only_mode_used_for_standalone_with_multiple_pipe_imports() {
3486+
let allocator = Allocator::default();
3487+
let source = r#"
3488+
import { Component } from '@angular/core';
3489+
import { AsyncPipe, DatePipe, SlicePipe } from '@angular/common';
3490+
3491+
@Component({
3492+
selector: 'app-test',
3493+
standalone: true,
3494+
imports: [AsyncPipe, DatePipe, SlicePipe],
3495+
template: `<div>Hello</div>`
3496+
})
3497+
export class TestComponent {}
3498+
"#;
3499+
3500+
let options = ComponentTransformOptions { use_dom_only_mode: true, ..Default::default() };
3501+
let result = transform_angular_file(&allocator, "test.ts", source, &options, None);
3502+
3503+
assert!(
3504+
result.code.contains("ɵɵdomElementStart"),
3505+
"Multiple pipe-only imports should use DomOnly mode. Output:\n{}",
3506+
result.code
3507+
);
3508+
}
3509+
3510+
/// Test that mixed pipe + directive imports still use Full mode.
3511+
#[test]
3512+
fn test_full_mode_used_for_standalone_with_mixed_imports() {
3513+
let allocator = Allocator::default();
3514+
let source = r#"
3515+
import { Component, Directive } from '@angular/core';
3516+
import { AsyncPipe } from '@angular/common';
3517+
3518+
@Directive({ selector: '[appHighlight]', standalone: true })
3519+
export class HighlightDirective {}
3520+
3521+
@Component({
3522+
selector: 'app-test',
3523+
standalone: true,
3524+
imports: [AsyncPipe, HighlightDirective],
3525+
template: `<div>Hello</div>`
3526+
})
3527+
export class TestComponent {}
3528+
"#;
3529+
3530+
let options = ComponentTransformOptions { use_dom_only_mode: true, ..Default::default() };
3531+
let result = transform_angular_file(&allocator, "test.ts", source, &options, None);
3532+
3533+
assert!(
3534+
result.code.contains("ɵɵelementStart"),
3535+
"Mixed imports should use Full mode. Output:\n{}",
3536+
result.code
3537+
);
3538+
}
3539+
34443540
/// Test that @let declaration in nested @if correctly preserves context variable.
34453541
///
34463542
/// When a `@let` declaration and a subsequent expression both reference the component

0 commit comments

Comments
 (0)