refactor(router-core): head, scripts, headers are executed in parallel and can skip store update (#4925)

Following #4916, this PR keeps
reducing the number of store updates (`__store.setState` through
`updateMatch`).

We can skip `route.options.head()`, `route.options.scripts()`, and
`route.options.headers()` calls, if they are not defined, and not even
call `updateMatch` after.

Additionally, `head`, `scripts` and `headers` are now called in
parallel. They don't depend on one another, and so there is no reason to
delay their execution by calling them serially.

---

This PR also fixes 2 issues:
- in `cancelMatch`, we were resetting the timeout id to `undefined`
**before** calling `clearTimeout()` with it
- `handleRedirectAndNotFound` is sometimes called w/ `match` being
`undefined` (in case of a redirecting match during preload), which went
undetected because of heavy `match!` assertions, and the many
`try/catch` blocks

---

With this PR, the `redirection in preload` test in
`store-updates-during-navigation` goes from **11 updates** during the
preload to **8 updates**.

---

Should be a partial improvement of
#4359

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>

Author: 2 people

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

@@ -16,7 +16,7 @@ afterEach(() => {
   cleanup()
 })
 
-async function setupAndRun({
+function setup({
   beforeLoad,
   loader,
   head,
@@ -78,6 +78,10 @@ async function setupAndRun({
 
   render(<RouterProvider router={router} />)
 
+  return { select, router }
+}
+
+async function run({ select }: ReturnType<typeof setup>, opts?: {}) {
   // navigate to /posts
   const link = await waitFor(() => screen.getByRole('link', { name: 'Posts' }))
   const before = select.mock.calls.length
@@ -93,30 +97,37 @@ async function setupAndRun({
 
 describe("Store doesn't update *too many* times during navigation", () => {
   test('async loader, async beforeLoad, pendingMs', async () => {
-    const updates = await setupAndRun({
+    const params = setup({
       beforeLoad: () =>
         new Promise<void>((resolve) => setTimeout(resolve, 100)),
       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(19)
   })
 
-  test('redirection', async () => {
-    const updates = await setupAndRun({
-      beforeLoad: () => {
+  test('redirection in preload', async () => {
+    const { select, router } = setup({
+      loader: () => {
         throw redirect({ to: '/other' })
       },
     })
 
+    const before = select.mock.calls.length
+    await router.preloadRoute({ to: '/posts' })
+    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(26)
+    expect(updates).toBe(8)
   })
 })

packages/router-core/src/router.ts

@@ -1386,8 +1386,8 @@ export class RouterCore<
     if (!match) return
 
     match.abortController.abort()
-    match._nonReactive.pendingTimeout = undefined
     clearTimeout(match._nonReactive.pendingTimeout)
+    match._nonReactive.pendingTimeout = undefined
   }
 
   cancelMatches = () => {
@@ -2114,7 +2114,10 @@ export class RouterCore<
       triggerOnReady()
     }
 
-    const handleRedirectAndNotFound = (match: AnyRouteMatch, err: any) => {
+    const handleRedirectAndNotFound = (
+      match: AnyRouteMatch | undefined,
+      err: any,
+    ) => {
       if (isRedirect(err) || isNotFound(err)) {
         if (isRedirect(err)) {
           if (err.redirectHandled) {
@@ -2124,27 +2127,30 @@ export class RouterCore<
           }
         }
 
-        match._nonReactive.beforeLoadPromise?.resolve()
-        match._nonReactive.loaderPromise?.resolve()
-        match._nonReactive.beforeLoadPromise = undefined
-        match._nonReactive.loaderPromise = undefined
-
-        updateMatch(match.id, (prev) => ({
-          ...prev,
-          status: isRedirect(err)
-            ? 'redirected'
-            : isNotFound(err)
-              ? 'notFound'
-              : 'error',
-          isFetching: false,
-          error: err,
-        }))
+        // in case of a redirecting match during preload, the match does not exist
+        if (match) {
+          match._nonReactive.beforeLoadPromise?.resolve()
+          match._nonReactive.loaderPromise?.resolve()
+          match._nonReactive.beforeLoadPromise = undefined
+          match._nonReactive.loaderPromise = undefined
+
+          updateMatch(match.id, (prev) => ({
+            ...prev,
+            status: isRedirect(err)
+              ? 'redirected'
+              : isNotFound(err)
+                ? 'notFound'
+                : 'error',
+            isFetching: false,
+            error: err,
+          }))
 
-        if (!(err as any).routeId) {
-          ;(err as any).routeId = match.routeId
-        }
+          if (!(err as any).routeId) {
+            ;(err as any).routeId = match.routeId
+          }
 
-        match._nonReactive.loadPromise?.resolve()
+          match._nonReactive.loadPromise?.resolve()
+        }
 
         if (isRedirect(err)) {
           rendered = true
@@ -2197,13 +2203,13 @@ export class RouterCore<
 
               err.routerCode = routerCode
               firstBadMatchIndex = firstBadMatchIndex ?? index
-              handleRedirectAndNotFound(this.getMatch(matchId)!, err)
+              handleRedirectAndNotFound(this.getMatch(matchId), err)
 
               try {
                 route.options.onError?.(err)
               } catch (errorHandlerErr) {
                 err = errorHandlerErr
-                handleRedirectAndNotFound(this.getMatch(matchId)!, err)
+                handleRedirectAndNotFound(this.getMatch(matchId), err)
               }
 
               updateMatch(matchId, (prev) => {
@@ -2458,35 +2464,45 @@ export class RouterCore<
                   let loaderIsRunningAsync = false
                   const route = this.looseRoutesById[routeId]!
 
-                  const executeHead = async () => {
+                  const executeHead = () => {
                     const match = this.getMatch(matchId)
                     // in case of a redirecting match during preload, the match does not exist
                     if (!match) {
                       return
                     }
+                    if (
+                      !route.options.head &&
+                      !route.options.scripts &&
+                      !route.options.headers
+                    ) {
+                      return
+                    }
                     const assetContext = {
                       matches,
                       match,
                       params: match.params,
                       loaderData: match.loaderData,
                     }
-                    const headFnContent =
-                      await route.options.head?.(assetContext)
-                    const meta = headFnContent?.meta
-                    const links = headFnContent?.links
-                    const headScripts = headFnContent?.scripts
-                    const styles = headFnContent?.styles
-
-                    const scripts = await route.options.scripts?.(assetContext)
-                    const headers = await route.options.headers?.(assetContext)
-                    return {
-                      meta,
-                      links,
-                      headScripts,
-                      headers,
-                      scripts,
-                      styles,
-                    }
+
+                    return Promise.all([
+                      route.options.head?.(assetContext),
+                      route.options.scripts?.(assetContext),
+                      route.options.headers?.(assetContext),
+                    ]).then(([headFnContent, scripts, headers]) => {
+                      const meta = headFnContent?.meta
+                      const links = headFnContent?.links
+                      const headScripts = headFnContent?.scripts
+                      const styles = headFnContent?.styles
+
+                      return {
+                        meta,
+                        links,
+                        headScripts,
+                        headers,
+                        scripts,
+                        styles,
+                      }
+                    })
                   }
 
                   const potentialPendingMinPromise = async () => {
@@ -2499,11 +2515,14 @@ export class RouterCore<
                   const prevMatch = this.getMatch(matchId)!
                   if (shouldSkipLoader(matchId)) {
                     if (this.isServer) {
-                      const head = await executeHead()
-                      updateMatch(matchId, (prev) => ({
-                        ...prev,
-                        ...head,
-                      }))
+                      const headResult = executeHead()
+                      if (headResult) {
+                        const head = await headResult
+                        updateMatch(matchId, (prev) => ({
+                          ...prev,
+                          ...head,
+                        }))
+                      }
                       return this.getMatch(matchId)!
                     }
                   }
@@ -2615,7 +2634,7 @@ export class RouterCore<
                             await route.options.loader?.(getLoaderContext())
 
                           handleRedirectAndNotFound(
-                            this.getMatch(matchId)!,
+                            this.getMatch(matchId),
                             loaderData,
                           )
                           updateMatch(matchId, (prev) => ({
@@ -2627,7 +2646,8 @@ export class RouterCore<
                           // so we need to wait for it to resolve before
                           // we can use the options
                           await route._lazyPromise
-                          const head = await executeHead()
+                          const headResult = executeHead()
+                          const head = headResult ? await headResult : undefined
                           await potentialPendingMinPromise()
 
                           // Last but not least, wait for the the components
@@ -2646,18 +2666,19 @@ export class RouterCore<
 
                           await potentialPendingMinPromise()
 
-                          handleRedirectAndNotFound(this.getMatch(matchId)!, e)
+                          handleRedirectAndNotFound(this.getMatch(matchId), e)
 
                           try {
                             route.options.onError?.(e)
                           } catch (onErrorError) {
                             error = onErrorError
                             handleRedirectAndNotFound(
-                              this.getMatch(matchId)!,
+                              this.getMatch(matchId),
                               onErrorError,
                             )
                           }
-                          const head = await executeHead()
+                          const headResult = executeHead()
+                          const head = headResult ? await headResult : undefined
                           updateMatch(matchId, (prev) => ({
                             ...prev,
                             error,
@@ -2667,16 +2688,20 @@ export class RouterCore<
                           }))
                         }
                       } catch (err) {
-                        const head = await executeHead()
-
-                        updateMatch(matchId, (prev) => {
-                          prev._nonReactive.loaderPromise = undefined
-                          return {
-                            ...prev,
-                            ...head,
+                        const match = this.getMatch(matchId)
+                        // in case of a redirecting match during preload, the match does not exist
+                        if (match) {
+                          const headResult = executeHead()
+                          if (headResult) {
+                            const head = await headResult
+                            updateMatch(matchId, (prev) => ({
+                              ...prev,
+                              ...head,
+                            }))
                           }
-                        })
-                        handleRedirectAndNotFound(this.getMatch(matchId)!, err)
+                          match._nonReactive.loaderPromise = undefined
+                        }
+                        handleRedirectAndNotFound(match, err)
                       }
                     }
 
@@ -2711,11 +2736,14 @@ export class RouterCore<
                       // if the loader did not run, still update head.
                       // reason: parent's beforeLoad may have changed the route context
                       // and only now do we know the route context (and that the loader would not run)
-                      const head = await executeHead()
-                      updateMatch(matchId, (prev) => ({
-                        ...prev,
-                        ...head,
-                      }))
+                      const headResult = executeHead()
+                      if (headResult) {
+                        const head = await headResult
+                        updateMatch(matchId, (prev) => ({
+                          ...prev,
+                          ...head,
+                        }))
+                      }
                     }
                   }
                   if (!loaderIsRunningAsync) {