Skip to content

fix(safe-nav): version-aware optional chaining for Angular v22+ (#317)#330

Open
brandonroberts wants to merge 1 commit into
mainfrom
fix/317-legacy-optional-chaining
Open

fix(safe-nav): version-aware optional chaining for Angular v22+ (#317)#330
brandonroberts wants to merge 1 commit into
mainfrom
fix/317-legacy-optional-chaining

Conversation

@brandonroberts
Copy link
Copy Markdown
Collaborator

Summary

Angular v22 changed the safe-navigation operator (?.) in template expressions to yield undefined via native optional chaining, gated behind the legacyOptionalChaining compiler option (angular/angular@2896c93cc1). OXC unconditionally emitted the legacy == null ? null ternary, so every ?. expression in a v22+ project got the wrong runtime value (null instead of undefined). This is a silent, time-bombed regression for anyone upgrading to v22.

This PR makes OXC version-aware and faithful to the reference compiler's emitted output.

Closes #317.

What changed

  • legacyOptionalChaining option added to TransformOptions (NAPI + Rust + index.d.ts) and threaded through ingest into both ComponentCompilationJob and HostBindingCompilationJob. The effective value is explicit_override ?? version_default, where the version default is derived from angularVersion:
    • major < 22 → legacy (null)
    • major ≥ 22 → modern (native ?., undefined)
    • unknown → legacy (Angular's conservative fallback)
  • optional flag added to the resolved IR read/call nodes (ResolvedPropertyRead / ResolvedKeyedRead / ResolvedCall) and passed through reify so it renders as native ?. / ?.[] / ?.().
  • expand_safe_reads rewritten to branch per node: legacy builds the SafeTernary (== null ? null); modern rewrites each safe access into the equivalent optional resolved read (no temporaries needed — native ?. evaluates the receiver once).
  • $safeNavigationMigration(...) escape hatch: a wrapped subtree is forced back to legacy null semantics even on a modern target, and the wrapper is stripped. Detected post-resolve_names (where the receiver's names are already resolved), so no new IR node or extra phase was needed.

Deviations from the issue text (both to match the reference compiler)

  • The modern form is native optional chaining (ctx.user?.name), not the == null ? undefined ternary the issue described. Both yield undefined; native ?. is what Angular actually emits.
  • The shipped v22 magic function is $safeNavigationMigration(...), not $null(...) — the referenced commit's message said $null, but the code (in v22.0.0-next.10) named it $safeNavigationMigration.

Out of scope

Partial/linker output keeps legacy semantics for now; threading the legacyOptionalChaining facade field through partial emit is deferred (issue required-work #4). Wiring a user's tsconfig angularCompilerOptions.legacyOptionalChaining → this NAPI option lives in the JS build-plugin layer.

Verification

Output was diffed against Angular's own compliance goldens (r3_view_compiler/safe_access) by running the exact fixture templates through OXC at v22:

  • Modern deep chains match character-for-character (ctx.p?.a?.b?.c?.d, ctx.p?.["a"]?.["b"]..., and the mixed-optional/plain cases).
  • The $safeNavigationMigration legacy expansion reproduces the golden including the same temporary name (tmp_3_0) and chain structure.
  • A template mixing an unwrapped span (modern) and a wrapped span (legacy) reproduced the golden's per-span split.

The only difference anywhere is OXC's emitter wrapping ternaries in extra parentheses — pre-existing style, semantically identical.

Test plan

  • cargo build --workspace (incl. NAPI crate) — clean
  • cargo test -p oxc_angular_compiler — all green; existing safe-nav snapshots unchanged (version-unknown stays legacy)
  • cargo fmt --check — clean
  • Added 6 integration tests: modern interpolation/chain/mixed-chain/call (v22), legacy on v21, and the $safeNavigationMigration escape hatch
  • JS/node-side tests not run (no behavioral changes in the JS layer)

🤖 Generated with Claude Code

Angular v22 changed the safe-navigation operator (`?.`) in template
expressions to yield `undefined` via native optional chaining, gated by
the `legacyOptionalChaining` compiler option. OXC unconditionally emitted
the legacy `== null ? null` ternary, so v22+ projects got the wrong
runtime value for any `?.` expression.

Changes:
- Add `legacyOptionalChaining` to `TransformOptions` (NAPI + Rust) and
  thread it through ingest into the compilation jobs. The effective default
  is derived from `angularVersion`: legacy for < v22, modern (native `?.`)
  for >= v22, and legacy when the version is unknown (matches Angular's
  conservative fallback).
- Add an `optional` flag to the resolved IR read/call nodes
  (`ResolvedPropertyRead`/`ResolvedKeyedRead`/`ResolvedCall`) and pass it
  through reify so it renders as native `?.` / `?.[]` / `?.()`.
- Rewrite `expand_safe_reads` to branch per node: legacy builds the
  `SafeTernary` (`== null ? null`); modern rewrites each safe access into
  the equivalent optional resolved read (no temporaries needed).
- Support the `$safeNavigationMigration(...)` escape hatch: a wrapped
  subtree is forced back to legacy null semantics even on a modern target,
  and the wrapper is stripped.

Two deviations from the issue text, both to match the reference compiler
(angular/angular@2896c93cc1):
- The modern form is native optional chaining (`ctx.user?.name`), not the
  `== null ? undefined` ternary the issue described. Both yield `undefined`
  at runtime; native `?.` matches Angular's emitted output.
- The magic function shipped in v22 is `$safeNavigationMigration(...)`, not
  `$null(...)` (the commit message named `$null` but the code renamed it).

Partial/linker output keeps legacy semantics for now; threading the facade
field through partial emit is deferred (issue required-work #4).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Brooooooklyn
Copy link
Copy Markdown
Member

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1269b230ee

ℹ️ 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".

Comment on lines +210 to +212
call.receiver.as_ref(),
IrExpression::ResolvedPropertyRead(p)
if p.name.as_str() == "$safeNavigationMigration"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restrict migration marker to the top-level helper

This marker check matches any one-argument call whose callee property is named $safeNavigationMigration, so a legitimate template call such as svc.$safeNavigationMigration(user?.name) is stripped and replaced with the argument instead of invoking svc. The magic wrapper should only apply to the unqualified migration helper on the component context; otherwise object methods/properties with the same name are silently dropped in both legacy and modern compilations.

Useful? React with 👍 / 👎.

@Brooooooklyn Brooooooklyn disabled auto-merge June 2, 2026 13:52
@Brooooooklyn Brooooooklyn enabled auto-merge (squash) June 2, 2026 13:52
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.

fix(safe-nav): support legacyOptionalChaining flag — emit undefined-yielding ?. for Angular 22+

2 participants