Skip to content

Commit 48551b3

Browse files
refactor(router-core): slightly faster deepEqual (#5051)
## description - inline `getObjectKeys` as regular control flow inside `deepEqual` to be able to skip as much work as possible, and create fewer objects - using a regular `for` loop for the array case to skip the overhead of using native array iterators w/ the `.every()` or `.some()` methods - check `Array.isArray` before `isPlainObject` because this check is cheaper - `opts.partial` is a special case that allows us to skip a bunch of work, so we give it its own branch ## benchmark TL;DR: consistently 1.7x faster implementation ```ts describe.only('deepEqual', () => { bench('old implementation', () => { deepEqualOld({ a: 1, b: [2, 3] }, { a: 1, b: [2, 3] }, {partial: true}) deepEqualOld({ a: 1, b: [2, 3] }, { a: 2, b: [2] }) }) bench('new implementation', () => { deepEqual({ a: 1, b: [2, 3] }, { a: 1, b: [2, 3] }, {partial: true}) deepEqual({ a: 1, b: [2, 3] }, { a: 2, b: [2] }) }) }) ``` ```sh ✓ @tanstack/router-core tests/utils.bench.ts > deepEqual 2484ms name hz min max mean p75 p99 p995 p999 rme samples · old implementation 3,806,354.20 0.0002 4.3084 0.0003 0.0003 0.0005 0.0007 0.0013 ±1.78% 1903178 · new implementation 6,978,154.05 0.0001 0.1937 0.0001 0.0002 0.0002 0.0002 0.0003 ±0.15% 3489078 fastest BENCH Summary @tanstack/router-core new implementation - tests/utils.bench.ts > deepEqual 1.83x faster than old implementation ``` ---- This PR also adds test cases for using `deepEqual` w/ objects containing Symbol and non-enumerable keys. The behavior for those was not changed in this PR, we're just documenting it: they are simply ignored. ---- <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved deep-equality checks for arrays and object comparisons; ignore-undefined handling clarified and made more consistent. * **Refactor** * Iteration and comparison logic reorganized for earlier short-circuiting; behavior around enumerable (including inherited) keys adjusted; no public API changes. * **Tests** * Added tests demonstrating edge cases with symbol/non-enumerable properties and prototype augmentation (marked as expected failures). <!-- 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 90e9006 commit 48551b3

File tree

2 files changed

+77
-19
lines changed

2 files changed

+77
-19
lines changed

packages/router-core/src/utils.ts

Lines changed: 31 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -306,14 +306,6 @@ export function isPlainArray(value: unknown): value is Array<unknown> {
306306
return Array.isArray(value) && value.length === Object.keys(value).length
307307
}
308308

309-
function getObjectKeys(obj: any, ignoreUndefined: boolean) {
310-
let keys = Object.keys(obj)
311-
if (ignoreUndefined) {
312-
keys = keys.filter((key) => obj[key] !== undefined)
313-
}
314-
return keys
315-
}
316-
317309
export function deepEqual(
318310
a: any,
319311
b: any,
@@ -327,23 +319,44 @@ export function deepEqual(
327319
return false
328320
}
329321

322+
if (Array.isArray(a) && Array.isArray(b)) {
323+
if (a.length !== b.length) return false
324+
for (let i = 0, l = a.length; i < l; i++) {
325+
if (!deepEqual(a[i], b[i], opts)) return false
326+
}
327+
return true
328+
}
329+
330330
if (isPlainObject(a) && isPlainObject(b)) {
331331
const ignoreUndefined = opts?.ignoreUndefined ?? true
332-
const aKeys = getObjectKeys(a, ignoreUndefined)
333-
const bKeys = getObjectKeys(b, ignoreUndefined)
334332

335-
if (!opts?.partial && aKeys.length !== bKeys.length) {
336-
return false
333+
if (opts?.partial) {
334+
for (const k in b) {
335+
if (!ignoreUndefined || b[k] !== undefined) {
336+
if (!deepEqual(a[k], b[k], opts)) return false
337+
}
338+
}
339+
return true
337340
}
338341

339-
return bKeys.every((key) => deepEqual(a[key], b[key], opts))
340-
}
342+
let aCount = 0
343+
if (!ignoreUndefined) {
344+
aCount = Object.keys(a).length
345+
} else {
346+
for (const k in a) {
347+
if (a[k] !== undefined) aCount++
348+
}
349+
}
341350

342-
if (Array.isArray(a) && Array.isArray(b)) {
343-
if (a.length !== b.length) {
344-
return false
351+
let bCount = 0
352+
for (const k in b) {
353+
if (!ignoreUndefined || b[k] !== undefined) {
354+
bCount++
355+
if (bCount > aCount || !deepEqual(a[k], b[k], opts)) return false
356+
}
345357
}
346-
return !a.some((item, index) => !deepEqual(item, b[index], opts))
358+
359+
return aCount === bCount
347360
}
348361

349362
return false

packages/router-core/tests/utils.test.ts

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, it } from 'vitest'
1+
import { afterEach, describe, expect, it } from 'vitest'
22
import { deepEqual, isPlainArray, replaceEqualDeep } from '../src/utils'
33

44
describe('replaceEqualDeep', () => {
@@ -434,4 +434,49 @@ describe('deepEqual', () => {
434434
expect(deepEqual(b, a, { partial: true })).toEqual(false)
435435
})
436436
})
437+
438+
// This might not be what we want, but this test documents how things are now
439+
describe('symbol and non-enumerable properties are not handled', () => {
440+
it.fails(
441+
'should return `false` for unequal objects with symbol properties',
442+
() => {
443+
const key = Symbol('foo')
444+
const a = { [key]: 1 }
445+
const b = { [key]: 2 }
446+
expect(deepEqual(a, b)).toEqual(false)
447+
},
448+
)
449+
450+
it.fails(
451+
'should return `false` for unequal objects with non-enumerable properties',
452+
() => {
453+
const a = {}
454+
Object.defineProperty(a, 'prop', { value: 1, enumerable: false })
455+
const b = {}
456+
Object.defineProperty(b, 'prop', { value: 2, enumerable: false })
457+
expect(deepEqual(a, b)).toEqual(false)
458+
},
459+
)
460+
})
461+
462+
// We voluntarily fail in this case, because users should not do it, and ignoring it enables some performance improvements
463+
describe('augmented object prototype fail case (no one should do this anyway)', () => {
464+
it.fails(
465+
'should not compare objects with augmented prototype properties',
466+
() => {
467+
// @ts-expect-error -- typescript is right to complain here, don't do this!
468+
Object.prototype.x = 'x'
469+
const a = { a: 1 }
470+
const b = { a: 1 }
471+
expect(deepEqual(a, b, { ignoreUndefined: false })).toEqual(true)
472+
},
473+
)
474+
475+
afterEach(() => {
476+
// it's probably not necessary to clean this up because vitest isolates tests
477+
// but just in case isolation ever gets disabled, we clean the prototype to avoid disturbing other tests
478+
// @ts-expect-error
479+
delete Object.prototype.x
480+
})
481+
})
437482
})

0 commit comments

Comments
 (0)