router icon indicating copy to clipboard operation
router copied to clipboard

types: navigating to a union of routes including invalid paths (#5466)

Open mdm317 opened this issue 2 months ago • 3 comments

fix #5466

https://github.com/TanStack/router/blob/4693e0e3d4340238a79f26a2a9b102b4e670ea2a/packages/router-core/src/link.ts#L222-L229

When TResolvedPath is a union such as "/invoices" | "/wrong". In this case, checking it directly with (TResolvedPath & RouteToPath<TRouter> extends never does not filter out invalid paths as expected. To properly remove invalid routes like "/wrong", we need to distribute types

Summary by CodeRabbit

  • Tests

    • Added comprehensive type-level validation for route path navigation with union types across various route configurations.
  • Refactor

    • Improved type inference for path navigation options to enhance IDE support and type accuracy.

mdm317 avatar Oct 19 '25 17:10 mdm317

Walkthrough

The PR introduces TypeScript type-level tests for Link component path validation in union routing scenarios, and refactors the ToPathOption type signature to use an intermediate type parameter for more consistent type inference across absolute and relative paths.

Changes

Cohort / File(s) Summary
Type Tests
packages/react-router/tests/link.test-d.tsx
Added two new type-test blocks validating Link component behavior when navigating to unions of routes with invalid paths, ensuring proper type inference for the to property across various route configurations.
Core Type Signature
packages/router-core/src/link.ts
Modified ToPathOption type to introduce intermediate TRelativeToPathAutoComplete type parameter, conditionally computed based on whether TTo is a generic placeholder, improving type inference consistency for path resolution in union scenarios.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The changes are focused and cohesive, affecting a single runtime declaration and its corresponding tests. While the type-level logic requires TypeScript expertise, the actual modifications are straightforward—a single conditional type parameter introduced and applied consistently. The test additions are formulaic and duplicate existing patterns.

Poem

🐰 A hop through union types so fine,
Where paths and routes align divine,
No more the shadows hide mistakes,
Relative or absolute—truthfulness awakes!
Through TypeScript's mystic halls we gleam,
Consistency flows in every beam. ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "types: navigating to a union of routes including invalid paths (#5466)" clearly and accurately describes the primary change in the changeset. The title directly corresponds to the core issue being addressed—fixing TypeScript type checking for union route types that include invalid paths—as documented in the linked issue #5466. The title is concise, specific, and avoids vague terminology, making it immediately clear to reviewers scanning commit history what the PR accomplishes.
Linked Issues Check ✅ Passed The changes directly address the objectives from linked issue #5466. The type signature modification to ToPathOption in packages/router-core/src/link.ts implements the conditional type distribution logic needed to properly filter invalid paths from union types, which was the root cause of TypeScript's inconsistent error reporting. The new type-test blocks added to packages/react-router/tests/link.test-d.tsx validate that TypeScript now correctly catches invalid routes in union scenarios for both absolute and relative paths, confirming the fix resolves the reported inconsistency.
Out of Scope Changes Check ✅ Passed All changes in this pull request are directly in-scope and related to the objectives of issue #5466. The modifications to the ToPathOption type in packages/router-core/src/link.ts and the type-test additions in packages/react-router/tests/link.test-d.tsx both directly support the goal of fixing TypeScript's type checking for union routes with invalid paths. No unrelated refactoring, formatting changes, or tangential features are present in the changeset.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4693e0e3d4340238a79f26a2a9b102b4e670ea2a and 8841f7e9e3e40055e9c1c64cdc4ed0b1fdb82107.

📒 Files selected for processing (2)
  • packages/react-router/tests/link.test-d.tsx (1 hunks)
  • packages/router-core/src/link.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TypeScript in strict mode with extensive type safety across the codebase

Files:

  • packages/router-core/src/link.ts
  • packages/react-router/tests/link.test-d.tsx
packages/router-core/**

📄 CodeRabbit inference engine (AGENTS.md)

Keep framework-agnostic core router logic in packages/router-core/

Files:

  • packages/router-core/src/link.ts
packages/{react-router,solid-router}/**

📄 CodeRabbit inference engine (AGENTS.md)

Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/

Files:

  • packages/react-router/tests/link.test-d.tsx
🧬 Code graph analysis (1)
packages/router-core/src/link.ts (1)
packages/router-core/src/utils.ts (2)
  • NoInfer (5-5)
  • ConstrainLiteral (135-137)
🔇 Additional comments (2)
packages/react-router/tests/link.test-d.tsx (1)

2372-2429: LGTM! Comprehensive test coverage for union type path filtering.

The new test block effectively validates that TypeScript correctly filters invalid paths from unions in all navigation scenarios (relative, absolute, and parent paths). This directly addresses issue #5466 and ensures the type fix works as expected.

packages/router-core/src/link.ts (1)

614-625: Excellent fix using distributive conditional types.

The introduction of the intermediate TRelativeToPathAutoComplete type parameter with TTo extends any ? ... : never is the correct approach to handle union types. This ensures that each member of a union in TTo (e.g., "/invoices" | "/wrong") is individually validated against valid routes, properly filtering out invalid paths. The use of NoInfer prevents unwanted type inference, maintaining type safety.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Oct 19 '25 17:10 coderabbitai[bot]

Small bump, I guess this PR has been swallowed in the day to day work

balzdur avatar Nov 18 '25 09:11 balzdur

View your CI Pipeline Execution ↗ for commit f4983566272bdf39415907d182cc1749f6ee7290

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 10m 13s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 25s View ↗

☁️ Nx Cloud last updated this comment at 2025-11-19 17:47:04 UTC

nx-cloud[bot] avatar Nov 19 '25 17:11 nx-cloud[bot]