From d70a91480570ab7429e4eef08f151f65fb6a9cdd Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Mon, 30 Jun 2025 19:27:34 +0200 Subject: [PATCH 1/2] fix: correctly handle pending vs. cached matches this reverts #4513 and implements a proper fix for #4245 fixes #4546 --- packages/react-router/tests/loaders.test.tsx | 242 ++++++++++++++++++- packages/router-core/src/router.ts | 44 ++-- 2 files changed, 265 insertions(+), 21 deletions(-) diff --git a/packages/react-router/tests/loaders.test.tsx b/packages/react-router/tests/loaders.test.tsx index 174ae4eeb22..8d92506468b 100644 --- a/packages/react-router/tests/loaders.test.tsx +++ b/packages/react-router/tests/loaders.test.tsx @@ -1,4 +1,5 @@ import { + act, cleanup, configure, fireEvent, @@ -17,6 +18,7 @@ import { createRootRoute, createRoute, createRouter, + useRouter, } from '../src' import { sleep } from './utils' @@ -380,7 +382,7 @@ test('reproducer #4245', async () => { render() // We wait for the initial loader to complete - await router.load() + await act(() => router.load()) const fooLink = await screen.findByTestId('link-to-foo') expect(fooLink).toBeInTheDocument() @@ -389,27 +391,257 @@ test('reproducer #4245', async () => { fireEvent.click(fooLink) // We immediately see the content of the foo route - const indexLink = await screen.findByTestId('link-to-index') + const indexLink = await screen.findByTestId('link-to-index', undefined, { + timeout: WAIT_TIME, + }) expect(indexLink).toBeInTheDocument() // We navigate to the index route fireEvent.click(indexLink) // We immediately see the content of the index route because the stale data is still available - const fooLink2 = await screen.findByTestId('link-to-foo') + const fooLink2 = await screen.findByTestId('link-to-foo', undefined, { + timeout: WAIT_TIME, + }) expect(fooLink2).toBeInTheDocument() // We navigate to the foo route again fireEvent.click(fooLink2) // We immediately see the content of the foo route - const indexLink2 = await screen.findByTestId('link-to-index') + const indexLink2 = await screen.findByTestId('link-to-index', undefined, { + timeout: WAIT_TIME, + }) expect(indexLink2).toBeInTheDocument() // We navigate to the index route again fireEvent.click(indexLink2) // We now should see the content of the index route immediately because the stale data is still available - const fooLink3 = await screen.findByTestId('link-to-foo') + const fooLink3 = await screen.findByTestId('link-to-foo', undefined, { + timeout: WAIT_TIME, + }) expect(fooLink3).toBeInTheDocument() }) + +test('reproducer #4546', async () => { + const rootRoute = createRootRoute({ + component: () => { + return ( + <> +
+ + Home + {' '} + + /1 + +
+
+ + + ) + }, + }) + + let counter = 0 + const appRoute = createRoute({ + getParentRoute: () => rootRoute, + id: '_app', + beforeLoad: () => { + counter += 1 + return { + counter, + } + }, + component: () => { + return ( +
+
+ +
+ ) + }, + }) + + function Header() { + const router = useRouter() + const { counter } = appRoute.useRouteContext() + + return ( +
+ Header Counter:

{counter}

+ +
+ ) + } + + const indexRoute = createRoute({ + getParentRoute: () => appRoute, + path: '/', + loader: ({ context }) => { + return { + counter: context.counter, + } + }, + + component: () => { + const data = indexRoute.useLoaderData() + const ctx = indexRoute.useRouteContext() + + return ( +
+
Index route
+
+ route context:{' '} +

{ctx.counter}

+
+
+ loader data:

{data.counter}

+
+
+ ) + }, + }) + const idRoute = createRoute({ + getParentRoute: () => appRoute, + path: '$id', + loader: ({ context }) => { + return { + counter: context.counter, + } + }, + + component: () => { + const data = idRoute.useLoaderData() + const ctx = idRoute.useRouteContext() + + return ( +
+
$id route
+
+ route context:

{ctx.counter}

+
+
+ loader data:

{data.counter}

+
+
+ ) + }, + }) + + const routeTree = rootRoute.addChildren([ + appRoute.addChildren([indexRoute, idRoute]), + ]) + const router = createRouter({ routeTree }) + + render() + + const indexLink = await screen.findByTestId('link-to-index') + expect(indexLink).toBeInTheDocument() + + const idLink = await screen.findByTestId('link-to-id') + expect(idLink).toBeInTheDocument() + + const invalidateRouterButton = await screen.findByTestId('invalidate-router') + expect(invalidateRouterButton).toBeInTheDocument() + + { + const headerCounter = await screen.findByTestId('header-counter') + expect(headerCounter).toHaveTextContent('1') + + const routeContext = await screen.findByTestId('index-route-context') + expect(routeContext).toHaveTextContent('1') + + const loaderData = await screen.findByTestId('index-loader-data') + expect(loaderData).toHaveTextContent('1') + } + + fireEvent.click(idLink) + + { + const headerCounter = await screen.findByTestId('header-counter') + expect(headerCounter).toHaveTextContent('2') + + const routeContext = await screen.findByTestId('id-route-context') + expect(routeContext).toHaveTextContent('2') + + const loaderData = await screen.findByTestId('id-loader-data') + expect(loaderData).toHaveTextContent('2') + } + + fireEvent.click(indexLink) + + { + const headerCounter = await screen.findByTestId('header-counter') + expect(headerCounter).toHaveTextContent('3') + + const routeContext = await screen.findByTestId('index-route-context') + expect(routeContext).toHaveTextContent('3') + + const loaderData = await screen.findByTestId('index-loader-data') + expect(loaderData).toHaveTextContent('3') + } + + fireEvent.click(invalidateRouterButton) + + { + const headerCounter = await screen.findByTestId('header-counter') + expect(headerCounter).toHaveTextContent('4') + + const routeContext = await screen.findByTestId('index-route-context') + expect(routeContext).toHaveTextContent('4') + + const loaderData = await screen.findByTestId('index-loader-data') + expect(loaderData).toHaveTextContent('4') + } + + fireEvent.click(idLink) + + { + const headerCounter = await screen.findByTestId('header-counter') + expect(headerCounter).toHaveTextContent('5') + + const routeContext = await screen.findByTestId('id-route-context') + expect(routeContext).toHaveTextContent('5') + + const loaderData = await screen.findByTestId('id-loader-data') + expect(loaderData).toHaveTextContent('5') + } +}) diff --git a/packages/router-core/src/router.ts b/packages/router-core/src/router.ts index e7fd84f7920..bd58e742500 100644 --- a/packages/router-core/src/router.ts +++ b/packages/router-core/src/router.ts @@ -1791,16 +1791,9 @@ export class RouterCore< location: this.latestLocation, pendingMatches, // If a cached moved to pendingMatches, remove it from cachedMatches - cachedMatches: s.cachedMatches.filter((cachedMatch) => { - const pendingMatch = pendingMatches.find((e) => e.id === cachedMatch.id) - - if (!pendingMatch) return true - - return ( - cachedMatch.status === 'success' && - (cachedMatch.isFetching || cachedMatch.loaderData !== undefined) - ) - }), + cachedMatches: s.cachedMatches.filter( + (d) => !pendingMatches.find((e) => e.id === d.id), + ), })) } @@ -2208,7 +2201,15 @@ export class RouterCore< // Wait for the beforeLoad to resolve before we continue await existingMatch.beforeLoadPromise - executeBeforeLoad = this.getMatch(matchId)!.status === 'error' + const match = this.getMatch(matchId)! + if (match.status === 'error') { + executeBeforeLoad = true + } else if ( + match.preload && + (match.status === 'redirected' || match.status === 'notFound') + ) { + handleRedirectAndNotFound(match, match.error) + } } if (executeBeforeLoad) { // If we are not in the middle of a load OR the previous load failed, start it @@ -2337,14 +2338,25 @@ export class RouterCore< validResolvedMatches.forEach(({ id: matchId, routeId }, index) => { matchPromises.push( (async () => { - const { loaderPromise: prevLoaderPromise } = - this.getMatch(matchId)! - let loaderShouldRunAsync = false let loaderIsRunningAsync = false - if (prevLoaderPromise) { - await prevLoaderPromise + const prevMatch = this.getMatch(matchId)! + // there is a loaderPromise, so we are in the middle of a load + if (prevMatch.loaderPromise) { + // do not block if we already have stale data we can show + if (prevMatch.status === 'success' && !sync) { + // if the ongoing load is a preload, make sure to execute redirects + if (prevMatch.preload) { + prevMatch.loaderPromise.catch((err) => { + if (isRedirect(err)) { + return this.navigate(err.options) + } + }) + } + return this.getMatch(matchId)! + } + await prevMatch.loaderPromise const match = this.getMatch(matchId)! if (match.error) { handleRedirectAndNotFound(match, match.error) From 45896eb71386711ad2f8d502ca7410c6f192187c Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Mon, 30 Jun 2025 21:52:02 +0200 Subject: [PATCH 2/2] block upon preload --- packages/router-core/src/router.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/router-core/src/router.ts b/packages/router-core/src/router.ts index bd58e742500..71d75548d3b 100644 --- a/packages/router-core/src/router.ts +++ b/packages/router-core/src/router.ts @@ -2345,15 +2345,13 @@ export class RouterCore< // there is a loaderPromise, so we are in the middle of a load if (prevMatch.loaderPromise) { // do not block if we already have stale data we can show - if (prevMatch.status === 'success' && !sync) { - // if the ongoing load is a preload, make sure to execute redirects - if (prevMatch.preload) { - prevMatch.loaderPromise.catch((err) => { - if (isRedirect(err)) { - return this.navigate(err.options) - } - }) - } + // but only if the ongoing load is not a preload since error handling is different for preloads + // and we don't want to swallow errors + if ( + prevMatch.status === 'success' && + !sync && + !prevMatch.preload + ) { return this.getMatch(matchId)! } await prevMatch.loaderPromise