perf(router-core): skip JSON parsing for plain search strings#7022
perf(router-core): skip JSON parsing for plain search strings#7022
Conversation
📝 WalkthroughWalkthroughAdded a pre-parse validator and updated parse/stringify flows so JSON.parse is only attempted for strings that look like JSON; custom parsers are still applied unconditionally. Tests updated to cover both JSON-like and non-JSON-like query values and round-trip behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 7a5b555
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ad29de715
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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 the current code and only fix it if needed.
Inline comments:
In `@packages/router-core/src/searchParams.ts`:
- Around line 23-31: The helper canStringBeJsonParsed rejects JSON literals if
they have surrounding whitespace; update canStringBeJsonParsed to trim the input
before checking literal values so inputs like " true " or "null\t" are
recognized the same as "true"/"false"/"null" (e.g. use a trimmedValue =
value.trim() and test trimmedValue === 'true' || trimmedValue === 'false' ||
trimmedValue === 'null'); ensure the existing firstCharCode checks still operate
on the original string start for objects/arrays/numbers or, if you prefer, use
trimmedValue consistently for both literal checks and the numeric firstCharCode
branch to preserve JSON.parse behavior with leading whitespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b379670e-aa36-4d1f-ae03-3a9a221cf7bc
📒 Files selected for processing (2)
packages/router-core/src/searchParams.tspackages/router-core/tests/searchParams.test.ts
| return ( | ||
| firstCharCode === 34 || | ||
| firstCharCode === 45 || | ||
| firstCharCode === 91 || | ||
| firstCharCode === 123 || | ||
| (firstCharCode >= 48 && firstCharCode <= 57) || | ||
| value === 'true' || | ||
| value === 'false' || | ||
| value === 'null' |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does JavaScript JSON.parse accept trailing whitespace for literals like "true ", "false ", and "null "?
💡 Result:
Yes. JSON.parse accepts trailing (and leading) JSON whitespace around any valid JSON value, including the literals true, false, and null—so "true ", "false\t\n", and " null\r" all parse successfully.
That’s because JSON defines a complete JSON text as ws value ws (so whitespace is allowed after the value) [2], and ECMAScript’s JSON.parse requires the input to be a valid JSON text per ECMA-404 [3]. ECMA-404 also states that insignificant whitespace is allowed before or after any token [1].
Only JSON whitespace is allowed (space, tab, LF, CR). Any other trailing non-whitespace character (e.g. "true x") must throw a SyntaxError.
Fix behavioral regression for JSON literals with surrounding whitespace.
JSON.parse accepts valid JSON text with leading/trailing whitespace (e.g., JSON.parse("true ") succeeds), but the canStringBeJsonParsed helper only accepts exact 'true' | 'false' | 'null' without whitespace. This causes inputs like true (from URL parameters with + encoding) to skip parsing and remain strings, regressing the original behavior.
Suggested fix
function canStringBeJsonParsed(value: string) {
if (!value) {
return false
}
- const firstCharCode = value.charCodeAt(0)
+ const normalized = value.trim()
+ if (!normalized) {
+ return false
+ }
+
+ const firstCharCode = normalized.charCodeAt(0)
if (firstCharCode <= 32) {
return true
}
return (
firstCharCode === 34 ||
firstCharCode === 45 ||
firstCharCode === 91 ||
firstCharCode === 123 ||
(firstCharCode >= 48 && firstCharCode <= 57) ||
- value === 'true' ||
- value === 'false' ||
- value === 'null'
+ normalized === 'true' ||
+ normalized === 'false' ||
+ normalized === 'null'
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router-core/src/searchParams.ts` around lines 23 - 31, The helper
canStringBeJsonParsed rejects JSON literals if they have surrounding whitespace;
update canStringBeJsonParsed to trim the input before checking literal values so
inputs like " true " or "null\t" are recognized the same as
"true"/"false"/"null" (e.g. use a trimmedValue = value.trim() and test
trimmedValue === 'true' || trimmedValue === 'false' || trimmedValue === 'null');
ensure the existing firstCharCode checks still operate on the original string
start for objects/arrays/numbers or, if you prefer, use trimmedValue
consistently for both literal checks and the numeric firstCharCode branch to
preserve JSON.parse behavior with leading whitespace.
Merging this PR will improve performance by 8.72%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | ssr request loop (vue) |
416.1 ms | 401.4 ms | +3.67% |
| ⚡ | ssr request loop (solid) |
153.1 ms | 140.8 ms | +8.72% |
| ⚡ | ssr request loop (react) |
328.1 ms | 308.1 ms | +6.49% |
| ⚡ | client-side navigation loop (react) |
61.4 ms | 58 ms | +5.74% |
Comparing flo/search-params-json-parse-guard (7a5b555) with main (6077120)1
Footnotes
Only apply the JSON fast path to JSON.parse so documented custom serializers still round-trip, and add regression coverage for non-JSON parsers.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/router-core/src/searchParams.ts (1)
17-31:⚠️ Potential issue | 🟠 MajorFix JSON-literal detection for trailing whitespace.
At Line 29–Line 31, the literal checks use raw
value, so inputs like"true "/"null\t"(valid forJSON.parse) fail the guard and stay strings. That regresses prior behavior and can break parse/stringify symmetry for literal-like query values.Suggested patch
function canStringBeJsonParsed(value: string) { if (!value) { return false } - const firstCharCode = value.charCodeAt(0) - - if (firstCharCode <= 32) { - return true - } + const normalized = value.trim() + if (!normalized) { + return false + } + + const firstCharCode = normalized.charCodeAt(0) return ( firstCharCode === 34 || firstCharCode === 45 || firstCharCode === 91 || firstCharCode === 123 || (firstCharCode >= 48 && firstCharCode <= 57) || - value === 'true' || - value === 'false' || - value === 'null' + normalized === 'true' || + normalized === 'false' || + normalized === 'null' ) }Does JavaScript `JSON.parse` accept trailing JSON whitespace for literals like "true ", "false\t", and "null\r"?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/searchParams.ts` around lines 17 - 31, The literal checks currently compare the raw value so inputs with trailing whitespace (e.g. "true ", "null\t") aren't recognized as JSON literals; create a trimmed version (const trimmed = value.trim()) and use trimmed for the equality checks (trimmed === 'true' || trimmed === 'false' || trimmed === 'null') and for any literal-specific comparisons instead of raw value, while keeping the existing firstCharCode logic for the initial-character checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/router-core/src/searchParams.ts`:
- Around line 17-31: The literal checks currently compare the raw value so
inputs with trailing whitespace (e.g. "true ", "null\t") aren't recognized as
JSON literals; create a trimmed version (const trimmed = value.trim()) and use
trimmed for the equality checks (trimmed === 'true' || trimmed === 'false' ||
trimmed === 'null') and for any literal-specific comparisons instead of raw
value, while keeping the existing firstCharCode logic for the initial-character
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98b675c9-eca6-4834-bf22-d3eea058e4b1
📒 Files selected for processing (2)
packages/router-core/src/searchParams.tspackages/router-core/tests/searchParams.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/router-core/tests/searchParams.test.ts
1 participant
Summary
parseSearchWithandstringifySearchWithwhile preserving existing behavior for JSON-like valuesBenchmark Notes
@benchmarks/client-nav: React29.6119ms -> 28.6312ms(~3.3% faster)@benchmarks/client-nav: Solid24.7548ms -> 24.2348ms(~2.1% faster)@benchmarks/client-nav: Vue22.5473ms -> 21.7420ms(~3.6% faster)@benchmarks/bundle-size: router scenario gzip deltas ranged from+71Bto+82BTesting
CI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:unit --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @tanstack/router-core:test:types --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf --outputStyle=stream --skipRemoteCacheCI=1 NX_DAEMON=false pnpm nx run @benchmarks/bundle-size:build --outputStyle=stream --skipRemoteCacheSummary by CodeRabbit
Refactor
Tests