-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
refactor(router-core): skip full matchRoutesInternal for Link's buildLocation, use simple getMatchedRoutes #6445
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
refactor(router-core): skip full matchRoutesInternal for Link's buildLocation, use simple getMatchedRoutes #6445
Conversation
…Location, use simple getMatchedRoutes
📝 WalkthroughWalkthroughReplaces full-match construction in buildLocation with lightweight getMatchedRoutes to obtain destRoutes/rawParams, centralizes global-not-found determination via a new findGlobalNotFoundRouteId, threads globalNotFoundRouteId into buildMatchSnapshotFromRoutes, and updates framework useParams test expectations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as Router.buildLocation
participant Matcher as getMatchedRoutes
participant Finder as findGlobalNotFoundRouteId
participant Snapshot as buildMatchSnapshotFromRoutes
participant Location as LocationBuilder
Client->>Router: navigate(request)
Router->>Matcher: getMatchedRoutes(destPath)
Note right of Matcher: returns destRoutes + rawParams
Router->>Finder: findGlobalNotFoundRouteId(notFoundMode, destRoutes)
Finder-->>Router: globalNotFoundRouteId
Router->>Snapshot: buildMatchSnapshotFromRoutes(routes, params, searchStr, globalNotFoundRouteId)
Snapshot-->>Router: match snapshot
Router->>Location: assemble final Location with snapshot
Location-->>Client: navigation completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit 99c67fe
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 1765-1788: In buildLocation the snapshot uses getMatchedRoutes
(destMatchResult -> destRoutes) which doesn't append a notFound route, so
snapshots can omit the 404 route; update buildLocation to mirror
matchRoutesInternal by appending this.options.notFoundRoute to destRoutes when
present and only compute globalNotFoundRouteId via
findGlobalNotFoundRouteId(this.options.notFoundMode, destRoutes) when there is
no custom this.options.notFoundRoute; ensure references to destRoutes,
globalNotFoundRouteId, this.options.notFoundRoute, getMatchedRoutes, and
findGlobalNotFoundRouteId are adjusted so the snapshot includes the notFound
route and still falls back to globalNotFoundRouteId when no custom notFoundRoute
exists.
…uild-for-location-only-link
|
Comparing the new code (16:07:01) vs main branch (16:13:51): Overview Comparison
Hotspot Comparison
What ChangedDropped from main's top 10 in new code:
New in new code's top 10:
Line Number Shifts
Summary
Bottom line: The new code shows significant improvements across most metrics:
The only regression is |
…uild-for-location-only-link
SSR benchmarks for 100 parallel connections, fetching pages w/ 100 links
Before:
After
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.