diff --git a/packages/router-core/src/new-process-route-tree.ts b/packages/router-core/src/new-process-route-tree.ts index b2eb99ab01..10253e2015 100644 --- a/packages/router-core/src/new-process-route-tree.ts +++ b/packages/router-core/src/new-process-route-tree.ts @@ -709,6 +709,8 @@ type RouteMatch> = { route: T params: Record branch: ReadonlyArray + /** Parsed params from routes with skipRouteOnParseError, accumulated during matching */ + parsedParams?: Record } export function findRouteMatch< @@ -804,7 +806,11 @@ function findMatch( path: string, segmentTree: AnySegmentNode, fuzzy = false, -): { route: T; params: Record } | null { +): { + route: T + params: Record + parsedParams?: Record +} | null { const parts = path.split('/') const leaf = getNodeMatch(path, parts, segmentTree, fuzzy) if (!leaf) return null @@ -813,6 +819,7 @@ function findMatch( return { route, params, + parsedParams: leaf.parsedParams, } } @@ -938,10 +945,10 @@ type MatchStackFrame = { optionals: number /** intermediary state for param extraction */ extract?: { part: number; node: number; path: number } - /** intermediary params from param extraction */ - // TODO: I'm not sure, but I think we need both the raw strings for `interpolatePath` and the parsed values for the final match object - // I think they can still be accumulated (separately) in a single object (each) because `interpolatePath` returns the `usedParams` anyway + /** intermediary raw string params from param extraction (for interpolatePath) */ params?: Record + /** intermediary parsed params from routes with skipRouteOnParseError */ + parsedParams?: Record } function getNodeMatch( @@ -953,7 +960,7 @@ function getNodeMatch( // quick check for root index // this is an optimization, algorithm should work correctly without this block if (path === '/' && segmentTree.index) - return { node: segmentTree.index, skipped: 0 } + return { node: segmentTree.index, skipped: 0, parsedParams: undefined } const trailingSlash = !last(parts) const pathIsIndex = trailingSlash && path !== '/' @@ -987,13 +994,14 @@ function getNodeMatch( while (stack.length) { const frame = stack.pop()! const { node, index, skipped, depth, statics, dynamics, optionals } = frame - let { extract, params } = frame + let { extract, params, parsedParams } = frame if (node.skipRouteOnParseError && node.parse) { const result = validateMatchParams(path, parts, frame) if (!result) continue params = result[0] extract = result[1] + parsedParams = frame.parsedParams } // In fuzzy mode, track the best partial match we've found so far @@ -1030,6 +1038,7 @@ function getNodeMatch( optionals, extract, params, + parsedParams, } if (node.index.skipRouteOnParseError && node.index.parse) { const result = validateMatchParams(path, parts, indexFrame) @@ -1075,6 +1084,7 @@ function getNodeMatch( optionals, extract, params, + parsedParams, } if (segment.skipRouteOnParseError && segment.parse) { const result = validateMatchParams(path, parts, frame) @@ -1102,6 +1112,7 @@ function getNodeMatch( optionals, extract, params, + parsedParams, }) // enqueue skipping the optional } if (!isBeyondPath) { @@ -1125,6 +1136,7 @@ function getNodeMatch( optionals: optionals + 1, extract, params, + parsedParams, }) } } @@ -1152,6 +1164,7 @@ function getNodeMatch( optionals, extract, params, + parsedParams, }) } } @@ -1172,6 +1185,7 @@ function getNodeMatch( optionals, extract, params, + parsedParams, }) } } @@ -1190,6 +1204,7 @@ function getNodeMatch( optionals, extract, params, + parsedParams, }) } } @@ -1209,6 +1224,7 @@ function getNodeMatch( optionals, extract, params, + parsedParams, }) } } @@ -1247,8 +1263,9 @@ function validateMatchParams( const result = extractParams(path, parts, frame) frame.params = result[0] frame.extract = result[1] - result[0] = frame.node.parse!(result[0]) - frame.params = result[0] + const parsedParams = frame.node.parse!(result[0]) + // Accumulate parsed params from this route + frame.parsedParams = { ...frame.parsedParams, ...parsedParams } return result } catch { return null diff --git a/packages/router-core/src/router.ts b/packages/router-core/src/router.ts index ffd4de77aa..bbc21f5113 100644 --- a/packages/router-core/src/router.ts +++ b/packages/router-core/src/router.ts @@ -701,6 +701,7 @@ export type GetMatchRoutesFn = (pathname: string) => { routeParams: Record foundRoute: AnyRoute | undefined parseError?: unknown + parsedParams?: Record } export type EmitFn = (routerEvent: RouterEvent) => void @@ -1249,7 +1250,7 @@ export class RouterCore< opts?: MatchRoutesOpts, ): Array { const matchedRoutesResult = this.getMatchedRoutes(next.pathname) - const { foundRoute, routeParams } = matchedRoutesResult + const { foundRoute, routeParams, parsedParams } = matchedRoutesResult let { matchedRoutes } = matchedRoutesResult let isGlobalNotFound = false @@ -1393,7 +1394,13 @@ export class RouterCore< const strictParseParams = route.options.params?.parse ?? route.options.parseParams - if (strictParseParams) { + // Skip parsing if this route was already parsed during matching (skipRouteOnParseError) + const alreadyParsed = + route.options.skipRouteOnParseError && + strictParseParams && + parsedParams !== undefined + + if (strictParseParams && !alreadyParsed) { try { Object.assign( strictParams, @@ -1412,6 +1419,9 @@ export class RouterCore< throw paramsError } } + } else if (alreadyParsed) { + // Use the pre-parsed params from matching + Object.assign(strictParams, parsedParams) } } @@ -2693,15 +2703,17 @@ export function getMatchedRoutes({ const trimmedPath = trimPathRight(pathname) let foundRoute: TRouteLike | undefined = undefined + let parsedParams: Record | undefined = undefined const match = findRouteMatch(trimmedPath, processedTree, true) if (match) { foundRoute = match.route Object.assign(routeParams, match.params) // Copy params, because they're cached + parsedParams = match.parsedParams } const matchedRoutes = match?.branch || [routesById[rootRouteId]!] - return { matchedRoutes, routeParams, foundRoute } + return { matchedRoutes, routeParams, foundRoute, parsedParams } } function applySearchMiddleware({ diff --git a/packages/router-core/tests/skipRouteOnParseError.test.ts b/packages/router-core/tests/skipRouteOnParseError.test.ts new file mode 100644 index 0000000000..ae124cad5e --- /dev/null +++ b/packages/router-core/tests/skipRouteOnParseError.test.ts @@ -0,0 +1,250 @@ +import { describe, expect, it, vi } from 'vitest' +import { createMemoryHistory } from '@tanstack/history' +import { BaseRootRoute, BaseRoute, RouterCore } from '../src' + +describe('skipRouteOnParseError optimization', () => { + it('should call params.parse only once for routes with skipRouteOnParseError', async () => { + const rootRoute = new BaseRootRoute() + + const parseSpy = vi.fn((params: { id: string }) => ({ + id: Number(params.id), + })) + + const route = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/posts/$id', + params: { + parse: parseSpy, + }, + skipRouteOnParseError: true, + }) + + const routeTree = rootRoute.addChildren([route]) + + const router = new RouterCore({ + routeTree, + history: createMemoryHistory({ initialEntries: ['/posts/123'] }), + }) + + await router.load() + + // params.parse should be called exactly once during matching + expect(parseSpy).toHaveBeenCalledTimes(1) + expect(parseSpy).toHaveBeenCalledWith({ id: '123' }) + + // Verify the parsed params are available in the match + const match = router.state.matches.find((m) => m.routeId === '/posts/$id') + expect(match?.params).toEqual({ id: 123 }) + }) + + it('should call params.parse for nested routes with skipRouteOnParseError only once each', async () => { + const rootRoute = new BaseRootRoute() + + const parentParseSpy = vi.fn((params: { userId: string }) => ({ + userId: Number(params.userId), + })) + + const childParseSpy = vi.fn((params: { postId: string }) => ({ + postId: Number(params.postId), + })) + + const userRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/users/$userId', + params: { + parse: parentParseSpy, + }, + skipRouteOnParseError: true, + }) + + const postRoute = new BaseRoute({ + getParentRoute: () => userRoute, + path: 'posts/$postId', + params: { + parse: childParseSpy, + }, + skipRouteOnParseError: true, + }) + + const routeTree = rootRoute.addChildren([ + userRoute.addChildren([postRoute]), + ]) + + const router = new RouterCore({ + routeTree, + history: createMemoryHistory({ + initialEntries: ['/users/456/posts/789'], + }), + }) + + await router.load() + + // Each params.parse should be called exactly once during matching + expect(parentParseSpy).toHaveBeenCalledTimes(1) + expect(parentParseSpy).toHaveBeenCalledWith({ userId: '456' }) + + expect(childParseSpy).toHaveBeenCalledTimes(1) + expect(childParseSpy).toHaveBeenCalledWith({ userId: '456', postId: '789' }) + + // Verify the parsed params are available and accumulated + const userMatch = router.state.matches.find( + (m) => m.routeId === '/users/$userId', + ) + expect(userMatch?.params).toEqual({ userId: 456, postId: 789 }) + + const postMatch = router.state.matches.find( + (m) => m.routeId === '/users/$userId/posts/$postId', + ) + expect(postMatch?.params).toEqual({ userId: 456, postId: 789 }) + }) + + it('should still call params.parse for routes WITHOUT skipRouteOnParseError', async () => { + const rootRoute = new BaseRootRoute() + + const parseSpy = vi.fn((params: { id: string }) => ({ + id: Number(params.id), + })) + + const route = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/posts/$id', + params: { + parse: parseSpy, + }, + // skipRouteOnParseError is NOT set (defaults to false) + }) + + const routeTree = rootRoute.addChildren([route]) + + const router = new RouterCore({ + routeTree, + history: createMemoryHistory({ initialEntries: ['/posts/123'] }), + }) + + await router.load() + + // params.parse should be called during matchRoutesInternal (not during matching) + expect(parseSpy).toHaveBeenCalledTimes(1) + // Note: receives parsed params because parent doesn't exist, so strictParams contains the parsed value + expect(parseSpy).toHaveBeenCalledWith({ id: 123 }) + + // Verify the parsed params are available + const match = router.state.matches.find((m) => m.routeId === '/posts/$id') + expect(match?.params).toEqual({ id: 123 }) + }) + + it('should skip route during matching if params.parse throws with skipRouteOnParseError', async () => { + const rootRoute = new BaseRootRoute() + + const strictParseSpy = vi.fn((params: { id: string }) => { + const num = Number(params.id) + if (isNaN(num)) { + throw new Error('Invalid ID') + } + return { id: num } + }) + + const fallbackParseSpy = vi.fn((params: { slug: string }) => ({ + slug: params.slug, + })) + + const strictRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/posts/$id', + params: { + parse: strictParseSpy, + }, + skipRouteOnParseError: true, + }) + + const fallbackRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/posts/$slug', + params: { + parse: fallbackParseSpy, + }, + }) + + const routeTree = rootRoute.addChildren([strictRoute, fallbackRoute]) + + const router = new RouterCore({ + routeTree, + history: createMemoryHistory({ initialEntries: ['/posts/invalid'] }), + }) + + await router.load() + + // strictParseSpy should be called and throw, causing the route to be skipped + expect(strictParseSpy).toHaveBeenCalledTimes(1) + expect(strictParseSpy).toHaveBeenCalledWith({ id: 'invalid' }) + + // fallbackParseSpy should be called for the fallback route + expect(fallbackParseSpy).toHaveBeenCalledTimes(1) + expect(fallbackParseSpy).toHaveBeenCalledWith({ slug: 'invalid' }) + + // Verify we matched the fallback route, not the strict route + const matches = router.state.matches.map((m) => m.routeId) + expect(matches).toContain('/posts/$slug') + expect(matches).not.toContain('/posts/$id') + }) + + it('should handle mixed routes with and without skipRouteOnParseError', async () => { + const rootRoute = new BaseRootRoute() + + const skipParseSpy = vi.fn((params: { userId: string }) => ({ + userId: Number(params.userId), + })) + + const normalParseSpy = vi.fn((params: { postId: string }) => ({ + postId: Number(params.postId), + })) + + const userRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/users/$userId', + params: { + parse: skipParseSpy, + }, + skipRouteOnParseError: true, + }) + + const postRoute = new BaseRoute({ + getParentRoute: () => userRoute, + path: 'posts/$postId', + params: { + parse: normalParseSpy, + }, + // skipRouteOnParseError NOT set + }) + + const routeTree = rootRoute.addChildren([ + userRoute.addChildren([postRoute]), + ]) + + const router = new RouterCore({ + routeTree, + history: createMemoryHistory({ + initialEntries: ['/users/456/posts/789'], + }), + }) + + await router.load() + + // skipParseSpy should be called once during matching + expect(skipParseSpy).toHaveBeenCalledTimes(1) + + // normalParseSpy should be called once during matchRoutesInternal + expect(normalParseSpy).toHaveBeenCalledTimes(1) + + // Both should have correct params + const userMatch = router.state.matches.find( + (m) => m.routeId === '/users/$userId', + ) + expect(userMatch?.params).toEqual({ userId: 456, postId: 789 }) + + const postMatch = router.state.matches.find( + (m) => m.routeId === '/users/$userId/posts/$postId', + ) + expect(postMatch?.params).toEqual({ userId: 456, postId: 789 }) + }) +})