refactor(router-core): slightly faster replaceEqualDeep #5046
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughIntroduces enumerable-own-key checks and an early bailout for non-object/non-array inputs, replaces Reflect.ownKeys-based enumeration with a getEnumerableOwnKeys helper that bails on non-enumerable own properties, pre-sizes array copies, and caches Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Utils as utils.replaceEqualDeep
Caller->>Utils: replaceEqualDeep(prev, next)
activate Utils
Utils->>Utils: determine types (array / plain object / other)
alt non-array & non-object
Utils-->>Caller: return next (early bailout)
else array
Utils->>Utils: allocate copy = new Array(nextSize)
loop indices
Utils->>Utils: compare prev[i] and next[i] (deep)
Utils->>Utils: assign reuse or copy[i] = value
end
else object
Utils->>Utils: prevKeys = getEnumerableOwnKeys(prev)
Utils->>Utils: nextKeys = getEnumerableOwnKeys(next)
alt any returned false
Utils-->>Caller: return next (non-enumerable own prop found)
else
Utils->>Utils: keys = union(prevKeys, nextKeys)
loop for each key
Utils->>Utils: p = prev[key] %% local cache
Utils->>Utils: if (array || prev.hasOwnProperty(key))?
alt prev missing/undefined
Utils->>Utils: copy[key] = next[key]
else
Utils->>Utils: value = replaceEqualDeep(p, next[key])
Utils->>Utils: if value === p && p !== undefined then reuse p else copy[key] = value
end
end
Utils-->>Caller: return prev if sizes and equalItems match else return copy
end
end
deactivate Utils
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 4b5515d
☁️ 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 (1)
packages/router-core/src/utils.ts (1)
231-235: Prefer hasOwnProperty over includes for O(1) membershipAvoids an O(n) includes scan per key when walking objects; short-circuits unchanged for arrays.
Apply this diff:
- if ( - (array || prevItems.includes(key)) && + if ( + (array || Object.prototype.hasOwnProperty.call(prev, key)) && prev[key] === undefined && next[key] === undefined ) {
📜 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/utils.ts(2 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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/router-core/src/utils.ts (1)
222-226: LGTM: Switch to Reflect.ownKeys reduces allocations without changing semanticsGiven isSimplePlainObject guards out non-enumerable string keys, this preserves behavior while likely shaving work vs keys+symbols concat.
Consider adding a tiny assertion-based test that covers symbol keys and undefined-valued own props to confirm parity with the old impl.
Sheraff
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/router-core/src/utils.ts (2)
220-223: Rename theobjectflag to avoid confusion with theobjecttype.Minor naming nit to improve readability.
- const object = !array && isPlainObject(prev) && isPlainObject(next) + const plainObject = !array && isPlainObject(prev) && isPlainObject(next) - if (!array && !object) return next + if (!array && !plainObject) return next
257-273: Type the helper and preallocatekeysto avoid push growth.Matches the PR’s “preallocate arrays” rationale and tightens types for callers.
-function getEnumerableOwnKeys(o: object) { - const keys = [] - const names = Object.getOwnPropertyNames(o) - for (const name of names) { - if (!Object.prototype.propertyIsEnumerable.call(o, name)) return false - keys.push(name) - } - const symbols = Object.getOwnPropertySymbols(o) - for (const symbol of symbols) { - if (!Object.prototype.propertyIsEnumerable.call(o, symbol)) return false - keys.push(symbol) - } - return keys +function getEnumerableOwnKeys( + o: object, +): Array<string | symbol> | false { + const names = Object.getOwnPropertyNames(o) + const symbols = Object.getOwnPropertySymbols(o) + const keys: Array<string | symbol> = new Array( + names.length + symbols.length, + ) + let i = 0 + for (let j = 0; j < names.length; j++) { + const name = names[j]! + if (!Object.prototype.propertyIsEnumerable.call(o, name)) return false + keys[i++] = name + } + for (let j = 0; j < symbols.length; j++) { + const symbol = symbols[j]! + if (!Object.prototype.propertyIsEnumerable.call(o, symbol)) return false + keys[i++] = symbol + } + return keys }
📜 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/utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sheraff
PR: TanStack/router#5051
File: packages/router-core/src/utils.ts:310-315
Timestamp: 2025-08-30T09:12:13.841Z
Learning: In TanStack Router's deepEqual utility, using for..in instead of Object.keys() in getObjectKeys() when ignoreUndefined=true is acceptable because it's called only after isPlainObject() checks, which ensure objects have standard Object prototype chains with no inherited enumerable properties.
📚 Learning: 2025-08-30T09:12:13.841Z
Learnt from: Sheraff
PR: TanStack/router#5051
File: packages/router-core/src/utils.ts:310-315
Timestamp: 2025-08-30T09:12:13.841Z
Learning: In TanStack Router's deepEqual utility, using for..in instead of Object.keys() in getObjectKeys() when ignoreUndefined=true is acceptable because it's called only after isPlainObject() checks, which ensure objects have standard Object prototype chains with no inherited enumerable properties.
Applied to files:
packages/router-core/src/utils.ts
🧬 Code graph analysis (1)
packages/router-core/src/utils.ts (3)
packages/react-router/src/index.tsx (2)
isPlainObject(28-28)replaceEqualDeep(27-27)packages/solid-router/src/index.tsx (2)
isPlainObject(28-28)replaceEqualDeep(27-27)packages/router-core/src/index.ts (2)
isPlainObject(273-273)replaceEqualDeep(272-272)
⏰ 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). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/router-core/src/utils.ts (1)
224-231: LGTM: key collection + array pre-sizing.The enumerable-own-keys bailout and array preallocation align with the perf goal.
Comment on lines
+234
to
254
| for (let i = 0; i < nextSize; i++) { | ||
| const key = array ? i : (nextItems[i] as any) | ||
| const p = prev[key] | ||
| if ( | ||
| (array || prev.hasOwnProperty(key)) && | ||
| p === undefined && | ||
| next[key] === undefined | ||
| ) { | ||
| copy[key] = undefined | ||
| equalItems++ | ||
| } else { | ||
| const value = replaceEqualDeep(p, next[key]) | ||
| copy[key] = value | ||
| if (value === p && p !== undefined) { | ||
| equalItems++ | ||
| } else { | ||
| copy[key] = replaceEqualDeep(prev[key], next[key]) | ||
| if (copy[key] === prev[key] && prev[key] !== undefined) { | ||
| equalItems++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return prevSize === nextSize && equalItems === prevSize ? prev : copy | ||
| } | ||
|
|
||
| return next | ||
| return prevSize === nextSize && equalItems === prevSize ? prev : copy | ||
| } |
There was a problem hiding this comment.
Use a safe hasOwnProperty check; current call can throw if shadowed. Also cache next[key].
Objects can define a string key named "hasOwnProperty", which would make prev.hasOwnProperty non-callable and throw at runtime. Prefer Object.prototype.hasOwnProperty.call. Caching next[key] avoids repeated lookups.
- const key = array ? i : (nextItems[i] as any)
- const p = prev[key]
- if (
- (array || prev.hasOwnProperty(key)) &&
- p === undefined &&
- next[key] === undefined
- ) {
+ const key = array ? i : (nextItems[i] as any)
+ const p = prev[key]
+ const n = next[key]
+ const hasPrevKey = array || Object.prototype.hasOwnProperty.call(prev, key)
+ if (hasPrevKey && p === undefined && n === undefined) {
copy[key] = undefined
equalItems++
} else {
- const value = replaceEqualDeep(p, next[key])
+ const value = replaceEqualDeep(p, n)
copy[key] = value
if (value === p && p !== undefined) {
equalItems++
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (let i = 0; i < nextSize; i++) { | |
| const key = array ? i : (nextItems[i] as any) | |
| const p = prev[key] | |
| if ( | |
| (array || prev.hasOwnProperty(key)) && | |
| p === undefined && | |
| next[key] === undefined | |
| ) { | |
| copy[key] = undefined | |
| equalItems++ | |
| } else { | |
| const value = replaceEqualDeep(p, next[key]) | |
| copy[key] = value | |
| if (value === p && p !== undefined) { | |
| equalItems++ | |
| } else { | |
| copy[key] = replaceEqualDeep(prev[key], next[key]) | |
| if (copy[key] === prev[key] && prev[key] !== undefined) { | |
| equalItems++ | |
| } | |
| } | |
| } | |
| return prevSize === nextSize && equalItems === prevSize ? prev : copy | |
| } | |
| return next | |
| return prevSize === nextSize && equalItems === prevSize ? prev : copy | |
| } | |
| for (let i = 0; i < nextSize; i++) { | |
| const key = array ? i : (nextItems[i] as any) | |
| const p = prev[key] | |
| const n = next[key] | |
| const hasPrevKey = array || Object.prototype.hasOwnProperty.call(prev, key) | |
| if (hasPrevKey && p === undefined && n === undefined) { | |
| copy[key] = undefined | |
| equalItems++ | |
| } else { | |
| const value = replaceEqualDeep(p, n) | |
| copy[key] = value | |
| if (value === p && p !== undefined) { | |
| equalItems++ | |
| } | |
| } | |
| } | |
| return prevSize === nextSize && equalItems === prevSize ? prev : copy | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/utils.ts around lines 234 to 254, the loop uses
prev.hasOwnProperty(key) which can throw if an object shadowed that method and
it repeatedly reads next[key]; change to use
Object.prototype.hasOwnProperty.call(prev, key) and cache next[key] into a local
variable (e.g. const nextVal = next[key]) before using it so you reuse nextVal
for the undefined checks and when calling replaceEqualDeep, replacing all
occurrences of next[key] with the cached nextVal.
schiller-manuel
approved these changes
Aug 30, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Improve performance of this function that gets called a lot for structural sharing. Main improvements are
Object.keys().concat(Object.getOwnPropertySymbols())creates 3 arrays, when we only want 1)new Array(size)over[])More minor changes (not 100% sure I can measure it, but I think so):
keys.includes(k)is slower thanObject.hasOwnProperty.call(obj, k)orobj.hasOwnProperty(k)benchmark
TL;DR: consistently 1.3x faster implementation
The
replaceEqualDeepimplementation before this PR handles Symbol keys, and non-enumerable keys (see #4237), but not keys that are both a Symbol and non-enumerable.This PR fixes this issue. But if we also fix it in the previous implementation before comparing performance we get a bigger perf diff
Summary by CodeRabbit
Bug Fixes
Performance