refactor(router-core): skip beforeLoad and related store updates if options is not defined

packages/react-router/tests/store-updates-during-navigation.test.tsx

@@ -1,12 +1,19 @@
 import { afterEach, describe, expect, test, vi } from 'vitest'
-import { act, cleanup, render, screen, waitFor } from '@testing-library/react'
+import {
+  cleanup,
+  fireEvent,
+  render,
+  screen,
+  waitFor,
+} from '@testing-library/react'
 import {
   Link,
   Outlet,
   RouterProvider,
   createRootRoute,
   createRoute,
   createRouter,
+  notFound,
   redirect,
   useRouterState,
 } from '../src'
@@ -74,21 +81,23 @@ function setup({
     defaultPendingMs,
     defaultPendingMinMs,
     defaultPendingComponent: () => <p>Loading...</p>,
+    defaultNotFoundComponent: () => <h1>Not Found Title</h1>,
+    defaultPreload: 'intent',
   })
 
   render(<RouterProvider router={router} />)
 
   return { select, router }
 }
 
-async function run({ select }: ReturnType<typeof setup>, opts?: {}) {
+async function run({ select }: ReturnType<typeof setup>) {
   // navigate to /posts
   const link = await waitFor(() => screen.getByRole('link', { name: 'Posts' }))
   const before = select.mock.calls.length
-  act(() => link.click())
-  const title = await waitFor(() =>
-    screen.getByRole('heading', { name: /Title$/ }),
-  ) // matches /posts and /other
+  fireEvent.click(link)
+  const title = await waitFor(
+    () => screen.getByRole('heading', { name: /Title$/ }), // matches /posts and /other and not found
+  )
   expect(title).toBeInTheDocument()
   const after = select.mock.calls.length
 
@@ -110,7 +119,7 @@ describe("Store doesn't update *too many* times during navigation", () => {
     // This number should be as small as possible to minimize the amount of work
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
-    expect(updates).toBe(17)
+    expect(updates).toBe(14)
   })
 
   test('redirection in preload', async () => {
@@ -125,9 +134,77 @@ describe("Store doesn't update *too many* times during navigation", () => {
     const after = select.mock.calls.length
     const updates = after - before
 
+    // This number should be as small as possible to minimize the amount of work
+    // that needs to be done during a navigation.
+    // Any change that increases this number should be investigated.
+    expect(updates).toBe(6)
+  })
+
+  test('sync beforeLoad', async () => {
+    const params = setup({
+      beforeLoad: () => ({ foo: 'bar' }),
+      loader: () => new Promise<void>((resolve) => setTimeout(resolve, 100)),
+      defaultPendingMs: 100,
+      defaultPendingMinMs: 300,
+    })
+
+    const updates = await run(params)
+
+    // This number should be as small as possible to minimize the amount of work
+    // that needs to be done during a navigation.
+    // Any change that increases this number should be investigated.
+    expect(updates).toBe(13)
+  })
+
+  test('nothing', async () => {
+    const params = setup({})
+
+    const updates = await run(params)
+
+    // This number should be as small as possible to minimize the amount of work
+    // that needs to be done during a navigation.
+    // Any change that increases this number should be investigated.
+    expect(updates).toBeGreaterThanOrEqual(10) // WARN: this is flaky, and sometimes (rarely) is 11
+    expect(updates).toBeLessThanOrEqual(11)
+  })
+
+  test('not found in beforeLoad', async () => {
+    const params = setup({
+      beforeLoad: () => {
+        throw notFound()
+      },
+    })
+
+    const updates = await run(params)
+
     // This number should be as small as possible to minimize the amount of work
     // that needs to be done during a navigation.
     // Any change that increases this number should be investigated.
     expect(updates).toBe(8)
   })
+
+  test('hover preload, then navigate, w/ async loaders', async () => {
+    const { select } = setup({
+      beforeLoad: () => Promise.resolve({ foo: 'bar' }),
+      loader: () => Promise.resolve({ hello: 'world' }),
+    })
+
+    const link = await waitFor(() =>
+      screen.getByRole('link', { name: 'Posts' }),
+    )
+    const before = select.mock.calls.length
+    fireEvent.focus(link)
+    fireEvent.click(link)
+    const title = await waitFor(() =>
+      screen.getByRole('heading', { name: /Title$/ }),
+    )
+    expect(title).toBeInTheDocument()
+    const after = select.mock.calls.length
+    const updates = after - before
+
+    // This number should be as small as possible to minimize the amount of work
+    // that needs to be done during a navigation.
+    // Any change that increases this number should be investigated.
+    expect(updates).toBe(19)
+  })
 })

packages/router-core/src/router.ts

@@ -2318,11 +2318,9 @@ export class RouterCore<
     const match = this.getMatch(matchId)!
     if (shouldPending && match._nonReactive.pendingTimeout === undefined) {
       const pendingTimeout = setTimeout(() => {
-        try {
-          // Update the match and prematurely resolve the loadMatches promise so that
-          // the pending component can start rendering
-          this.triggerOnReady(innerLoadContext)
-        } catch {}
+        // Update the match and prematurely resolve the loadMatches promise so that
+        // the pending component can start rendering
+        this.triggerOnReady(innerLoadContext)
       }, pendingMs)
       match._nonReactive.pendingTimeout = pendingTimeout
     }
@@ -2371,131 +2369,150 @@ export class RouterCore<
     index: number,
     route: AnyRoute,
   ): void | Promise<void> => {
-    const resolve = () => {
-      innerLoadContext.updateMatch(matchId, (prev) => {
-        prev._nonReactive.beforeLoadPromise?.resolve()
-        prev._nonReactive.beforeLoadPromise = undefined
+    const match = this.getMatch(matchId)!
 
-        return {
-          ...prev,
-          isFetching: false,
-        }
-      })
-    }
+    match._nonReactive.beforeLoadPromise = createControlledPromise<void>()
+    // explicitly capture the previous loadPromise
+    const prevLoadPromise = match._nonReactive.loadPromise
+    match._nonReactive.loadPromise = createControlledPromise<void>(() => {
+      prevLoadPromise?.resolve()
+    })
 
-    try {
-      const match = this.getMatch(matchId)!
-      match._nonReactive.beforeLoadPromise = createControlledPromise<void>()
-      // explicitly capture the previous loadPromise
-      const prevLoadPromise = match._nonReactive.loadPromise
-      match._nonReactive.loadPromise = createControlledPromise<void>(() => {
-        prevLoadPromise?.resolve()
-      })
+    const { paramsError, searchError } = match
 
-      const { paramsError, searchError } = this.getMatch(matchId)!
+    if (paramsError) {
+      this.handleSerialError(
+        innerLoadContext,
+        index,
+        paramsError,
+        'PARSE_PARAMS',
+      )
+    }
 
-      if (paramsError) {
-        this.handleSerialError(
-          innerLoadContext,
-          index,
-          paramsError,
-          'PARSE_PARAMS',
-        )
-      }
+    if (searchError) {
+      this.handleSerialError(
+        innerLoadContext,
+        index,
+        searchError,
+        'VALIDATE_SEARCH',
+      )
+    }
 
-      if (searchError) {
-        this.handleSerialError(
-          innerLoadContext,
-          index,
-          searchError,
-          'VALIDATE_SEARCH',
-        )
-      }
+    this.setupPendingTimeout(innerLoadContext, matchId, route)
 
-      this.setupPendingTimeout(innerLoadContext, matchId, route)
+    const abortController = new AbortController()
 
-      const abortController = new AbortController()
+    const parentMatchId = innerLoadContext.matches[index - 1]?.id
+    const parentMatch = parentMatchId
+      ? this.getMatch(parentMatchId)!
+      : undefined
+    const parentMatchContext =
+      parentMatch?.context ?? this.options.context ?? undefined
 
-      const parentMatchId = innerLoadContext.matches[index - 1]?.id
-      const parentMatch = parentMatchId
-        ? this.getMatch(parentMatchId)!
-        : undefined
-      const parentMatchContext =
-        parentMatch?.context ?? this.options.context ?? undefined
+    const context = { ...parentMatchContext, ...match.__routeContext }
 
+    let isPending = false
+    const pending = () => {
+      if (isPending) return
+      isPending = true
       innerLoadContext.updateMatch(matchId, (prev) => ({
         ...prev,
         isFetching: 'beforeLoad',
         fetchCount: prev.fetchCount + 1,
         abortController,
-        context: {
-          ...parentMatchContext,
-          ...prev.__routeContext,
-        },
+        context,
+      }))
+    }
+
+    const resolve = () => {
+      match._nonReactive.beforeLoadPromise?.resolve()
+      match._nonReactive.beforeLoadPromise = undefined
+      innerLoadContext.updateMatch(matchId, (prev) => ({
+        ...prev,
+        isFetching: false,
       }))
+    }
 
-      const { search, params, context, cause } = this.getMatch(matchId)!
+    // if there is no `beforeLoad` option, skip everything, batch update the store, return early
+    if (!route.options.beforeLoad) {
+      batch(() => {
+        pending()
+        resolve()
+      })
+      return
+    }
 
-      const preload = this.resolvePreload(innerLoadContext, matchId)
+    const { search, params, cause } = match
+    const preload = this.resolvePreload(innerLoadContext, matchId)
+    const beforeLoadFnContext: BeforeLoadContextOptions<
+      any,
+      any,
+      any,
+      any,
+      any
+    > = {
+      search,
+      abortController,
+      params,
+      preload,
+      context,
+      location: innerLoadContext.location,
+      navigate: (opts: any) =>
+        this.navigate({ ...opts, _fromLocation: innerLoadContext.location }),
+      buildLocation: this.buildLocation,
+      cause: preload ? 'preload' : cause,
+      matches: innerLoadContext.matches,
+    }
 
-      const beforeLoadFnContext: BeforeLoadContextOptions<
-        any,
-        any,
-        any,
-        any,
-        any
-      > = {
-        search,
-        abortController,
-        params,
-        preload,
-        context,
-        location: innerLoadContext.location,
-        navigate: (opts: any) =>
-          this.navigate({ ...opts, _fromLocation: innerLoadContext.location }),
-        buildLocation: this.buildLocation,
-        cause: preload ? 'preload' : cause,
-        matches: innerLoadContext.matches,
+    const updateContext = (beforeLoadContext: any) => {
+      if (beforeLoadContext === undefined) {
+        batch(() => {
+          pending()
+          resolve()
+        })
+        return
+      }
+      if (isRedirect(beforeLoadContext) || isNotFound(beforeLoadContext)) {
+        pending()
+        this.handleSerialError(
+          innerLoadContext,
+          index,
+          beforeLoadContext,
+          'BEFORE_LOAD',
+        )
       }
 
-      const updateContext = (beforeLoadContext: any) => {
-        if (isRedirect(beforeLoadContext) || isNotFound(beforeLoadContext)) {
-          this.handleSerialError(
-            innerLoadContext,
-            index,
-            beforeLoadContext,
-            'BEFORE_LOAD',
-          )
-        }
-
+      batch(() => {
+        pending()
         innerLoadContext.updateMatch(matchId, (prev) => ({
           ...prev,
           __beforeLoadContext: beforeLoadContext,
           context: {
-            ...parentMatchContext,
-            ...prev.__routeContext,
+            ...prev.context,
             ...beforeLoadContext,
           },
-          abortController,
         }))
-      }
+        resolve()
+      })
+    }
 
-      const beforeLoadContext = route.options.beforeLoad?.(beforeLoadFnContext)
+    let beforeLoadContext
+    try {
+      beforeLoadContext = route.options.beforeLoad(beforeLoadFnContext)
       if (isPromise(beforeLoadContext)) {
+        pending()
         return beforeLoadContext
-          .then(updateContext)
           .catch((err) => {
             this.handleSerialError(innerLoadContext, index, err, 'BEFORE_LOAD')
           })
-          .then(resolve)
-      } else {
-        updateContext(beforeLoadContext)
+          .then(updateContext)
       }
     } catch (err) {
+      pending()
       this.handleSerialError(innerLoadContext, index, err, 'BEFORE_LOAD')
     }
 
-    resolve()
+    updateContext(beforeLoadContext)
     return
   }
 
@@ -2709,7 +2726,8 @@ export class RouterCore<
       } catch (e) {
         let error = e
 
-        await this.potentialPendingMinPromise(matchId)
+        const pendingPromise = this.potentialPendingMinPromise(matchId)
+        if (pendingPromise) await pendingPromise
 
         this.handleRedirectAndNotFound(
           innerLoadContext,