refactor(router-core): buildLocation performance by simplifying 'from' location parsing #6464
Conversation
…' location parsing
|
View your CI Pipeline Execution ↗ for commit 540f118
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Router
participant MatchLight as matchRoutesLightweight
participant ParamExt as extractStrictParams
Caller->>Router: buildLocation(currentLocation)
Router->>MatchLight: matchRoutesLightweight(location)
MatchLight-->>Router: { matchedRoutes, fullPath, search, params }
Router->>ParamExt: extractStrictParams(params, routeDefs)
ParamExt-->>Router: parsedParams or PathParamError
Router-->>Caller: built Location (using lightweight result + parsedParams)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 1696-1716: The lightweight param-parsing loop in
matchRoutesInternal currently calls route.options.params?.parse (parseParams)
even for routes marked skipRouteOnParseError, which breaks the original
semantics; modify the matchedRoutes loop so that you first check
route.options.skipRouteOnParseError and, if true, skip invoking parseParams
entirely and instead copy pre-parsed values from parsedParams into
accumulatedParams (as the existing else branch does), otherwise proceed to call
parseParams and merge results into accumulatedParams; update logic around
accumulatedParams, parseParams, parsedParams, and
route.options.skipRouteOnParseError to match the behavior used during
buildLocation.
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
1606-1609: Update stale comment on context execution.The guard now only checks
!existingMatch, so the comment about buildLocation no longer reflects the actual condition. Consider updating or removing it to avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 1680-1684: The loop that calls extractStrictParams over
matchedRoutes lacks error handling and can let parseParams exceptions bubble up
and crash buildLocation; wrap the body that calls extractStrictParams (the loop
iterating matchedRoutes which assigns strictParams and later sets params) in a
try-catch, mirror the pattern used in matchRoutesInternal around
extractStrictParams, catch and rethrow or return a controlled error (including
route identity/context) so buildLocation can handle malformed URL params
gracefully; update the scope where params = strictParams to only run if no
exception was thrown.
♻️ Duplicate comments (1)
packages/router-core/src/router.ts (1)
1652-1667: Consider extracting shared search accumulation logic.This search validation loop mirrors the pattern in
matchRoutesInternal. As noted in past reviews, extracting a shared helper could reduce duplication.
packages/router-core/src/router.ts
Outdated
| // Accumulate search validation through the route chain | ||
| const accumulatedSearch: Record<string, unknown> = applySearchMiddleware({ | ||
| search: { ...location.search }, | ||
| dest: location, | ||
| destRoutes: matchedRoutes, | ||
| _includeValidateSearch: true, | ||
| }) | ||
| // I don't know if we should run the full search middleware chain, or just validateSearch | ||
| // let accumulatedSearch = { ...location.search } | ||
| // for (const route of matchedRoutes) { | ||
| // try { | ||
| // Object.assign( | ||
| // accumulatedSearch, | ||
| // validateSearch(route.options.validateSearch, accumulatedSearch), | ||
| // ) | ||
| // } catch { | ||
| // // Ignore errors, we're not actually routing | ||
| // } | ||
| // } |
There was a problem hiding this comment.
@schiller-manuel any idea if we should just run the validateSearch or if we need to run the full applySearchMiddleware (with _includeValidateSearch) when building the from data for buildLocation?
I do know that we need to run one of them, but idk which one.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 1652-1669: matchRoutesLightweight currently calls
applySearchMiddleware (with _includeValidateSearch: true) against the current
location which runs full custom search middleware on the "from" location and can
double‑apply non‑idempotent logic; instead run only validation like the
commented loop: iterate matchedRoutes and call
validateSearch(route.options.validateSearch, accumulatedSearch) catching errors
and merging results into accumulatedSearch (using Object.assign) so only
validation runs for the current location and buildLocation can still run the
full middleware once for the destination; reference applySearchMiddleware,
matchRoutesLightweight, validateSearch, buildLocation, matchedRoutes and
location.search when making the change.
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
3096-3104: Tightendesttyping to keep TypeScript safety.
dest: { search?: unknown }weakens type guarantees aroundsearch(e.g., updater vs object). Consider a small union that preserves the expected shapes while still accepting ParsedLocation.♻️ Suggested typing refinement
+type SearchDest = { + search?: true | Updater<unknown> | Record<string, unknown> +} function applySearchMiddleware({ search, dest, destRoutes, _includeValidateSearch, }: { search: any - dest: { search?: unknown } + dest: SearchDest destRoutes: ReadonlyArray<AnyRoute> _includeValidateSearch: boolean | undefined }) {
Sheraff
deleted the
refactor-router-core-build-location-from-parsing-performance
branch
The
buildLocationfunction needs some information about thefromlocation.Before this PR, to get this information we would build a full match branch through
matchRoutes, but with a_buildLocation: trueargument that skipped some things.This PR proposes that we implement a fully separate function that can skip much more work. We also allow for the case where
fromis the current location to short-circuit search validation and params validation.Before
After
Summary by CodeRabbit
Breaking Changes
Performance Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.