fix(router-core): use configured search serializer for loader dependency keys#7818
fix(router-core): use configured search serializer for loader dependency keys#7818gonzoblasco wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLoader dependency cache keys now use the router’s configured search serializer. Regression tests cover BigInt-compatible serialization and distinct cache identities for changing loader dependency values. ChangesLoader dependency serialization
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router-core/tests/loader-deps-serializer.test.ts`:
- Line 10: Update the bigint handling condition in the loader dependency
serializer to wrap its single-line return body in curly braces, preserving the
existing serialization behavior and value.toString()n output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51396397-c8c3-41fa-8349-d74017ce065a
📒 Files selected for processing (2)
packages/router-core/src/router.tspackages/router-core/tests/loader-deps-serializer.test.ts
gonzoblasco
force-pushed
the
fix/7787-loader-deps-search-serializer
branch
from
508c851 to
80ed772
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
HANDOFF.md (1)
4-7: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate mutable project status from durable handoff instructions.
The star count, PR state, estimated review window, and “Qué sigue” items will become stale as the work progresses. Add a “last updated” date and link to authoritative PR metadata, or move this tracking out of
HANDOFF.mdso future readers do not treat outdated status as current.Also applies to: 18-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@HANDOFF.md` around lines 4 - 7, Separate mutable project-status details from the durable handoff guidance in HANDOFF.md: move the star count, PR state, review estimate, and “Qué sigue” tracking to an appropriate status location, or clearly mark that section with a last-updated date and authoritative PR metadata link. Keep HANDOFF.md focused on stable instructions and ensure future readers can distinguish current status from durable guidance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@HANDOFF.md`:
- Around line 4-7: Separate mutable project-status details from the durable
handoff guidance in HANDOFF.md: move the star count, PR state, review estimate,
and “Qué sigue” tracking to an appropriate status location, or clearly mark that
section with a last-updated date and authoritative PR metadata link. Keep
HANDOFF.md focused on stable instructions and ensure future readers can
distinguish current status from durable guidance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46fe2442-45f1-4c00-a3cf-e785fb85fb3d
📒 Files selected for processing (3)
HANDOFF.mdpackages/router-core/src/router.tspackages/router-core/tests/loader-deps-serializer.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/router-core/tests/loader-deps-serializer.test.ts
- packages/router-core/src/router.ts
gonzoblasco
force-pushed
the
fix/7787-loader-deps-search-serializer
branch
2 times, most recently
from
0ce9fa4 to
f7a3eb5
Compare
…ncy keys The loaderDepsHash was hardcoded to JSON.stringify, ignoring the router's configured stringifySearch option. This meant custom search serializers that support values JSON.stringify cannot handle (e.g. bigint, Set, Map) would fail when those values were used in loaderDeps. Replace JSON.stringify(loaderDeps) with this.options.stringifySearch( loaderDeps) so the hash uses the same serializer configured for URL search state. This continues the work started in TanStack#4713. Add regression tests verifying: - Custom serializer with bigint support works for loaderDeps - Different deps values produce different match IDs Fixes TanStack#7787
gonzoblasco
force-pushed
the
fix/7787-loader-deps-search-serializer
branch
from
f7a3eb5 to
4e78e57
Compare
|
Hi @LadyBluenotes — done! The file has been removed from this PR. |
|
View your CI Pipeline Execution ↗ for commit beb40e4
☁️ Nx Cloud last updated this comment at |
Merging this PR will improve performance by 81.45%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server error-paths unmatched (react) |
264.2 KB | 571.6 KB | -53.78% |
| ❌ | Memory | mem server peak-large-page (solid) |
771.4 KB | 961.7 KB | -19.79% |
| ❌ | Memory | mem server streaming-peak chunked (vue) |
11.1 MB | 13.7 MB | -19.43% |
| ❌ | Memory | mem server error-paths not-found (react) |
267 KB | 277.4 KB | -3.73% |
| ❌ | Simulation | client-async-pipeline navigation loop (react) |
49.8 ms | 51.7 ms | -3.57% |
| ⚡ | Memory | mem server server-fn-churn (vue) |
3,985.2 KB | 263.4 KB | ×15 |
| ⚡ | Memory | mem server server-fn-churn (solid) |
3,825.7 KB | 273.8 KB | ×14 |
| ⚡ | Memory | mem server error-paths redirect (vue) |
611.4 KB | 305 KB | ×2 |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing gonzoblasco:fix/7787-loader-deps-search-serializer (beb40e4) with main (0b178a7)
|
Thanks for the benchmark report! I want to acknowledge the flagged regressions. The change in this PR is minimal — replacing Looking at the report:
I believe these are false positives from the runtime environment differences, but happy to investigate further if maintainers think otherwise. @LadyBluenotes — the |
I want to be candid: this reply feel like it's coming from an AI agent rather than from a human directly. AI assistance is fine, but I need to know that you understand and have personally verified the changes you’re submitting. If it continues to feel like an agent is speaking on your behalf, I’ll close the PR. As for the benchmark results, these benchmarks are known to be flaky, so the reported changes can be taken with a grain of salt. |
|
a loader dep CAN be a search param, but it doesnt have to be. tying it to the search stringifier is not the way |
|
Hi, my name is Gonzalo, I'm who is creating this PRs. I'm testing how this agent I created with OpenClaw allowed me to help with the community. I'm checking everything what is doing, step by step. Most my code was written by GLM-5.2 |
3 participants
Description
The
loaderDepsHashwas hardcoded toJSON.stringify, ignoring the router's configuredstringifySearchoption. This meant custom search serializers that support valuesJSON.stringifycannot handle (e.g.bigint,Set,Map) would fail or produce incorrect hashes when those values were used inloaderDeps.Root Cause
In
packages/router-core/src/router.ts, line 1538:JSON.stringifyis used regardless of thestringifySearchoption configured on the router. If a user configures a customstringifySearchto support non-JSON-serializable values in search params, those same values cannot be used inloaderDepsbecause the hash will throw or produce incorrect results.Fix
Replace
JSON.stringify(loaderDeps)withthis.options.stringifySearch(loaderDeps)so the hash uses the same serializer configured for URL search state.This continues the work started in #4713.
Tests
Added
packages/router-core/tests/loader-deps-serializer.test.tswith two regression tests:Custom serializer with bigint support — verifies that a router configured with a custom
stringifySearchsupportingbigintcan usebigintvalues inloaderDepswithout throwing (whichJSON.stringifywould do withTypeError: Do not know how to serialize a BigInt).Different deps produce different match IDs — verifies that changing
loaderDepsvalues produces different match IDs, confirming the hash is still deterministic and distinguishable.Verification
callbacks.test.ts(9 tests) — all passmatch-params.test.ts(23 tests) — all passload.test.ts— 1 pre-existing flaky test fails (skip if pending preload (redirect)) — confirmed failing onmainwithout this changeFixes #7787
Summary by CodeRabbit
bigint.