-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(router-core): favor index matches, or deepest non-index #5933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(router-core): favor index matches, or deepest non-index #5933
Conversation
WalkthroughThis PR fixes route matching when optional parameters coexist with layout routes. The changes modify the early-termination condition in Changes
Sequence DiagramsequenceDiagram
participant Router as Route Matcher
participant Old as Old Logic
participant New as New Logic
Note over Router,New: Matching "/posts" with optional {-$category}
rect rgb(200, 220, 240)
Note over Old,New: Perfect match detection
Old->>Old: Check statics === partsLength?
Old->>Old: ❌ Early return (missing index check)
New->>New: Check statics === partsLength && node.isIndex?
New->>New: ✓ Properly gate early termination
end
rect rgb(220, 240, 200)
Note over Old,New: Tie-breaker when dynamics equal
Old->>Old: Limited comparison
New->>New: Compare optionals → depth → index flag
New->>New: ✓ More precise specificity ranking
end
Note over Router: Result: Layout routes + optional params now match correctly
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 6300434
☁️ Nx Cloud last updated this comment at |
Fix #5912
All things being equal, a shorter route should only be favored over a longer one w/ skipped optional segments if that shorter route is an index route. Otherwise, favor deeper routes, even if that means we have to skip many optional segments.
This changes the expectations for 2 of our existing unit tests, but we didn't have a strong opinion on either of those tests:
'/{-$other}/posts/new', '/posts/new'matching path/posts/new=> used to match
/posts/new, with this PR will match/{-$other}/posts/new'/foo/{-$p}.tsx', '/foo/{-$p}/{-$x}.tsx'matching path/foo=> used to match
/foo/{-$p}.tsx, with this PR will match/foo/{-$p}/{-$x}.tsxSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.