diff --git a/packages/react-router/tests/store-updates-during-navigation.test.tsx b/packages/react-router/tests/store-updates-during-navigation.test.tsx index 19507e182e..9a81d91f13 100644 --- a/packages/react-router/tests/store-updates-during-navigation.test.tsx +++ b/packages/react-router/tests/store-updates-during-navigation.test.tsx @@ -1,5 +1,11 @@ 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, @@ -7,6 +13,7 @@ import { createRootRoute, createRoute, createRouter, + notFound, redirect, useRouterState, } from '../src' @@ -74,6 +81,8 @@ function setup({ defaultPendingMs, defaultPendingMinMs, defaultPendingComponent: () =>

Loading...

, + defaultNotFoundComponent: () =>

Not Found Title

, + defaultPreload: 'intent', }) render() @@ -81,14 +90,14 @@ function setup({ return { select, router } } -async function run({ select }: ReturnType, opts?: {}) { +async function run({ select }: ReturnType) { // 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((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) + }) }) diff --git a/packages/router-core/src/router.ts b/packages/router-core/src/router.ts index 1200bcc892..0008552246 100644 --- a/packages/router-core/src/router.ts +++ b/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 => { - 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() + // explicitly capture the previous loadPromise + const prevLoadPromise = match._nonReactive.loadPromise + match._nonReactive.loadPromise = createControlledPromise(() => { + prevLoadPromise?.resolve() + }) - try { - const match = this.getMatch(matchId)! - match._nonReactive.beforeLoadPromise = createControlledPromise() - // explicitly capture the previous loadPromise - const prevLoadPromise = match._nonReactive.loadPromise - match._nonReactive.loadPromise = createControlledPromise(() => { - 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,