Uh oh!
There was an error while loading. Please reload this page.
test(router-core): add search param serialization / deserialization unit tests - #4987
Merged
Conversation
Contributor
WalkthroughAdds a new test suite that verifies serialization and deserialization of values to/from URL query strings using defaultStringifySearch and defaultParseSearch, covering primitives, complex objects, special keys, circular references, alien inputs, and non-primitive objects. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ 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 ( |
Contributor
View your CI Pipeline Execution ↗ for commit 64b52bc
☁️ 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-adaptercommit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/router-core/tests/searchParams.test.ts (4)
9-43: Solid and readable isomorphism coverage; consider a few more representative casesGreat coverage across primitives, special characters, empty keys, and nested structures. To further harden the contract and document edge behaviors, consider adding:
- Booleans: false vs "false"
- Negative and floating-point numbers
- Plus sign in values and keys (ensures '+' vs '%2B' handling)
- Keys with spaces (confirm '+' encoding in keys as well)
- NaN/Infinity (they’re not valid JSON; document how they’re treated)
Example additions to the table:
[{ foo: true }, '?foo=true'], + [{ foo: false }, '?foo=false'], [{ foo: 'true' }, '?foo=%22true%22'], + [{ foo: 'false' }, '?foo=%22false%22'],+ [{ foo: -1 }, '?foo=-1'],+ [{ foo: 12.34 }, '?foo=12.34'],+ [{ foo: 'a+b' }, '?foo=a%2Bb'],+ [{ 'a b': 1 }, '?a+b=1'],+ [{ foo: NaN }, '?foo=null'], // if JSON.stringify is used -> "null"+ [{ foo: 'NaN' }, '?foo=%22NaN%22'],+ [{ foo: Infinity }, '?foo=null'], // if JSON.stringify is used -> "null"+ [{ foo: 'Infinity' }, '?foo=%22Infinity%22'],If
defaultStringifySearchdoes not useJSON.stringifyfor numbers, adjust the NaN/Infinity expectations accordingly (e.g., to stringification fallback or omission). Happy to align once confirmed.
45-51: Nit: fix test name to reflect “[object Object]”The expectation encodes
[object Object], but the description says"object Object". Clarifying the title avoids confusion.- test('[edge case] self-reference serializes to "object Object"', () => {+ test('[edge case] self-reference serializes to "[object Object]"', () => {
60-74: Prefer toEqual over toBe for semantic consistencyFor primitives,
toBeandtoEqualbehave the same. UsingtoEqualhere aligns with the rest of the file and avoids any confusion about identity vs deep/value equality.- expect(defaultStringifySearch(obj)).not.toBe(input)+ expect(defaultStringifySearch(obj)).not.toEqual(input)
81-95: Promise inputs: avoid constructing an unresolved PromiseUsing
new Promise(() => {})creates an unresolved promise, which can trip linters or future runtime diagnostics. UsingPromise.resolve()expresses intent without side effects. Behavior for serialization remains the same (no own enumerable keys at top-level; JSON serialization yields "{}" when nested).- expect(defaultStringifySearch(new Promise(() => {}))).toEqual('')+ expect(defaultStringifySearch(Promise.resolve())).toEqual('') @@ - expect(defaultStringifySearch({ foo: new Promise(() => {}) })).toEqual(+ expect(defaultStringifySearch({ foo: Promise.resolve() })).toEqual( '?foo=%7B%7D', )Optional: You may also want to document BigInt behavior (e.g.,
{ foo: 10n }), since JSON does not support BigInt and implementations often fall back to stringification or error. Adding an assertion will lock down current behavior.
📜 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/tests/searchParams.test.ts(1 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: Preview
- GitHub Check: Test
Uh oh!
There was an error while loading. Please reload this page.
SeanCassiere
requested changes
Aug 18, 2025
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Sheraff
commented
Aug 18, 2025
CollaboratorAuthor
@SeanCassiere thank you for the review. Can you think of any more weird cases that might be missing from these tests? |
SeanCassiere
approved these changes
Aug 19, 2025
SeanCassiere
left a comment
Member
There was a problem hiding this comment.
@Sheraff nope, these tests seems pretty good to me! Plus, they're laid out in such a way that adding more would be easy.
Uh oh!
There was an error while loading. Please reload this page.
Sheraff added a commit
that referenced
this pull request
Aug 19, 2025
…Search (#4985) Wait for this PR before merging: #4987. It adds unit tests that should help make sure we're not breaking anything here. --- The functions used to serialize and deserialize search params are called *a lot*, but they are creating many unnecessary objects. This PR refactors - `encode` to be only as generic as what we need => in practice we only need to handle 1 value per key (i.e. `.set()` and not `.append()`) so we don't need a special case for arrays - `decode` can avoid creating many arrays - `stringifySearchWith` (and `defaultStringifySearch`) don't need to pre-stringify the input into a `<string,string>` dictionary as `encode` now accepts a stringifier function <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - Breaking Changes - Removed encode and decode from React Router and Solid Router public exports. - Query string prefix support removed in encode/decode. - Refactor - Updated query string handling: arrays encode as comma-separated values; duplicate keys decode into arrays. - Streamlined error handling and avoided input mutation; consistent leading “?” management. - Tests - Updated tests to match new encoding/decoding behavior and removed prefix-related cases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Just some tests for
defaultParseSearchanddefaultStringifySearch, because they're used a lot and are very public and can handle user inputs (as in final human users, not devs).These tests are just documentation of the current behavior.
Summary by CodeRabbit