diff --git a/.changeset/early-static-route-matches.md b/.changeset/early-static-route-matches.md new file mode 100644 index 00000000000..6b738f11b53 --- /dev/null +++ b/.changeset/early-static-route-matches.md @@ -0,0 +1,5 @@ +--- +'@tanstack/router-core': patch +--- + +Fix route matching specificity so routes with the same number of static segments prefer the match whose static segment occurs earlier, including when competing with wildcard routes. diff --git a/packages/router-core/src/new-process-route-tree.ts b/packages/router-core/src/new-process-route-tree.ts index 6ba6a45c870..ea27d940fe8 100644 --- a/packages/router-core/src/new-process-route-tree.ts +++ b/packages/router-core/src/new-process-route-tree.ts @@ -1010,6 +1010,10 @@ function buildBranch(node: AnySegmentNode) { return list } +function appendStaticMask(staticMask: number, index: number) { + return (staticMask | (1 << index)) >>> 0 +} + type MatchStackFrame = { node: AnySegmentNode /** index of the segment of path */ @@ -1024,6 +1028,8 @@ type MatchStackFrame = { */ skipped: number statics: number + /** Bitmask of static match path indexes, used to prefer earlier static segments. Limited to 32 segments by JS bitwise ops. */ + staticMask: number dynamics: number optionals: number /** intermediary state for param extraction */ @@ -1067,6 +1073,7 @@ function getNodeMatch( skipped: 0, depth: 1, statics: 1, + staticMask: 0, dynamics: 0, optionals: 0, }, @@ -1078,7 +1085,16 @@ function getNodeMatch( while (stack.length) { const frame = stack.pop()! - const { node, index, skipped, depth, statics, dynamics, optionals } = frame + const { + node, + index, + skipped, + depth, + statics, + staticMask, + dynamics, + optionals, + } = frame let { extract, rawParams, parsedParams } = frame if (node.skipOnParamError) { @@ -1120,6 +1136,7 @@ function getNodeMatch( skipped, depth: depth + 1, statics, + staticMask, dynamics, optionals, extract, @@ -1169,6 +1186,7 @@ function getNodeMatch( skipped, depth, statics, + staticMask, dynamics, optionals, extract, @@ -1197,6 +1215,7 @@ function getNodeMatch( skipped: nextSkipped, depth: nextDepth, statics, + staticMask, dynamics, optionals, extract, @@ -1221,6 +1240,7 @@ function getNodeMatch( skipped, depth: nextDepth, statics, + staticMask, dynamics, optionals: optionals + 1, extract, @@ -1249,6 +1269,7 @@ function getNodeMatch( skipped, depth: depth + 1, statics, + staticMask, dynamics: dynamics + 1, optionals, extract, @@ -1270,6 +1291,7 @@ function getNodeMatch( skipped, depth: depth + 1, statics: statics + 1, + staticMask: appendStaticMask(staticMask, index), dynamics, optionals, extract, @@ -1289,6 +1311,7 @@ function getNodeMatch( skipped, depth: depth + 1, statics: statics + 1, + staticMask: appendStaticMask(staticMask, index), dynamics, optionals, extract, @@ -1309,6 +1332,7 @@ function getNodeMatch( skipped, depth: nextDepth, statics, + staticMask, dynamics, optionals, extract, @@ -1371,17 +1395,21 @@ function isFrameMoreSpecific( next: MatchStackFrame, ): boolean { if (!prev) return true + if (next.statics !== prev.statics) return next.statics > prev.statics + if (next.staticMask !== prev.staticMask) { + const diff = (next.staticMask ^ prev.staticMask) >>> 0 + const firstDifferentPosition = diff & -diff + return (next.staticMask & firstDifferentPosition) !== 0 + } return ( - next.statics > prev.statics || - (next.statics === prev.statics && - (next.dynamics > prev.dynamics || - (next.dynamics === prev.dynamics && - (next.optionals > prev.optionals || - (next.optionals === prev.optionals && - ((next.node.kind === SEGMENT_TYPE_INDEX) > - (prev.node.kind === SEGMENT_TYPE_INDEX) || - ((next.node.kind === SEGMENT_TYPE_INDEX) === - (prev.node.kind === SEGMENT_TYPE_INDEX) && - next.depth > prev.depth))))))) + next.dynamics > prev.dynamics || + (next.dynamics === prev.dynamics && + (next.optionals > prev.optionals || + (next.optionals === prev.optionals && + ((next.node.kind === SEGMENT_TYPE_INDEX) > + (prev.node.kind === SEGMENT_TYPE_INDEX) || + ((next.node.kind === SEGMENT_TYPE_INDEX) === + (prev.node.kind === SEGMENT_TYPE_INDEX) && + next.depth > prev.depth))))) ) } diff --git a/packages/router-core/tests/new-process-route-tree.bench.ts b/packages/router-core/tests/new-process-route-tree.bench.ts new file mode 100644 index 00000000000..c068d1a9551 --- /dev/null +++ b/packages/router-core/tests/new-process-route-tree.bench.ts @@ -0,0 +1,148 @@ +import { bench, describe } from 'vitest' +import { findRouteMatch, processRouteTree } from '../src/new-process-route-tree' + +type BenchRoute = { + id: string + isRoot?: boolean + fullPath: string + path: string + children?: Array +} + +const BENCH_OPTIONS = { + warmupIterations: 100, + warmupTime: 1_500, + time: 3_000, +} + +let benchmarkSink = 0 + +function makeRouteTree(routes: Array): BenchRoute { + return { + id: '__root__', + isRoot: true, + fullPath: '/', + path: '/', + children: routes.map((route) => ({ + id: route, + fullPath: route, + path: route, + })), + } +} + +const unitDerivedPatterns = [ + // Derived from the priority and edge-case tests in new-process-route-tree.test.ts. + '/{-$a}/b', + '/a/$', + '/a/{-$b}', + '/{-$a}/{-$b}/{-$c}/d/e', + '/{-$a}/{-$b}/c/d/{-$e}', + '/$a/$b/$c/d/e', + '/$a/$b/c/d/$e', + '/users/{-$org}/settings', + '/users/$id', + '/{-$other}/posts/new', + '/posts/$id', + '/a/{-$b}/', + '/a/{-$b}/$c', + '/foo/{-$p}.tsx', + '/foo/{-$p}/{-$x}.tsx', + '/file{$}', + '/{$}/c/file', +] + +const unitDerivedMatchPaths = [ + '/a/b', + '/a/c', + '/a/b/c', + '/a/b/c/d/e', + '/users/settings', + '/posts/new', + '/foo/bar.tsx', + '/file/a/b/c', + '/x/y/c/file', +] + +function prefixed(pattern: string, index: number) { + return `/g${index}${pattern}` +} + +function makeBigRoutes(groupCount: number) { + const routes: Array = [] + for (let i = 0; i < groupCount; i++) { + for (const pattern of unitDerivedPatterns) { + routes.push(prefixed(pattern, i)) + } + } + return routes +} + +function makeBigMatchPaths(groupCount: number) { + const paths: Array = [] + for (let i = 0; i < groupCount; i++) { + for (const path of unitDerivedMatchPaths) { + paths.push(`/g${i}${path}`) + } + } + return paths +} + +const routeSuites = [ + { + name: 'small route tree', + routes: unitDerivedPatterns, + matchPaths: unitDerivedMatchPaths, + buildIterations: 3_000, + matchIterations: 3_000, + }, + { + name: 'big route tree', + routes: makeBigRoutes(128), + matchPaths: makeBigMatchPaths(128), + buildIterations: 20, + matchIterations: 20, + }, +] + +describe.each(routeSuites)( + 'new process route tree - $name', + ({ routes, matchPaths, buildIterations, matchIterations }) => { + const routeTree = makeRouteTree(routes) + const processedTree = processRouteTree(routeTree).processedTree + + bench( + `build ${routes.length} routes x ${buildIterations}`, + () => { + for (let i = 0; i < buildIterations; i++) { + const result = processRouteTree(routeTree) + benchmarkSink ^= result.routesById[routes[0]!] ? 1 : 0 + } + }, + BENCH_OPTIONS, + ) + + bench( + `match ${matchPaths.length} paths x ${matchIterations} (uncached)`, + () => { + let matches = 0 + + for (let i = 0; i < matchIterations; i++) { + processedTree.matchCache.clear() + + for (const path of matchPaths) { + if (findRouteMatch(path, processedTree)) matches++ + } + } + + const expectedMatches = matchPaths.length * matchIterations + if (matches !== expectedMatches) { + throw new Error(`Expected ${expectedMatches} matches, got ${matches}`) + } + + benchmarkSink ^= matches + }, + BENCH_OPTIONS, + ) + }, +) diff --git a/packages/router-core/tests/new-process-route-tree.test.ts b/packages/router-core/tests/new-process-route-tree.test.ts index 936df988c14..35280984a22 100644 --- a/packages/router-core/tests/new-process-route-tree.test.ts +++ b/packages/router-core/tests/new-process-route-tree.test.ts @@ -233,6 +233,18 @@ describe('findRouteMatch', () => { const treeWithIndex = makeTree(['/a/', '/a/{-$b}']) expect(findRouteMatch('/a', treeWithIndex)?.route.id).toBe('/a/') }) + it('optional+static vs static+wildcard', () => { + const tree = makeTree(['/{-$a}/b', '/a/$']) + expect(findRouteMatch('/a/b', tree)?.route.id).toBe('/a/$') + }) + it('dynamic+static vs static+wildcard', () => { + const tree = makeTree(['/$a/b', '/a/$']) + expect(findRouteMatch('/a/b', tree)?.route.id).toBe('/a/$') + }) + it('optional+static vs static+optional', () => { + const tree = makeTree(['/{-$a}/b', '/a/{-$b}']) + expect(findRouteMatch('/a/b', tree)?.route.id).toBe('/a/{-$b}') + }) }) })