-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
test: client-nav benchmarks use a more representative app #6812
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||||||||||||||||
| import { For, createEffect } from 'solid-js' | ||||||||||||||||||||||||||||||||||
| import { For } from 'solid-js' | ||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||
| Link, | ||||||||||||||||||||||||||||||||||
| Outlet, | ||||||||||||||||||||||||||||||||||
|
|
@@ -7,14 +7,15 @@ import { | |||||||||||||||||||||||||||||||||
| createRootRoute, | ||||||||||||||||||||||||||||||||||
| createRoute, | ||||||||||||||||||||||||||||||||||
| createRouter, | ||||||||||||||||||||||||||||||||||
| useMatch, | ||||||||||||||||||||||||||||||||||
| useParams, | ||||||||||||||||||||||||||||||||||
| useSearch, | ||||||||||||||||||||||||||||||||||
| } from '@tanstack/solid-router' | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| function runPerfSelectorComputation(seed: number) { | ||||||||||||||||||||||||||||||||||
| let value = Math.trunc(seed) | 0 | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| for (let index = 0; index < 100; index++) { | ||||||||||||||||||||||||||||||||||
| for (let index = 0; index < 50; index++) { | ||||||||||||||||||||||||||||||||||
| value = (value * 1664525 + 1013904223 + index) >>> 0 | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -24,29 +25,27 @@ function runPerfSelectorComputation(seed: number) { | |||||||||||||||||||||||||||||||||
| const selectors = Array.from({ length: 20 }, (_, index) => index) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| function Params() { | ||||||||||||||||||||||||||||||||||
| const params = useParams({ | ||||||||||||||||||||||||||||||||||
| const number = useParams({ | ||||||||||||||||||||||||||||||||||
| strict: false, | ||||||||||||||||||||||||||||||||||
| select: (params) => runPerfSelectorComputation(Number(params.id ?? 0)), | ||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| createEffect(() => { | ||||||||||||||||||||||||||||||||||
| void params() | ||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||
| return <>{runPerfSelectorComputation(number())}</> | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| function Search() { | ||||||||||||||||||||||||||||||||||
| const search = useSearch({ | ||||||||||||||||||||||||||||||||||
| const number = useSearch({ | ||||||||||||||||||||||||||||||||||
| strict: false, | ||||||||||||||||||||||||||||||||||
| select: (search) => runPerfSelectorComputation(Number(search.id ?? 0)), | ||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||
| return <>{runPerfSelectorComputation(number())}</> | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| createEffect(() => { | ||||||||||||||||||||||||||||||||||
| void search() | ||||||||||||||||||||||||||||||||||
| function Match() { | ||||||||||||||||||||||||||||||||||
| const number = useMatch({ | ||||||||||||||||||||||||||||||||||
| strict: false, | ||||||||||||||||||||||||||||||||||
| select: (match) => runPerfSelectorComputation(Number(match.params.id ?? 0)), | ||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||
| return <>{runPerfSelectorComputation(number())}</> | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cat -n benchmarks/client-nav/solid/app.tsx | head -60Repository: TanStack/router Length of output: 1748 🌐 Web query:
💡 Result: TanStack Router documents
|
||||||||||||||||||||||||||||||||||
| function Match() { | |
| const number = useMatch({ | |
| strict: false, | |
| select: (match) => runPerfSelectorComputation(Number(match.params.id ?? 0)), | |
| }) | |
| return null | |
| return <>{runPerfSelectorComputation(number())}</> | |
| } | |
| function Match() { | |
| const number = useMatch({ | |
| strict: false, | |
| select: (match) => runPerfSelectorComputation(Number(match?.params?.id ?? 0)), | |
| }) | |
| return <>{runPerfSelectorComputation(number())}</> | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/client-nav/solid/app.tsx` around lines 43 - 49, The select
callback in Match is accessing match.params.id but with strict: false
match.params can be undefined; update the select to use optional chaining (e.g.
select: (match) => runPerfSelectorComputation(Number(match.params?.id ?? 0))) so
params is safely accessed, and ensure any other direct uses of match.params in
Match use the same match.params?.id pattern; keep using
runPerfSelectorComputation and the number() call as-is.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,14 +7,15 @@ import { | |||||||||||||||||||||||||
| createRootRoute, | ||||||||||||||||||||||||||
| createRoute, | ||||||||||||||||||||||||||
| createRouter, | ||||||||||||||||||||||||||
| useMatch, | ||||||||||||||||||||||||||
| useParams, | ||||||||||||||||||||||||||
| useSearch, | ||||||||||||||||||||||||||
| } from '@tanstack/vue-router' | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| function runPerfSelectorComputation(seed: number) { | ||||||||||||||||||||||||||
| let value = Math.trunc(seed) | 0 | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| for (let index = 0; index < 100; index++) { | ||||||||||||||||||||||||||
| for (let index = 0; index < 50; index++) { | ||||||||||||||||||||||||||
| value = (value * 1664525 + 1013904223 + index) >>> 0 | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -25,29 +26,35 @@ const selectors = Array.from({ length: 20 }, (_, index) => index) | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const Params = Vue.defineComponent({ | ||||||||||||||||||||||||||
| setup() { | ||||||||||||||||||||||||||
| const params = useParams({ | ||||||||||||||||||||||||||
| const number = useParams({ | ||||||||||||||||||||||||||
| strict: false, | ||||||||||||||||||||||||||
| select: (params) => runPerfSelectorComputation(Number(params.id ?? 0)), | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return () => { | ||||||||||||||||||||||||||
| void params.value | ||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return () => runPerfSelectorComputation(number.value) | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const Search = Vue.defineComponent({ | ||||||||||||||||||||||||||
| setup() { | ||||||||||||||||||||||||||
| const search = useSearch({ | ||||||||||||||||||||||||||
| const number = useSearch({ | ||||||||||||||||||||||||||
| strict: false, | ||||||||||||||||||||||||||
| select: (search) => runPerfSelectorComputation(Number(search.id ?? 0)), | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return () => { | ||||||||||||||||||||||||||
| void search.value | ||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return () => runPerfSelectorComputation(number.value) | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const Match = Vue.defineComponent({ | ||||||||||||||||||||||||||
| setup() { | ||||||||||||||||||||||||||
| const number = useMatch({ | ||||||||||||||||||||||||||
| strict: false, | ||||||||||||||||||||||||||
| select: (match) => | ||||||||||||||||||||||||||
| runPerfSelectorComputation(Number(match.params.id ?? 0)), | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return () => runPerfSelectorComputation(number.value) | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -74,6 +81,9 @@ const Root = Vue.defineComponent({ | |||||||||||||||||||||||||
| {selectors.map((selector) => ( | ||||||||||||||||||||||||||
| <Links key={`link-${selector}`} /> | ||||||||||||||||||||||||||
| ))} | ||||||||||||||||||||||||||
| {selectors.map((selector) => ( | ||||||||||||||||||||||||||
| <Match key={`match-${selector}`} /> | ||||||||||||||||||||||||||
| ))} | ||||||||||||||||||||||||||
| <Outlet /> | ||||||||||||||||||||||||||
| </> | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
|
@@ -87,7 +97,11 @@ const root = createRootRoute({ | |||||||||||||||||||||||||
| const route = createRoute({ | ||||||||||||||||||||||||||
| getParentRoute: () => root, | ||||||||||||||||||||||||||
| path: '/$id', | ||||||||||||||||||||||||||
| validateSearch: (search) => ({ id: search.id }), | ||||||||||||||||||||||||||
| component: () => <div />, | ||||||||||||||||||||||||||
| beforeLoad: () => Promise.resolve(), | ||||||||||||||||||||||||||
| loaderDeps: ({ search }) => ({ id: search.id }), | ||||||||||||||||||||||||||
| loader: () => Promise.resolve(), | ||||||||||||||||||||||||||
|
Comment on lines
+100
to
+104
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check if file exists and examine the code =="
if [ -f benchmarks/client-nav/vue/app.tsx ]; then
echo "File exists. Showing lines 95-110:"
sed -n '95,110p' benchmarks/client-nav/vue/app.tsx
else
echo "File not found at benchmarks/client-nav/vue/app.tsx"
fi
echo
echo "== List files in benchmarks/client-nav =="
fd . benchmarks/client-nav --max-depth 2 -t f | head -20Repository: TanStack/router Length of output: 1372 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check TypeScript strict mode settings =="
find . -name "tsconfig.json" -type f | head -10 | while read f; do
echo "--- $f ---"
jq '.compilerOptions | {strict, noImplicitAny, noImplicitThis}' "$f" 2>/dev/null || cat "$f"
doneRepository: TanStack/router Length of output: 1281 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Search for validateSearch patterns =="
rg -n "validateSearch" benchmarks --type tsx --type ts -A 2 | head -50Repository: TanStack/router Length of output: 144 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check root and parent tsconfigs =="
for f in ./tsconfig.json ./tsconfig.base.json ./benchmarks/tsconfig.json benchmarks/client-nav/tsconfig.json benchmarks/client-nav/vue/tsconfig.json; do
if [ -f "$f" ]; then
echo "--- $f ---"
head -30 "$f"
echo
fi
done
echo "== Check if benchmarks/client-nav/vue/tsconfig.json extends anything =="
cat benchmarks/client-nav/vue/tsconfig.jsonRepository: TanStack/router Length of output: 1959 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Search for validateSearch in tsx files =="
rg -n "validateSearch" benchmarks/client-nav --type-list | head
rg -n "validateSearch" benchmarks/client-nav -t ts -t tsx 2>/dev/null | head -20 || find benchmarks/client-nav -name "*.tsx" -exec rg -n "validateSearch" {} +Repository: TanStack/router Length of output: 510 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check what the search parameter type is =="
sed -n '1,110p' benchmarks/client-nav/vue/app.tsx | tail -50Repository: TanStack/router Length of output: 1258 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Find createRoute definition and validateSearch typing =="
find . -path ./node_modules -prune -o -name "*.ts" -o -name "*.tsx" | xargs rg -l "createRoute" | grep -E "(src|packages)" | head -5
echo
echo "== Search for other validateSearch usages in non-benchmark code =="
rg -n "validateSearch" src packages --type ts --type tsx -B 1 -A 1 2>/dev/null | head -60 || find . -path ./node_modules -prune -o -path ./benchmarks -prune -o \( -name "*.ts" -o -name "*.tsx" \) -exec rg -l "validateSearch" {} +Repository: TanStack/router Length of output: 367 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check actual React benchmark for comparison =="
if [ -f benchmarks/client-nav/react/app.tsx ]; then
echo "=== React benchmark validateSearch ==="
rg -n "validateSearch" benchmarks/client-nav/react/app.tsx -B 2 -A 2
fi
echo
echo "== Check if there are type definitions or examples for validateSearch =="
rg -n "validateSearch.*search\|search.*validateSearch" packages src e2e --type ts --type tsx -A 3 2>/dev/null | head -80 || find . -path ./node_modules -prune -o \( -name "*.ts" -o -name "*.tsx" \) -exec rg "validateSearch" {} + | grep -v benchmarks | head -20Repository: TanStack/router Length of output: 3321 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Look at the e2e example that uses explicit typing =="
if [ -f e2e/vue-router/basic-file-based-jsx/src/routes/remountDeps.tsx ]; then
rg -n "validateSearch" e2e/vue-router/basic-file-based-jsx/src/routes/remountDeps.tsx -B 2 -A 5
fi
echo
echo "== Check if TypeScript compilation would fail for the benchmark =="
echo "Looking for any build or type-check scripts..."
cat benchmarks/client-nav/package.json | grep -A 20 '"scripts"' || echo "No package.json found"Repository: TanStack/router Length of output: 2040 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check if benchmarks are included in any type-checking or build =="
cat package.json | jq '.scripts' | head -30
echo
echo "== Look at what README says about benchmarks =="
head -50 benchmarks/client-nav/README.mdRepository: TanStack/router Length of output: 3999 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Verify the exact pattern used in e2e tests =="
cat e2e/vue-router/basic-file-based-jsx/src/routes/remountDeps.tsx | sed -n '1,80p'Repository: TanStack/router Length of output: 548 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check what happens when running type check on benchmarks =="
echo "Trying to type-check the vue benchmark..."
tsc -p benchmarks/client-nav/vue/tsconfig.json --noEmit 2>&1 | head -30 || true
echo
echo "== Check if the pattern in the code is correct given the input =="
echo "What type does TypeScript infer for validateSearch parameter?"
cat > /tmp/check.ts << 'EOF'
import { createRoute } from '@tanstack/router'
// Checking the implicit type of search parameter
const route = createRoute({
path: '/$id',
validateSearch: (search) => ({ id: search.id }),
})
EOF
tsc --noEmit --strict /tmp/check.ts 2>&1 | head -20 || echo "Type check completed"Repository: TanStack/router Length of output: 691 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Check if React benchmark has the same issue =="
rg -n "validateSearch" benchmarks/client-nav/react/app.tsx -B 1 -A 1
echo
echo "== Do both benchmarks have identical validateSearch patterns? =="
echo "Vue:"
rg -n "validateSearch:" benchmarks/client-nav/vue/app.tsx -A 1
echo
echo "React:"
rg -n "validateSearch:" benchmarks/client-nav/react/app.tsx -A 1Repository: TanStack/router Length of output: 463 Add explicit type annotation and narrowing to The Suggested refactor const route = createRoute({
getParentRoute: () => root,
path: '/$id',
- validateSearch: (search) => ({ id: search.id }),
+ validateSearch: (search: Record<string, unknown>) => ({
+ id: typeof search.id === 'string' ? search.id : '0',
+ }),
component: () => <div />,
beforeLoad: () => Promise.resolve(),
loaderDeps: ({ search }) => ({ id: search.id }),
loader: () => Promise.resolve(),
})📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| export function createTestRouter() { | ||||||||||||||||||||||||||
|
|
@@ -97,6 +111,7 @@ export function createTestRouter() { | |||||||||||||||||||||||||
| }), | ||||||||||||||||||||||||||
| scrollRestoration: true, | ||||||||||||||||||||||||||
| routeTree: root.addChildren([route]), | ||||||||||||||||||||||||||
| defaultPendingMinMs: 0, | ||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| const component = <RouterProvider router={router} /> | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: TanStack/router
Length of output: 131
🏁 Script executed:
Repository: TanStack/router
Length of output: 3066
🏁 Script executed:
Repository: TanStack/router
Length of output: 289
🏁 Script executed:
Repository: TanStack/router
Length of output: 86
🏁 Script executed:
Repository: TanStack/router
Length of output: 4193
🏁 Script executed:
Repository: TanStack/router
Length of output: 1035
🏁 Script executed:
Repository: TanStack/router
Length of output: 41
🏁 Script executed:
Repository: TanStack/router
Length of output: 148
🏁 Script executed:
Repository: TanStack/router
Length of output: 434
🏁 Script executed:
Repository: TanStack/router
Length of output: 3445
🏁 Script executed:
Repository: TanStack/router
Length of output: 2385
🏁 Script executed:
Repository: TanStack/router
Length of output: 1446
🏁 Script executed:
Repository: TanStack/router
Length of output: 4209
🏁 Script executed:
Repository: TanStack/router
Length of output: 347
Bind
useMatchto the/$idroute to access its parameters.The
Match()component is rendered inside the root route's match context. WhenuseMatch({ strict: false })is called without afromoption, it resolves to the nearest match—the root route—which has noidparameter. This causesmatch.params.idto always beundefined, falling back to 0, which doesn't reflect actual navigation state and reduces performance variation in this benchmark.Suggested fix
function Match() { const number = useMatch({ - strict: false, - select: (match) => runPerfSelectorComputation(Number(match.params.id ?? 0)), + from: '/$id', + shouldThrow: false, + select: (match) => runPerfSelectorComputation(Number(match?.params.id ?? 0)), }) return runPerfSelectorComputation(number) }📝 Committable suggestion
🤖 Prompt for AI Agents