refactor(router-core): reduce buildLocation object allocations #4981
Conversation
WalkthroughbuildLocation in Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Router as buildLocation
Caller->>Router: buildLocation(from, dest)
Router->>Router: Create/assign nextParams (mutate via Object.assign)
Router->>Router: Interpolate path using mutated nextParams
Router->>Router: Stringify params (mutate nextParams further)
Router->>Router: Validate/merge search into validatedSearch (mutate)
Router-->>Caller: Return { pathname, search, params }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit e4422ff
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/router-core/src/router.ts (3)
1498-1504: Guard against non-object returns from params.stringify and reduce subtle bugsIf a route's
params.stringifyaccidentally returns a primitive or array,Object.assignmay behave unexpectedly (e.g., copy string indices). A tiny guard (dev-only warning) keeps behavior sane without hurting perf.Apply within this hunk:
- for (const route of destRoutes) { - const fn = - route.options.params?.stringify ?? route.options.stringifyParams - if (fn) { - Object.assign(nextParams, fn(nextParams)) - } - } + for (const route of destRoutes) { + const fn = + route.options.params?.stringify ?? route.options.stringifyParams + if (!fn) continue + const res = fn(nextParams) + if (res && typeof res === 'object' && !Array.isArray(res)) { + Object.assign(nextParams, res) + } else if (process.env.NODE_ENV !== 'production') { + // Help consumers catch mistakes early + console.warn( + 'params.stringify should return a plain object. Received:', + res, + ) + } + }
1521-1534: Prefer for..of + explicit typing for validatedSearch; maintains order and trims allocationsMinor polish that aligns with the PR’s intent and tightens types.
Apply within this hunk:
- const validatedSearch = {} - destRoutes.forEach((route) => { - if (route.options.validateSearch) { - try { - Object.assign( - validatedSearch, - validateSearch(route.options.validateSearch, { - ...validatedSearch, - ...nextSearch, - }), - ) - } catch { - // ignore errors here because they are already handled in matchRoutes - } - } - }) + const validatedSearch: Record<string, unknown> = {} + for (const route of destRoutes) { + const validator = route.options.validateSearch + if (!validator) continue + try { + Object.assign( + validatedSearch, + validateSearch(validator, { + ...validatedSearch, + ...nextSearch, + }) ?? undefined, + ) + } catch { + // ignore errors here because they are already handled in matchRoutes + } + }
1475-1484: Param merge now mutates the base object; check type allowances and caller assumptionsWe’re now doing an in-place merge of
fromParams, so:
- Ensure no downstream code relies on the old
fromParamsobject identity.- At runtime we special-case
dest.params === false || dest.params === null
but TS only lets you passtrue | Updater<…>. Let’s widen the types of all four fields to match the behavior.Targets in packages/router-core/src/router.ts (around line 443 and inside
mask):export interface BuildNextOptions { to?: string | number | null - params?: true | Updater<unknown> + params?: true | false | null | Updater<unknown> - search?: true | Updater<unknown> + search?: true | false | Updater<unknown> - hash?: true | Updater<string> + hash?: true | false | Updater<string> - state?: true | NonNullableUpdater<ParsedHistoryState, HistoryState> + state?: true | false | NonNullableUpdater<ParsedHistoryState, HistoryState> mask?: { - params?: true | Updater<unknown> + params?: true | false | null | Updater<unknown> - search?: true | Updater<unknown> + search?: true | false | Updater<unknown> - hash?: true | Updater<string> + hash?: true | false | Updater<string> - state?: NonNullableUpdater<ParsedHistoryState, HistoryState> + state?: true | false | NonNullableUpdater<ParsedHistoryState, HistoryState> unmaskOnReload?: boolean } … }(optional) Double-check that no callers depend on
nextParams !== fromParamsfor change-detection.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/router-core/src/router.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/router-core/src/router.ts (2)
packages/router-core/src/utils.ts (1)
functionalUpdate(195-204)packages/router-core/src/path.ts (1)
interpolatePath(371-481)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (2)
packages/router-core/src/router.ts (2)
1486-1491: Passingparams: nextParamsis safe and avoids an extra fallback allocation
nextParamsis always an object given the preceding logic, so dropping?? {}is correct and avoids a redundant allocation.
1507-1516: Second interpolation correctly uses the (now) stringified paramsReusing the mutated
nextParamsfor the final interpolation maintains behavior and avoids transient objects.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/solid-router/tests/link.test.tsx (1)
3670-3670: Same note as above: avoid structuredClone or add a fallbackUse a shallow clone or a small helper to reduce dependency on structuredClone in the test environment.
Apply the same diffs suggested earlier for these lines as well.
Also applies to: 3677-3677
packages/react-router/tests/link.test.tsx (1)
4102-4102: Same note as above: avoid structuredClone or add a fallbackReplicate the shallow clone or helper approach here for robustness and lower overhead.
Also applies to: 4109-4109
🧹 Nitpick comments (2)
packages/solid-router/tests/link.test.tsx (1)
3607-3607: Prefer a shallow clone or add a fallback instead of structuredClone in testsstructuredClone might not exist in all CI environments; a shallow clone is sufficient here and avoids an unnecessary deep copy.
If you want to keep it minimal, switch to a shallow clone:
- parseParams = structuredClone(params) // clone object, because source will get mutated + parseParams = { ...params } // shallow clone to capture input before mutation- stringifyParams = structuredClone(params) // clone object, because source will get mutated + stringifyParams = { ...params } // shallow clone to capture input before mutationIf you prefer deep cloning, consider a tiny helper with a runtime fallback (added once per file) and use it in both places:
// near the top of the file const cloneParams = <T extends object>(p: T): T => typeof structuredClone === 'function' ? structuredClone(p) : { ...p }Then:
parseParams = cloneParams(params) stringifyParams = cloneParams(params)Please confirm your CI/test runners always expose structuredClone (Node ≥17 or a polyfill) if you keep the current approach.
Also applies to: 3614-3614
packages/react-router/tests/link.test.tsx (1)
4039-4039: Prefer a shallow clone or add a fallback instead of structuredClone in testsTo avoid depending on structuredClone availability (and to keep cloning lightweight in tests), use a shallow clone or a small helper.
Shallow clone option:
- parseParams = structuredClone(params) // clone object, because source will get mutated + parseParams = { ...params } // shallow clone to capture input before mutation- stringifyParams = structuredClone(params) // clone object, because source will get mutated + stringifyParams = { ...params } // shallow clone to capture input before mutationOr add once per file:
const cloneParams = <T extends object>(p: T): T => typeof structuredClone === 'function' ? structuredClone(p) : { ...p }Then:
parseParams = cloneParams(params) stringifyParams = cloneParams(params)Please confirm your CI Node/jsdom environment supports structuredClone if you keep it as-is.
Also applies to: 4046-4046
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/react-router/tests/link.test.tsx(8 hunks)packages/solid-router/tests/link.test.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (12)
packages/solid-router/tests/link.test.tsx (6)
3595-3596: LGTM: simpler input-capture over mocks is a good moveReplacing vi.fn call assertions with direct captured inputs makes the tests less brittle and validates actual values.
3632-3632: LGTM: assertion validates the raw input to stringifyParamsThe check now confirms the exact object given to stringify, aligning with the new in-place mutation semantics.
3641-3641: LGTM: assertion validates the raw input to parseParamsThis ensures params arriving from the URL are captured before any mutation.
3657-3658: LGTM: consistent capture pattern across testsDuplicating the capture pattern here keeps the test suite consistent.
3696-3696: LGTM: assertion validates stringify inputCapturing and asserting the pre-mutation input is correct and future-proof.
3705-3705: LGTM: assertion validates parse inputAsserting the string form received by parseParams improves test fidelity.
packages/react-router/tests/link.test.tsx (6)
4027-4028: LGTM: direct input capture simplifies the testsGood change replacing vi.fn assertions with precise captured inputs.
4064-4064: LGTM: verifies the exact object passed to stringifyParamsMatches the new semantics and removes reliance on call-history mocks.
4073-4073: LGTM: verifies the exact object passed to parseParamsCorrectly asserts the string form from the URL segment.
4089-4091: LGTM: consistent capture variables in the nested params scenarioKeeps the approach uniform across both parse/stringify locations.
4128-4128: LGTM: assertion for stringify input (params.stringify path)Confirms we’re capturing inputs at the right boundary.
4137-4137: LGTM: assertion for parse input (params.parse path)Validates the expected shape and type before any internal mutation.
Sheraff
deleted the
refactor-router-core-build-location-reduce-object-allocation
branch
When we can (and it's reasonable), it's always beneficial to reduce the
number of objects we create in JS, so as to minimize the memory pressure
and the GC work.
`buildLocation` gets called pretty often, so removing a few object
allocations can be very beneficial (especially inside loops).
Also, from a purely "immediate performance" point of view,
`Object.assign` tends to be faster than `{...foo}`.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Refactor**
* Route parameter and search handling now use in-place updates to reduce
allocations and streamline iteration/control flow; behavior and public
API remain unchanged.
* **Tests**
* Updated tests to capture cloned parameter objects and assert on exact
values rather than relying on mock call history, preventing
mutation-related flakiness.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
When we can (and it's reasonable), it's always beneficial to reduce the number of objects we create in JS, so as to minimize the memory pressure and the GC work.
buildLocationgets called pretty often, so removing a few object allocations can be very beneficial (especially inside loops).Also, from a purely "immediate performance" point of view,
Object.assigntends to be faster than{...foo}.Summary by CodeRabbit