Skip to content

Commit 84b5451

Browse files
Sheraffautofix-ci[bot]
authored and
tannerlinsley
committed
refactor(router-core): reduce buildLocation object allocations (#4981)
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>
1 parent dfa089e commit 84b5451

3 files changed

Lines changed: 48 additions & 51 deletions

File tree

‎packages/react-router/tests/link.test.tsx‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4024,8 +4024,8 @@ describe('Link', () => {
40244024
),
40254025
})
40264026

4027-
const parseParams = vi.fn()
4028-
const stringifyParams = vi.fn()
4027+
let parseParams: any
4028+
let stringifyParams: any
40294029

40304030
const PostComponent = () => {
40314031
const params = useParams({ strict: false })
@@ -4036,14 +4036,14 @@ describe('Link', () => {
40364036
getParentRoute: () => rootRoute,
40374037
path: '$postId',
40384038
parseParams: (params) => {
4039-
parseParams(params)
4039+
parseParams = structuredClone(params) // clone object, because source will get mutated
40404040
return {
40414041
status: 'parsed',
40424042
postId: params.postId,
40434043
}
40444044
},
40454045
stringifyParams: (params) => {
4046-
stringifyParams(params)
4046+
stringifyParams = structuredClone(params) // clone object, because source will get mutated
40474047
return {
40484048
status: 'stringified',
40494049
postId: params.postId,
@@ -4061,7 +4061,7 @@ describe('Link', () => {
40614061
name: 'Go to post',
40624062
})
40634063

4064-
expect(stringifyParams).toHaveBeenCalledWith({ postId: 2 })
4064+
expect(stringifyParams).toEqual({ postId: 2 })
40654065

40664066
expect(postLink).toHaveAttribute('href', '/2')
40674067

@@ -4070,7 +4070,7 @@ describe('Link', () => {
40704070
const posts2Text = await screen.findByText('Post: 2')
40714071
expect(posts2Text).toBeInTheDocument()
40724072

4073-
expect(parseParams).toHaveBeenCalledWith({ status: 'parsed', postId: '2' })
4073+
expect(parseParams).toEqual({ postId: '2' })
40744074
})
40754075

40764076
test('when navigating to /$postId with params.parse and params.stringify', async () => {
@@ -4086,8 +4086,8 @@ describe('Link', () => {
40864086
),
40874087
})
40884088

4089-
const parseParams = vi.fn()
4090-
const stringifyParams = vi.fn()
4089+
let parseParams: any
4090+
let stringifyParams: any
40914091

40924092
const PostComponent = () => {
40934093
const params = useParams({ strict: false })
@@ -4099,14 +4099,14 @@ describe('Link', () => {
40994099
path: '$postId',
41004100
params: {
41014101
parse: (params) => {
4102-
parseParams(params)
4102+
parseParams = structuredClone(params) // clone object, because source will get mutated
41034103
return {
41044104
status: 'parsed',
41054105
postId: params.postId,
41064106
}
41074107
},
41084108
stringify: (params) => {
4109-
stringifyParams(params)
4109+
stringifyParams = structuredClone(params) // clone object, because source will get mutated
41104110
return {
41114111
status: 'stringified',
41124112
postId: params.postId,
@@ -4125,7 +4125,7 @@ describe('Link', () => {
41254125
name: 'Go to post',
41264126
})
41274127

4128-
expect(stringifyParams).toHaveBeenCalledWith({ postId: 2 })
4128+
expect(stringifyParams).toEqual({ postId: 2 })
41294129

41304130
expect(postLink).toHaveAttribute('href', '/2')
41314131

@@ -4134,7 +4134,7 @@ describe('Link', () => {
41344134
const posts2Text = await screen.findByText('Post: 2')
41354135
expect(posts2Text).toBeInTheDocument()
41364136

4137-
expect(parseParams).toHaveBeenCalledWith({ status: 'parsed', postId: '2' })
4137+
expect(parseParams).toEqual({ postId: '2' })
41384138
})
41394139

41404140
test('when navigating to /$postId with params.parse and params.stringify handles falsey inputs', async () => {

‎packages/router-core/src/router.ts‎

Lines changed: 24 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1480,20 +1480,20 @@ export class RouterCore<
14801480
: this.resolvePathWithBase(fromPath, '.')
14811481

14821482
// Resolve the next params
1483-
let nextParams =
1483+
const nextParams =
14841484
dest.params === false || dest.params === null
14851485
? {}
14861486
: (dest.params ?? true) === true
14871487
? fromParams
1488-
: {
1489-
...fromParams,
1490-
...functionalUpdate(dest.params as any, fromParams),
1491-
}
1488+
: Object.assign(
1489+
fromParams,
1490+
functionalUpdate(dest.params as any, fromParams),
1491+
)
14921492

14931493
// Interpolate the path first to get the actual resolved path, then match against that
14941494
const interpolatedNextTo = interpolatePath({
14951495
path: nextTo,
1496-
params: nextParams ?? {},
1496+
params: nextParams,
14971497
parseCache: this.parsePathnameCache,
14981498
}).interpolatedPath
14991499

@@ -1503,23 +1503,20 @@ export class RouterCore<
15031503

15041504
// If there are any params, we need to stringify them
15051505
if (Object.keys(nextParams).length > 0) {
1506-
destRoutes
1507-
.map((route) => {
1508-
return (
1509-
route.options.params?.stringify ?? route.options.stringifyParams
1510-
)
1511-
})
1512-
.filter(Boolean)
1513-
.forEach((fn) => {
1514-
nextParams = { ...nextParams!, ...fn!(nextParams) }
1515-
})
1506+
for (const route of destRoutes) {
1507+
const fn =
1508+
route.options.params?.stringify ?? route.options.stringifyParams
1509+
if (fn) {
1510+
Object.assign(nextParams, fn(nextParams))
1511+
}
1512+
}
15161513
}
15171514

15181515
const nextPathname = interpolatePath({
15191516
// Use the original template path for interpolation
15201517
// This preserves the original parameter syntax including optional parameters
15211518
path: nextTo,
1522-
params: nextParams ?? {},
1519+
params: nextParams,
15231520
leaveWildcards: false,
15241521
leaveParams: opts.leaveParams,
15251522
decodeCharMap: this.pathParamsDecodeCharMap,
@@ -1529,20 +1526,20 @@ export class RouterCore<
15291526
// Resolve the next search
15301527
let nextSearch = fromSearch
15311528
if (opts._includeValidateSearch && this.options.search?.strict) {
1532-
let validatedSearch = {}
1529+
const validatedSearch = {}
15331530
destRoutes.forEach((route) => {
1534-
try {
1535-
if (route.options.validateSearch) {
1536-
validatedSearch = {
1537-
...validatedSearch,
1538-
...(validateSearch(route.options.validateSearch, {
1531+
if (route.options.validateSearch) {
1532+
try {
1533+
Object.assign(
1534+
validatedSearch,
1535+
validateSearch(route.options.validateSearch, {
15391536
...validatedSearch,
15401537
...nextSearch,
1541-
}) ?? {}),
1542-
}
1538+
}),
1539+
)
1540+
} catch {
1541+
// ignore errors here because they are already handled in matchRoutes
15431542
}
1544-
} catch {
1545-
// ignore errors here because they are already handled in matchRoutes
15461543
}
15471544
})
15481545
nextSearch = validatedSearch

‎packages/solid-router/tests/link.test.tsx‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3592,8 +3592,8 @@ describe('Link', () => {
35923592
),
35933593
})
35943594

3595-
const parseParams = vi.fn()
3596-
const stringifyParams = vi.fn()
3595+
let parseParams: any
3596+
let stringifyParams: any
35973597

35983598
const PostComponent = () => {
35993599
const params = useParams({ strict: false })
@@ -3604,14 +3604,14 @@ describe('Link', () => {
36043604
getParentRoute: () => rootRoute,
36053605
path: '$postId',
36063606
parseParams: (params) => {
3607-
parseParams(params)
3607+
parseParams = structuredClone(params) // clone object, because source will get mutated
36083608
return {
36093609
status: 'parsed',
36103610
postId: params.postId,
36113611
}
36123612
},
36133613
stringifyParams: (params) => {
3614-
stringifyParams(params)
3614+
stringifyParams = structuredClone(params) // clone object, because source will get mutated
36153615
return {
36163616
status: 'stringified',
36173617
postId: params.postId,
@@ -3629,7 +3629,7 @@ describe('Link', () => {
36293629
name: 'Go to post',
36303630
})
36313631

3632-
expect(stringifyParams).toHaveBeenCalledWith({ postId: 2 })
3632+
expect(stringifyParams).toEqual({ postId: 2 })
36333633

36343634
expect(postLink).toHaveAttribute('href', '/2')
36353635

@@ -3638,7 +3638,7 @@ describe('Link', () => {
36383638
const posts2Text = await screen.findByText('Post: 2')
36393639
expect(posts2Text).toBeInTheDocument()
36403640

3641-
expect(parseParams).toHaveBeenCalledWith({ status: 'parsed', postId: '2' })
3641+
expect(parseParams).toEqual({ postId: '2' })
36423642
})
36433643

36443644
test('when navigating to /$postId with params.parse and params.stringify', async () => {
@@ -3654,8 +3654,8 @@ describe('Link', () => {
36543654
),
36553655
})
36563656

3657-
const parseParams = vi.fn()
3658-
const stringifyParams = vi.fn()
3657+
let parseParams: any
3658+
let stringifyParams: any
36593659

36603660
const PostComponent = () => {
36613661
const params = useParams({ strict: false })
@@ -3667,14 +3667,14 @@ describe('Link', () => {
36673667
path: '$postId',
36683668
params: {
36693669
parse: (params) => {
3670-
parseParams(params)
3670+
parseParams = structuredClone(params) // clone object, because source will get mutated
36713671
return {
36723672
status: 'parsed',
36733673
postId: params.postId,
36743674
}
36753675
},
36763676
stringify: (params) => {
3677-
stringifyParams(params)
3677+
stringifyParams = structuredClone(params) // clone object, because source will get mutated
36783678
return {
36793679
status: 'stringified',
36803680
postId: params.postId,
@@ -3693,7 +3693,7 @@ describe('Link', () => {
36933693
name: 'Go to post',
36943694
})
36953695

3696-
expect(stringifyParams).toHaveBeenCalledWith({ postId: 2 })
3696+
expect(stringifyParams).toEqual({ postId: 2 })
36973697

36983698
expect(postLink).toHaveAttribute('href', '/2')
36993699

@@ -3702,7 +3702,7 @@ describe('Link', () => {
37023702
const posts2Text = await screen.findByText('Post: 2')
37033703
expect(posts2Text).toBeInTheDocument()
37043704

3705-
expect(parseParams).toHaveBeenCalledWith({ status: 'parsed', postId: '2' })
3705+
expect(parseParams).toEqual({ postId: '2' })
37063706
})
37073707

37083708
test('when navigating to /$postId with params.parse and params.stringify handles falsey inputs', async () => {

0 commit comments

Comments
 (0)