fix(router-core): params.parse should run as minimally as possible - Alpha

packages/react-router/tests/useParams.test.tsx

@@ -1,4 +1,4 @@
-import { expect, test } from 'vitest'
+import { expect, test, vi } from 'vitest'
 import { act, fireEvent, render, screen } from '@testing-library/react'
 import {
   Link,
@@ -24,6 +24,7 @@ test('useParams must return parsed result if applicable.', async () => {
     },
   ]
 
+  const mockedfn = vi.fn()
   const rootRoute = createRootRoute()
 
   const postsRoute = createRoute({
@@ -70,23 +71,19 @@ test('useParams must return parsed result if applicable.', async () => {
   const postRoute = createRoute({
     getParentRoute: () => postCategoryRoute,
     path: '$postId',
+    loader: ({ params }) => {
+      return { post: posts.find((post) => post.id === parseInt(params.postId)) }
+    },
     params: {
       parse: (params) => {
+        mockedfn()
         return {
           ...params,
-          id: params.postId === 'one' ? 1 : 2,
-        }
-      },
-      stringify: (params) => {
-        return {
-          postId: params.id === 1 ? 'one' : 'two',
+          postId: params.postId === 'one' ? '1' : '2',
         }
       },
     },
     component: PostComponent,
-    loader: ({ params }) => ({
-      post: posts.find((post) => post.id === params.id),
-    }),
   })
 
   function PostsComponent() {
@@ -124,7 +121,8 @@ test('useParams must return parsed result if applicable.', async () => {
             <Link
               key={id}
               from={postCategoryRoute.fullPath}
-              to={`./${id}`}
+              to="./$postId"
+              params={{ postId: id }}
               data-testid={`post-${id}-link`}
             >
               {post.title}
@@ -152,9 +150,6 @@ test('useParams must return parsed result if applicable.', async () => {
           PostId_Param:{' '}
           <span data-testid="param_postId_value">{params.postId}</span>
         </div>
-        <div>
-          Id_Param: <span data-testid="param_id_value">{params.id}</span>
-        </div>
         <div>
           PostId: <span data-testid="post_id_value">{data.post.id}</span>
         </div>
@@ -187,75 +182,69 @@ test('useParams must return parsed result if applicable.', async () => {
 
   expect(firstCategoryLink).toBeInTheDocument()
 
+  mockedfn.mockClear()
   await act(() => fireEvent.click(firstCategoryLink))
 
-  expect(window.location.pathname).toBe('/posts/category_first')
-
-  const postCategoryHeading = await screen.findByTestId('post-category-heading')
   const firstPostLink = await screen.findByTestId('post-one-link')
 
-  expect(postCategoryHeading).toBeInTheDocument()
+  expect(window.location.pathname).toBe('/posts/category_first')
+  expect(await screen.findByTestId('post-category-heading')).toBeInTheDocument()
+  expect(mockedfn).toHaveBeenCalledTimes(1)
 
-  fireEvent.click(firstPostLink)
+  mockedfn.mockClear()
+  await act(() => fireEvent.click(firstPostLink))
 
-  let postHeading = await screen.findByTestId('post-heading')
+  const allCategoryLink = await screen.findByTestId('all-category-link')
   let paramCategoryValue = await screen.findByTestId('param_category_value')
   let paramPostIdValue = await screen.findByTestId('param_postId_value')
-  let paramIdValue = await screen.findByTestId('param_id_value')
   let postCategory = await screen.findByTestId('post_category_value')
   let postTitleValue = await screen.findByTestId('post_title_value')
   let postIdValue = await screen.findByTestId('post_id_value')
 
-  expect(window.location.pathname).toBe('/posts/category_first/one')
-  expect(postHeading).toBeInTheDocument()
-
   let renderedPost = {
     id: parseInt(postIdValue.textContent),
     title: postTitleValue.textContent,
     category: postCategory.textContent,
   }
 
+  expect(window.location.pathname).toBe('/posts/category_first/one')
+  expect(await screen.findByTestId('post-heading')).toBeInTheDocument()
   expect(renderedPost).toEqual(posts[0])
   expect(renderedPost.category).toBe('one')
   expect(paramCategoryValue.textContent).toBe('one')
-  expect(paramPostIdValue.textContent).toBe('one')
-  expect(paramIdValue.textContent).toBe('1')
-
-  const allCategoryLink = await screen.findByTestId('all-category-link')
-
+  expect(paramPostIdValue.textContent).toBe('1')
+  expect(mockedfn).toHaveBeenCalledTimes(2)
   expect(allCategoryLink).toBeInTheDocument()
 
+  mockedfn.mockClear()
   await act(() => fireEvent.click(allCategoryLink))
 
-  expect(window.location.pathname).toBe('/posts/category_all')
-
   const secondPostLink = await screen.findByTestId('post-two-link')
 
-  expect(postCategoryHeading).toBeInTheDocument()
+  expect(window.location.pathname).toBe('/posts/category_all')
+  expect(await screen.findByTestId('post-category-heading')).toBeInTheDocument()
   expect(secondPostLink).toBeInTheDocument()
+  expect(mockedfn).toHaveBeenCalledTimes(2)
 
-  fireEvent.click(secondPostLink)
+  mockedfn.mockClear()
+  await act(() => fireEvent.click(secondPostLink))
 
-  postHeading = await screen.findByTestId('post-heading')
   paramCategoryValue = await screen.findByTestId('param_category_value')
   paramPostIdValue = await screen.findByTestId('param_postId_value')
-  paramIdValue = await screen.findByTestId('param_id_value')
   postCategory = await screen.findByTestId('post_category_value')
   postTitleValue = await screen.findByTestId('post_title_value')
   postIdValue = await screen.findByTestId('post_id_value')
-
-  expect(window.location.pathname).toBe('/posts/category_all/two')
-  expect(postHeading).toBeInTheDocument()
-
   renderedPost = {
     id: parseInt(postIdValue.textContent),
     title: postTitleValue.textContent,
     category: postCategory.textContent,
   }
 
+  expect(window.location.pathname).toBe('/posts/category_all/two')
+  expect(await screen.findByTestId('post-heading')).toBeInTheDocument()
   expect(renderedPost).toEqual(posts[1])
   expect(renderedPost.category).toBe('two')
   expect(paramCategoryValue.textContent).toBe('all')
-  expect(paramPostIdValue.textContent).toBe('two')
-  expect(paramIdValue.textContent).toBe('2')
+  expect(paramPostIdValue.textContent).toBe('2')
+  expect(mockedfn).toHaveBeenCalledTimes(2)
 })

packages/router-core/src/router.ts

@@ -1202,33 +1202,6 @@ export class RouterCore<
       return rootRouteId
     })()
 
-    const parseErrors = matchedRoutes.map((route) => {
-      let parsedParamsError
-
-      const parseParams =
-        route.options.params?.parse ?? route.options.parseParams
-
-      if (parseParams) {
-        try {
-          const parsedParams = parseParams(routeParams)
-          // Add the parsed params to the accumulated params bag
-          Object.assign(routeParams, parsedParams)
-        } catch (err: any) {
-          parsedParamsError = new PathParamError(err.message, {
-            cause: err,
-          })
-
-          if (opts?.throwOnError) {
-            throw parsedParamsError
-          }
-
-          return parsedParamsError
-        }
-      }
-
-      return
-    })
-
     const matches: Array<AnyRouteMatch> = []
 
     const getParentContext = (parentMatch?: AnyRouteMatch) => {
@@ -1315,20 +1288,36 @@ export class RouterCore<
         parseCache: this.parsePathnameCache,
       })
 
-      const strictParams = interpolatePathResult.usedParams
+      // Waste not, want not. If we already have a match for this route,
+      // reuse it. This is important for layout routes, which might stick
+      // around between navigation actions that only change leaf routes.
+
+      // Existing matches are matches that are already loaded along with
+      // pending matches that are still loading
+      const matchId = interpolatePathResult.interpolatedPath + loaderDepsHash
+
+      const existingMatch = this.getMatch(matchId)
 
-      let paramsError = parseErrors[index]
+      const previousMatch = this.state.matches.find(
+        (d) => d.routeId === route.id,
+      )
 
-      const strictParseParams =
-        route.options.params?.parse ?? route.options.parseParams
+      const strictParams =
+        existingMatch?._strictParams ?? interpolatePathResult.usedParams
 
-      if (strictParseParams) {
-        try {
-          Object.assign(strictParams, strictParseParams(strictParams as any))
-        } catch (err: any) {
-          // any param errors should already have been dealt with above, if this
-          // somehow differs, let's report this in the same manner
-          if (!paramsError) {
+      let paramsError: PathParamError | undefined = undefined
+
+      if (!existingMatch) {
+        const strictParseParams =
+          route.options.params?.parse ?? route.options.parseParams
+
+        if (strictParseParams) {
+          try {
+            Object.assign(
+              strictParams,
+              strictParseParams(strictParams as Record<string, string>),
+            )
+          } catch (err: any) {
             paramsError = new PathParamError(err.message, {
               cause: err,
             })
@@ -1340,19 +1329,7 @@ export class RouterCore<
         }
       }
 
-      // Waste not, want not. If we already have a match for this route,
-      // reuse it. This is important for layout routes, which might stick
-      // around between navigation actions that only change leaf routes.
-
-      // Existing matches are matches that are already loaded along with
-      // pending matches that are still loading
-      const matchId = interpolatePathResult.interpolatedPath + loaderDepsHash
-
-      const existingMatch = this.getMatch(matchId)
-
-      const previousMatch = this.state.matches.find(
-        (d) => d.routeId === route.id,
-      )
+      Object.assign(routeParams, strictParams)
 
       const cause = previousMatch ? 'stay' : 'enter'
 
@@ -1398,7 +1375,7 @@ export class RouterCore<
           status,
           isFetching: false,
           error: undefined,
-          paramsError: paramsError,
+          paramsError,
           __routeContext: undefined,
           _nonReactive: {
             loadPromise: createControlledPromise(),