Skip to content

fix(di): resolve T | null constructor param types as injection tokens#297

Merged
Brooooooklyn merged 1 commit into
mainfrom
fix/di-nullable-token-resolution
May 26, 2026
Merged

fix(di): resolve T | null constructor param types as injection tokens#297
Brooooooklyn merged 1 commit into
mainfrom
fix/di-nullable-token-resolution

Conversation

@Brooooooklyn
Copy link
Copy Markdown
Member

@Brooooooklyn Brooooooklyn commented May 26, 2026

Summary

  • Resolves issue DI: T | null constructor param types not narrowed for token resolution #285. @Optional() svc: MyService | null (and the bare svc: MyService | null variant) silently emitted ɵɵinvalidFactoryDep, which throws at instantiation — the canonical optional-DI pattern was unusable.
  • Root cause: five copies of extract_param_token (component / directive / injectable / pipe / ng_module) plus two helpers in class_metadata/builders.rs only matched bare TSTypeReference. Any union annotation fell through to None.
  • Adds shared helper util::resolve_di_token_type that mirrors Angular's typeReferenceToExpression in compiler-cli/src/ngtsc/reflection/src/typescript.ts — filters null keyword variants out of unions and narrows to the sole remaining type, transparently unwrapping TSParenthesizedType so (MyService | null), (MyService) | null, and MyService | (null) all resolve. undefined is intentionally preserved to keep T | undefined ambiguous, matching the reference.
  • Threads the helper through all seven call sites.

Test plan

  • 9 new regression tests across component / directive / injectable / pipe / ng_module — covering T | null, null | T, parenthesized forms, with and without @Optional().
  • cargo test -p oxc_angular_compiler — 1044 lib tests pass (+ all integration tests).
  • cargo run -p oxc_angular_conformance — 1252/1252 conformance cases pass.
  • Pre-existing optional-DI tests (?: T suffix syntax) still pass; no behavior change for non-union annotations.
  • Codex adversarial review caught a follow-up gap (parenthesized types) which is fixed in the same commit.

🤖 Generated with Claude Code


Note

Medium Risk
Touches DI token resolution across all Angular decorator kinds and class metadata; behavior change is scoped to union/nullable annotations but incorrect narrowing could still mis-emit factory dependencies.

Overview
Fixes #285: constructor params typed as MyService | null (with or without @Optional()) no longer lose their injection token and emit ɵɵinvalidFactoryDep at runtime.

A shared resolve_di_token_type helper (aligned with Angular’s reference typeReferenceToExpression) strips null from unions, unwraps parenthesized types, and leaves T | undefined ambiguous. All extract_param_token paths (component, directive, injectable, pipe, ng_module) and class metadata type extraction now use it instead of matching only bare TSTypeReference.

Regression tests cover union order, optional vs non-optional, and parenthesized forms across decorator kinds.

Reviewed by Cursor Bugbot for commit 78dd5b7. Bugbot is set up for automated code reviews on this repo. Configure here.

`@Optional() svc: MyService | null` (and the bare `svc: MyService | null`
variant) silently emitted `ɵɵinvalidFactoryDep`, which throws at
instantiation. All five token extractors and both class-metadata helpers
only matched bare `TSTypeReference`, so any union annotation fell through.

Adds `util::resolve_di_token_type` which mirrors Angular's
`typeReferenceToExpression`: filters `null` keyword variants out of unions
and narrows to the sole remaining type, while transparently unwrapping
`TSParenthesizedType` so `(MyService | null)`, `(MyService) | null`, and
`MyService | (null)` all resolve. `undefined` is intentionally preserved
to match the reference compiler.

Closes #285

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@Brooooooklyn Brooooooklyn merged commit 1f290f0 into main May 26, 2026
10 checks passed
@Brooooooklyn Brooooooklyn deleted the fix/di-nullable-token-resolution branch May 26, 2026 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant