refactor(router-core): unify replaceEqualDeep implementation w/ query #5067
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
WalkthroughRefactors replaceEqualDeep in packages/router-core/src/utils.ts to unify object/array guards, add early short-circuits for identical values, streamline primitive/null handling, and simplify recursive assignments and equality counting. Return logic and public API remain unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant replaceEqualDeep
Caller->>replaceEqualDeep: (prev, next)
alt Non-object/primitive/null on either side
replaceEqualDeep-->>Caller: return next
else Both plain objects or arrays
loop Iterate entries
alt p === n
Note right of replaceEqualDeep: Short-circuit copy and increment equalItems
else p or n is primitive/null
Note right of replaceEqualDeep: Copy n without recursion
else Both objects/arrays
replaceEqualDeep->>replaceEqualDeep: recurse(p, n)
Note right of replaceEqualDeep: Count equal when v === p
end
end
alt sizes unchanged and all items equal
replaceEqualDeep-->>Caller: return prev
else
replaceEqualDeep-->>Caller: return copied structure
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 ed2c481
☁️ 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: 1
📜 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)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Sheraff
PR: TanStack/router#5051
File: packages/router-core/src/utils.ts:310-315
Timestamp: 2025-08-30T09:12:13.852Z
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.
⏰ 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 (3)
packages/router-core/src/utils.ts (3)
221-221: Guard consolidation LGTMRequiring both sides to be plain arrays or plain objects before recursing is correct and aligns with Query’s semantics. Early return keeps this fast.
245-252: Primitive/null short-circuit LGTMThe broadened check avoids unnecessary recursion for non-objects and nulls; good for both correctness and perf.
254-256: Simplified recursion and equality counting LGTMUsing
v === pfor equality accounting is clean and preserves structural sharing when possible.
Comment on lines
+238
to
+242
| if (p === n) { | ||
| copy[key] = p | ||
| if (array ? i < prevSize : prev.hasOwnProperty(key)) equalItems++ | ||
| continue | ||
| } |
There was a problem hiding this comment.
Use safe hasOwnProperty check (prev.hasOwnProperty can throw or be shadowed)
Calling prev.hasOwnProperty(key) can fail for null-prototype objects and can be shadowed by an own property. Use Object.prototype.hasOwnProperty.call(prev, key) like you already do elsewhere in this file (deepEqual).
Apply this diff:
- if (array ? i < prevSize : prev.hasOwnProperty(key)) equalItems++
+ if (array ? i < prevSize : Object.prototype.hasOwnProperty.call(prev, key))
+ 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
| if (p === n) { | |
| copy[key] = p | |
| if (array ? i < prevSize : prev.hasOwnProperty(key)) equalItems++ | |
| continue | |
| } | |
| if (p === n) { | |
| copy[key] = p | |
| if (array ? i < prevSize : Object.prototype.hasOwnProperty.call(prev, key)) | |
| equalItems++ | |
| continue | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/utils.ts around lines 238 to 242, the code uses
prev.hasOwnProperty(key) which can be unsafe for null-prototype objects or if
hasOwnProperty is shadowed; replace that call with
Object.prototype.hasOwnProperty.call(prev, key) to perform a safe property
ownership check, keeping the surrounding logic unchanged so equalItems++ only
increments when the property is truly an own property.
Sheraff
deleted the
refactor-router-core-replace-equal-deep-unify-with-query
branch
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.
Following TanStack/query#9604, this PR replicates the implementation of
replaceEqualDeephere, so that both are more similar (avoid drifting apart over time).There are no performance changes.
Summary by CodeRabbit
Performance
Refactor