diff --git a/e2e/react-router/issue-7120/index.html b/e2e/react-router/issue-7120/index.html new file mode 100644 index 0000000000..de92cad2a3 --- /dev/null +++ b/e2e/react-router/issue-7120/index.html @@ -0,0 +1,12 @@ + + + + + + Issue 7120 + + +
+ + + diff --git a/e2e/react-router/issue-7120/package.json b/e2e/react-router/issue-7120/package.json new file mode 100644 index 0000000000..c379ca791c --- /dev/null +++ b/e2e/react-router/issue-7120/package.json @@ -0,0 +1,26 @@ +{ + "name": "tanstack-router-e2e-react-issue-7120", + "private": true, + "type": "module", + "scripts": { + "dev": "vite --port 3000", + "dev:e2e": "vite", + "build": "vite build && tsc --noEmit", + "preview": "vite preview", + "start": "vite", + "test:e2e": "rm -rf port*.txt; playwright test --project=chromium" + }, + "dependencies": { + "@tanstack/react-router": "workspace:^", + "react": "^19.0.0", + "react-dom": "^19.0.0" + }, + "devDependencies": { + "@playwright/test": "^1.50.1", + "@tanstack/router-e2e-utils": "workspace:^", + "@types/react": "^19.0.8", + "@types/react-dom": "^19.0.3", + "@vitejs/plugin-react": "^6.0.1", + "vite": "^8.0.0" + } +} diff --git a/e2e/react-router/issue-7120/playwright.config.ts b/e2e/react-router/issue-7120/playwright.config.ts new file mode 100644 index 0000000000..6ba60eaff1 --- /dev/null +++ b/e2e/react-router/issue-7120/playwright.config.ts @@ -0,0 +1,27 @@ +import { defineConfig, devices } from '@playwright/test' +import { getTestServerPort } from '@tanstack/router-e2e-utils' +import packageJson from './package.json' with { type: 'json' } + +const PORT = await getTestServerPort(packageJson.name) +const baseURL = `http://localhost:${PORT}` + +export default defineConfig({ + testDir: './tests', + workers: 1, + reporter: [['line']], + use: { + baseURL, + }, + webServer: { + command: `VITE_NODE_ENV="test" VITE_SERVER_PORT=${PORT} pnpm build && pnpm preview --port ${PORT}`, + url: baseURL, + reuseExistingServer: !process.env.CI, + stdout: 'pipe', + }, + projects: [ + { + name: 'chromium', + use: { ...devices['Desktop Chrome'] }, + }, + ], +}) diff --git a/e2e/react-router/issue-7120/src/main.tsx b/e2e/react-router/issue-7120/src/main.tsx new file mode 100644 index 0000000000..473e8e1d75 --- /dev/null +++ b/e2e/react-router/issue-7120/src/main.tsx @@ -0,0 +1,67 @@ +import ReactDOM from 'react-dom/client' +import { + Outlet, + RouterProvider, + createRootRoute, + createRoute, + createRouter, + redirect, +} from '@tanstack/react-router' +import './styles.css' + +const posts = [ + { + id: '1', + title: + 'sunt aut facere repellat provident occaecati excepturi optio reprehenderit', + }, +] + +const rootRoute = createRootRoute({ + component: () => , + pendingMs: 0, + pendingComponent: () =>
loading
, + beforeLoad: async ({ matches }) => { + if (matches.find((match) => match.routeId === '/posts')) { + return + } + + await new Promise((resolve) => setTimeout(resolve, 1000)) + throw redirect({ to: '/posts' }) + }, +}) + +const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + component: () =>
Home
, +}) + +const postsRoute = createRoute({ + getParentRoute: () => rootRoute, + path: 'posts', + loader: async () => { + await new Promise((resolve) => setTimeout(resolve, 10)) + return posts + }, +}).lazy(() => import('./posts.lazy').then((d) => d.Route)) + +const routeTree = rootRoute.addChildren([indexRoute, postsRoute]) + +const router = createRouter({ + routeTree, + defaultViewTransition: true, +}) + +declare module '@tanstack/react-router' { + interface Register { + router: typeof router + } +} + +const rootElement = document.getElementById('app')! + +if (!rootElement.innerHTML) { + const root = ReactDOM.createRoot(rootElement) + root.render() +} diff --git a/e2e/react-router/issue-7120/src/posts.lazy.tsx b/e2e/react-router/issue-7120/src/posts.lazy.tsx new file mode 100644 index 0000000000..c902ed7cba --- /dev/null +++ b/e2e/react-router/issue-7120/src/posts.lazy.tsx @@ -0,0 +1,17 @@ +import { createLazyRoute } from '@tanstack/react-router' + +export const Route = createLazyRoute('/posts')({ + component: PostsComponent, +}) + +function PostsComponent() { + const posts = Route.useLoaderData() + + return ( + + ) +} diff --git a/e2e/react-router/issue-7120/src/styles.css b/e2e/react-router/issue-7120/src/styles.css new file mode 100644 index 0000000000..c3298a2e5e --- /dev/null +++ b/e2e/react-router/issue-7120/src/styles.css @@ -0,0 +1,7 @@ +html { + color-scheme: light dark; +} + +body { + font-family: system-ui, sans-serif; +} diff --git a/e2e/react-router/issue-7120/tests/issue-7120.repro.spec.ts b/e2e/react-router/issue-7120/tests/issue-7120.repro.spec.ts new file mode 100644 index 0000000000..b53f0fe68f --- /dev/null +++ b/e2e/react-router/issue-7120/tests/issue-7120.repro.spec.ts @@ -0,0 +1,18 @@ +import { expect, test } from '@playwright/test' + +test('root beforeLoad redirect does not blank when pending UI and view transitions are enabled', async ({ + page, +}) => { + const pageErrors: Array = [] + + page.on('pageerror', (error) => { + pageErrors.push(error.message) + }) + + await page.goto('/') + + await expect(page).toHaveURL(/\/posts$/) + await expect(page.getByText('sunt aut facere repe')).toBeVisible() + await expect(page.getByTestId('root-pending')).not.toBeVisible() + expect(pageErrors).toEqual([]) +}) diff --git a/e2e/react-router/issue-7120/tsconfig.json b/e2e/react-router/issue-7120/tsconfig.json new file mode 100644 index 0000000000..4f6089bc08 --- /dev/null +++ b/e2e/react-router/issue-7120/tsconfig.json @@ -0,0 +1,15 @@ +{ + "compilerOptions": { + "strict": true, + "esModuleInterop": true, + "jsx": "react-jsx", + "target": "ESNext", + "moduleResolution": "Bundler", + "module": "ESNext", + "resolveJsonModule": true, + "allowJs": true, + "skipLibCheck": true, + "types": ["vite/client"] + }, + "exclude": ["node_modules", "dist"] +} diff --git a/e2e/react-router/issue-7120/vite.config.js b/e2e/react-router/issue-7120/vite.config.js new file mode 100644 index 0000000000..9ffcc67574 --- /dev/null +++ b/e2e/react-router/issue-7120/vite.config.js @@ -0,0 +1,6 @@ +import { defineConfig } from 'vite' +import react from '@vitejs/plugin-react' + +export default defineConfig({ + plugins: [react()], +}) diff --git a/packages/react-router/src/Match.tsx b/packages/react-router/src/Match.tsx index fefd256bf6..a5b1def939 100644 --- a/packages/react-router/src/Match.tsx +++ b/packages/react-router/src/Match.tsx @@ -22,6 +22,8 @@ import { ClientOnly } from './ClientOnly' import { useLayoutEffect } from './utils' import type { AnyRoute, RootRouteOptions } from '@tanstack/router-core' +const resolvedPromise = Promise.resolve() + export const Match = React.memo(function MatchImpl({ matchId, }: { @@ -277,6 +279,11 @@ export const MatchInner = React.memo(function MatchInnerImpl({ ) } + const getRedirectPromise = (match: Parameters[0]) => + getMatchPromise(match, 'loadPromise') ?? + router.latestLoadPromise ?? + resolvedPromise + if (isServer ?? router.isServer) { const match = router.stores.matchStores.get(matchId)?.get() if (!match) { @@ -313,7 +320,7 @@ export const MatchInner = React.memo(function MatchInnerImpl({ } if (match.status === 'pending') { - throw getMatchPromise(match, 'loadPromise') + throw getRedirectPromise(match) } if (match.status === 'notFound') { @@ -431,7 +438,7 @@ export const MatchInner = React.memo(function MatchInnerImpl({ } } } - throw getMatchPromise(match, 'loadPromise') + throw getRedirectPromise(match) } if (match.status === 'notFound') { 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 0c4c1b4146..2dd12cc2b9 100644 --- a/packages/react-router/tests/store-updates-during-navigation.test.tsx +++ b/packages/react-router/tests/store-updates-during-navigation.test.tsx @@ -136,7 +136,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(8) + expect(updates).toBe(7) }) test('redirection in preload', async () => { @@ -170,7 +170,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(5) + expect(updates).toBe(4) }) test('nothing', async () => { diff --git a/packages/router-core/src/load-matches.ts b/packages/router-core/src/load-matches.ts index da2d0244a6..eb3f669404 100644 --- a/packages/router-core/src/load-matches.ts +++ b/packages/router-core/src/load-matches.ts @@ -34,10 +34,21 @@ type InnerLoadContext = { preload?: boolean forceStaleReload?: boolean onReady?: () => Promise + isLatest?: () => boolean sync?: boolean + cancelled?: boolean } +const isStaleLoad = (inner: InnerLoadContext): boolean => + inner.isLatest?.() === false || + (!inner.preload && inner.router.latestLocation.href !== inner.location.href) + const triggerOnReady = (inner: InnerLoadContext): void | Promise => { + if (isStaleLoad(inner)) { + inner.cancelled = true + return + } + if (!inner.rendered) { inner.rendered = true return inner.onReady?.() @@ -117,52 +128,59 @@ const handleRedirectAndNotFound = ( match: AnyRouteMatch | undefined, err: unknown, ): void => { - if (!isRedirect(err) && !isNotFound(err)) return + const redirect = isRedirect(err) ? err : undefined + const notFound = isNotFound(err) ? err : undefined - if (isRedirect(err) && err.redirectHandled && !err.options.reloadDocument) { - throw err + if (!redirect && !notFound) return + + if (redirect?.redirectHandled && !redirect.options.reloadDocument) { + throw redirect } // in case of a redirecting match during preload, the match does not exist if (match) { + // If a rendered client match redirects, the stale render may observe the + // redirected status before the redirect navigation replaces it. + const keepLoadPromise = + redirect && + inner.rendered && + !(isServer ?? inner.router.isServer) && + !inner.preload + match._nonReactive.beforeLoadPromise?.resolve() match._nonReactive.loaderPromise?.resolve() match._nonReactive.beforeLoadPromise = undefined match._nonReactive.loaderPromise = undefined - match._nonReactive.error = err + if (!keepLoadPromise) { + match._nonReactive.error = err - inner.updateMatch(match.id, (prev) => ({ - ...prev, - status: isRedirect(err) - ? 'redirected' - : isNotFound(err) - ? 'notFound' - : prev.status === 'pending' - ? 'success' - : prev.status, - context: buildMatchContext(inner, match.index), - isFetching: false, - error: err, - })) + inner.updateMatch(match.id, (prev) => ({ + ...prev, + status: redirect ? 'redirected' : 'notFound', + context: buildMatchContext(inner, match.index), + isFetching: false, + error: err, + })) + + match._nonReactive.loadPromise?.resolve() + } - if (isNotFound(err) && !err.routeId) { + if (notFound && !notFound.routeId) { // Stamp the throwing match's routeId so that the finalization step in // loadMatches knows where the notFound originated. The actual boundary // resolution (walking up to the nearest notFoundComponent) is deferred to // the finalization step, where firstBadMatchIndex is stable and // headMaxIndex can be capped correctly. - err.routeId = match.routeId + notFound.routeId = match.routeId } - - match._nonReactive.loadPromise?.resolve() } - if (isRedirect(err)) { + if (redirect) { inner.rendered = true - err.options._fromLocation = inner.location - err.redirectHandled = true - err = inner.router.resolveRedirect(err) + redirect.options._fromLocation = inner.location + redirect.redirectHandled = true + err = inner.router.resolveRedirect(redirect) } throw err @@ -967,6 +985,7 @@ export async function loadMatches(arg: { preload?: boolean forceStaleReload?: boolean onReady?: () => Promise + isLatest?: () => boolean updateMatch: UpdateMatchFn sync?: boolean }): Promise> { @@ -1001,6 +1020,10 @@ export async function loadMatches(arg: { break } + if (inner.cancelled) { + return inner.matches + } + if (inner.serialError || inner.firstBadMatchIndex != null) { break } diff --git a/packages/router-core/src/router.ts b/packages/router-core/src/router.ts index 3fbcd91387..54310102a5 100644 --- a/packages/router-core/src/router.ts +++ b/packages/router-core/src/router.ts @@ -2406,43 +2406,55 @@ export class RouterCore< load: LoadFn = async (opts?: { sync?: boolean }): Promise => { let redirect: AnyRedirect | undefined let notFound: NotFoundError | undefined - let loadPromise: Promise const previousLocation = this.stores.resolvedLocation.get() ?? this.stores.location.get() - // eslint-disable-next-line prefer-const - loadPromise = new Promise((resolve) => { - this.startTransition(async () => { - try { - this.beforeLoad() - const next = this.latestLocation - const prevLocation = this.stores.resolvedLocation.get() - const locationChangeInfo = getLocationChangeInfo(next, prevLocation) - - if (!this.stores.redirect.get()) { - this.emit({ - type: 'onBeforeNavigate', - ...locationChangeInfo, - }) - } + const loadPromise = createControlledPromise() + const commitLocationPromise = this.commitLocationPromise + this.latestLoadPromise = loadPromise + this.startTransition(async () => { + try { + this.beforeLoad() + const next = this.latestLocation + const prevLocation = this.stores.resolvedLocation.get() + const locationChangeInfo = getLocationChangeInfo(next, prevLocation) + + if (!this.stores.redirect.get()) { this.emit({ - type: 'onBeforeLoad', + type: 'onBeforeNavigate', ...locationChangeInfo, }) + } + + this.emit({ + type: 'onBeforeLoad', + ...locationChangeInfo, + }) + + await loadMatches({ + router: this, + sync: opts?.sync, + forceStaleReload: previousLocation.href === next.href, + matches: this.stores.pendingMatches.get(), + location: next, + updateMatch: this.updateMatch, + isLatest: () => this.latestLoadPromise === loadPromise, + onReady: async () => { + if (this.latestLoadPromise !== loadPromise) { + return + } + + const commitPromise = createControlledPromise() + + // Wrap batch in framework-specific transition wrapper (e.g., Solid's startTransition) + this.startTransition(() => { + this.startViewTransition(async () => { + try { + if (this.latestLoadPromise !== loadPromise) { + return + } - await loadMatches({ - router: this, - sync: opts?.sync, - forceStaleReload: previousLocation.href === next.href, - matches: this.stores.pendingMatches.get(), - location: next, - updateMatch: this.updateMatch, - // eslint-disable-next-line @typescript-eslint/require-await - onReady: async () => { - // Wrap batch in framework-specific transition wrapper (e.g., Solid's startTransition) - this.startTransition(() => { - this.startViewTransition(async () => { // this.viewTransitionPromise = createControlledPromise() // Commit the pending matches. If a previous match was @@ -2511,10 +2523,7 @@ export class RouterCore< this.stores.setCached([ ...this.stores.cachedMatches.get(), ...exitingMatches!.filter( - (d) => - d.status !== 'error' && - d.status !== 'notFound' && - d.status !== 'redirected', + (d) => d.status === 'success', ), ]) this.clearExpiredCache() @@ -2534,50 +2543,56 @@ export class RouterCore< ) } } - }) + } finally { + commitPromise.resolve() + } }) - }, - }) - } catch (err) { - if (isRedirect(err)) { - redirect = err - if (!(isServer ?? this.isServer)) { - this.navigate({ - ...redirect.options, - replace: true, - ignoreBlocker: true, - }) - } - } else if (isNotFound(err)) { - notFound = err + }) + + await commitPromise + }, + }) + } catch (err) { + if (isRedirect(err)) { + redirect = err + if (!(isServer ?? this.isServer)) { + this.navigate({ + ...redirect.options, + replace: true, + ignoreBlocker: true, + }) } + } else if (isNotFound(err)) { + notFound = err + } - const nextStatusCode = redirect - ? redirect.status - : notFound - ? 404 - : this.stores.matches.get().some((d) => d.status === 'error') - ? 500 - : 200 + const nextStatusCode = redirect + ? redirect.status + : notFound + ? 404 + : this.stores.matches.get().some((d) => d.status === 'error') + ? 500 + : 200 + if (this.latestLoadPromise === loadPromise) { this.batch(() => { this.stores.statusCode.set(nextStatusCode) this.stores.redirect.set(redirect) }) } + } - if (this.latestLoadPromise === loadPromise) { - this.commitLocationPromise?.resolve() - this.latestLoadPromise = undefined + if (this.latestLoadPromise === loadPromise) { + commitLocationPromise?.resolve() + this.latestLoadPromise = undefined + if (this.commitLocationPromise === commitLocationPromise) { this.commitLocationPromise = undefined } + } - resolve() - }) + loadPromise.resolve() }) - this.latestLoadPromise = loadPromise - await loadPromise while ( diff --git a/packages/router-core/tests/callbacks.test.ts b/packages/router-core/tests/callbacks.test.ts index 1673626f7e..20e331807b 100644 --- a/packages/router-core/tests/callbacks.test.ts +++ b/packages/router-core/tests/callbacks.test.ts @@ -88,6 +88,65 @@ describe('callbacks', () => { expect.objectContaining({ id: '/bar/bar' }), ) }) + + it('does not resolve a newer commit promise from an older load', async () => { + const history = createMemoryHistory() + let unblock!: () => void + const blockerPromise = new Promise((resolve) => { + unblock = resolve + }) + let blockerCalled = false + let nestedNavigatePromise: Promise | undefined + let nestedNavigateSettled = false + + const rootRoute = new BaseRootRoute({}) + + let router!: ReturnType + const fooRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/foo', + onEnter: () => { + history.block({ + blockerFn: () => { + blockerCalled = true + return blockerPromise.then(() => false) + }, + }) + nestedNavigatePromise = router.navigate({ to: '/bar' }) + nestedNavigatePromise.then(() => { + nestedNavigateSettled = true + }) + }, + }) + + const barRoute = new BaseRoute({ + getParentRoute: () => rootRoute, + path: '/bar', + }) + + router = createTestRouter({ + routeTree: rootRoute.addChildren([fooRoute, barRoute]), + history, + }) + + const unsubscribe = history.subscribe(() => router.load()) + try { + await router.navigate({ to: '/foo' }) + expect(blockerCalled).toBe(true) + expect(nestedNavigatePromise).toBeDefined() + + await Promise.resolve() + expect(nestedNavigateSettled).toBe(false) + + unblock() + await nestedNavigatePromise + + expect(nestedNavigateSettled).toBe(true) + expect(router.state.location.pathname).toBe('/bar') + } finally { + unsubscribe() + } + }) }) describe('onLeave', () => { diff --git a/packages/solid-router/tests/store-updates-during-navigation.test.tsx b/packages/solid-router/tests/store-updates-during-navigation.test.tsx index 69b570122d..b341dcad29 100644 --- a/packages/solid-router/tests/store-updates-during-navigation.test.tsx +++ b/packages/solid-router/tests/store-updates-during-navigation.test.tsx @@ -136,7 +136,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(9) + expect(updates).toBe(8) }) test('redirection in preload', async () => { @@ -172,7 +172,7 @@ describe("Store doesn't update *too many* times during navigation", () => { // that needs to be done during a navigation. // Any change that increases this number should be investigated. // Note: Solid has different update counts than React due to different reactivity - expect(updates).toBe(5) + expect(updates).toBe(4) }) test('nothing', async () => { diff --git a/packages/vue-router/tests/store-updates-during-navigation.test.tsx b/packages/vue-router/tests/store-updates-during-navigation.test.tsx index 1c40bf7be7..0cade46f9e 100644 --- a/packages/vue-router/tests/store-updates-during-navigation.test.tsx +++ b/packages/vue-router/tests/store-updates-during-navigation.test.tsx @@ -202,7 +202,7 @@ describe("Store doesn't update *too many* times during navigation", () => { // that needs to be done during a navigation. // Any change that increases this number should be investigated. // Note: Vue has different update counts than React/Solid due to different reactivity - expect(updates).toBe(9) + expect(updates).toBe(8) }) test('hover preload, then navigate, w/ async loaders', async () => { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0823c60b4a..237f88fd2d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1020,6 +1020,37 @@ importers: specifier: ^8.0.0 version: 8.0.0(@emnapi/core@1.10.0)(@emnapi/runtime@1.10.0)(@types/node@25.0.9)(esbuild@0.27.4)(jiti@2.7.0)(sass@1.97.2)(terser@5.37.0)(tsx@4.20.3)(yaml@2.8.1) + e2e/react-router/issue-7120: + dependencies: + '@tanstack/react-router': + specifier: workspace:* + version: link:../../../packages/react-router + react: + specifier: ^19.2.3 + version: 19.2.3 + react-dom: + specifier: ^19.2.3 + version: 19.2.3(react@19.2.3) + devDependencies: + '@playwright/test': + specifier: ^1.57.0 + version: 1.58.0 + '@tanstack/router-e2e-utils': + specifier: workspace:^ + version: link:../../e2e-utils + '@types/react': + specifier: ^19.2.8 + version: 19.2.9 + '@types/react-dom': + specifier: ^19.2.3 + version: 19.2.3(@types/react@19.2.9) + '@vitejs/plugin-react': + specifier: ^6.0.1 + version: 6.0.1(vite@8.0.0(@emnapi/core@1.10.0)(@emnapi/runtime@1.10.0)(@types/node@25.0.9)(esbuild@0.27.4)(jiti@2.7.0)(sass@1.97.2)(terser@5.37.0)(tsx@4.20.3)(yaml@2.8.1)) + vite: + specifier: ^8.0.0 + version: 8.0.0(@emnapi/core@1.10.0)(@emnapi/runtime@1.10.0)(@types/node@25.0.9)(esbuild@0.27.4)(jiti@2.7.0)(sass@1.97.2)(terser@5.37.0)(tsx@4.20.3)(yaml@2.8.1) + e2e/react-router/js-only-file-based: dependencies: '@tailwindcss/vite':