From 006ab083800013e2d4795d4c9306db87f05393c9 Mon Sep 17 00:00:00 2001 From: Tanner Linsley Date: Sat, 6 Jul 2024 12:25:48 -0600 Subject: [PATCH 01/34] fix(router): context issues --- .../framework/react/guide/server-functions.md | 6 +- .../src/routes/about.tsx | 8 + .../start-trellaux/app/components/Column.tsx | 4 +- packages/react-router/src/Matches.tsx | 4 +- packages/react-router/src/RouterProvider.tsx | 11 - packages/react-router/src/index.tsx | 2 +- packages/react-router/src/router.ts | 626 ++++++++++-------- .../react-router/tests/redirects.test.tsx | 4 +- packages/react-router/tests/route.test-d.tsx | 5 +- packages/router-plugin/src/code-splitter.ts | 2 +- .../router-plugin/src/router-generator.ts | 2 +- packages/start/src/client/renderRSC.tsx | 2 +- 12 files changed, 371 insertions(+), 305 deletions(-) diff --git a/docs/framework/react/guide/server-functions.md b/docs/framework/react/guide/server-functions.md index c856680cf0f..5c8316f4c31 100644 --- a/docs/framework/react/guide/server-functions.md +++ b/docs/framework/react/guide/server-functions.md @@ -37,7 +37,7 @@ function Component() { if (!el) return el.addEventListener('click', async () => { const result = await yourFn() - console.log(result) + console.info(result) }) } @@ -55,7 +55,7 @@ Or from another server function: ```typescript const yourFn2 = createServerFn('POST', async () => { const result = await yourFn() - console.log(result) + console.info(result) }) ``` @@ -139,7 +139,7 @@ const yourFn = createServerFn('POST', async () => { // Server-side code lives here }) -console.log(yourFn.url) +console.info(yourFn.url) ``` And pass this to the `action` attribute of the form: diff --git a/examples/react/quickstart-file-based/src/routes/about.tsx b/examples/react/quickstart-file-based/src/routes/about.tsx index 492e6b85c25..0eda76df7cb 100644 --- a/examples/react/quickstart-file-based/src/routes/about.tsx +++ b/examples/react/quickstart-file-based/src/routes/about.tsx @@ -3,6 +3,14 @@ import { createFileRoute } from '@tanstack/react-router' export const Route = createFileRoute('/about')({ component: AboutComponent, + beforeLoad: async () => { + await new Promise((resolve) => setTimeout(resolve, 2000)) + return { someData: 'hello' } + }, + loader: async ({ context }) => { + await new Promise((resolve) => setTimeout(resolve, 1000)) + console.debug(context.someData) + }, }) function AboutComponent() { diff --git a/examples/react/start-trellaux/app/components/Column.tsx b/examples/react/start-trellaux/app/components/Column.tsx index 3c363380608..66fd02309f7 100644 --- a/examples/react/start-trellaux/app/components/Column.tsx +++ b/examples/react/start-trellaux/app/components/Column.tsx @@ -3,7 +3,7 @@ import invariant from 'tiny-invariant' import { twMerge } from 'tailwind-merge' import { flushSync } from 'react-dom' -import { CONTENT_TYPES, INTENTS, type RenderedItem } from '../types' +import { CONTENT_TYPES, type RenderedItem } from '../types' import { Icon } from '../icons/icons' import { useDeleteColumnMutation, @@ -119,8 +119,6 @@ export const Column = forwardRef( acceptColumnDrop === 'left' ? previousOrder : nextOrder const moveOrder = (droppedOrder + order) / 2 - console.log('moveOrder', moveOrder) - updateColumnMutation.mutate({ boardId, id: transfer.id, diff --git a/packages/react-router/src/Matches.tsx b/packages/react-router/src/Matches.tsx index d842b7507ea..70218592545 100644 --- a/packages/react-router/src/Matches.tsx +++ b/packages/react-router/src/Matches.tsx @@ -47,8 +47,10 @@ export interface RouteMatch< paramsError: unknown searchError: unknown updatedAt: number + beforeLoadPromise?: ControlledPromise> + loaderPromise?: ControlledPromise + componentsPromise?: Promise> loadPromise: ControlledPromise - loaderPromise: Promise loaderData?: TLoaderData routeContext: TRouteContext context: TAllContext diff --git a/packages/react-router/src/RouterProvider.tsx b/packages/react-router/src/RouterProvider.tsx index 3cfec5ee494..6e5f038753d 100644 --- a/packages/react-router/src/RouterProvider.tsx +++ b/packages/react-router/src/RouterProvider.tsx @@ -104,17 +104,6 @@ export function RouterProvider< ) } -export function getRouteMatch( - state: RouterState, - id: string, -): undefined | MakeRouteMatch { - return [ - ...state.cachedMatches, - ...(state.pendingMatches ?? []), - ...state.matches, - ].find((d) => d.id === id) -} - export type RouterProps< TRouter extends AnyRouter = RegisteredRouter, TDehydrated extends Record = Record, diff --git a/packages/react-router/src/index.tsx b/packages/react-router/src/index.tsx index d6cf02af991..ca5c951eb4d 100644 --- a/packages/react-router/src/index.tsx +++ b/packages/react-router/src/index.tsx @@ -183,6 +183,7 @@ export { PathParamError, getInitialRouterState, defaultSerializeError, + getRouteMatch, type Register, type AnyRouter, type RegisteredRouter, @@ -207,7 +208,6 @@ export { export { RouterProvider, RouterContextProvider, - getRouteMatch, type RouterProps, type CommitLocationOptions, type MatchLocation, diff --git a/packages/react-router/src/router.ts b/packages/react-router/src/router.ts index 203c7d92632..79cc7c04234 100644 --- a/packages/react-router/src/router.ts +++ b/packages/react-router/src/router.ts @@ -12,7 +12,6 @@ import { pick, replaceEqualDeep, } from './utils' -import { getRouteMatch } from './RouterProvider' import { cleanPath, interpolatePath, @@ -1082,7 +1081,6 @@ export class Router< isFetching: false, error: undefined, paramsError: parseErrors[index], - loaderPromise: Promise.resolve(), loadPromise, routeContext: undefined!, context: undefined!, @@ -1545,6 +1543,7 @@ export class Router< location: next, checkLatest: () => this.checkLatest(promise), onReady: async () => { + // eslint-disable-next-line ts/require-await await this.startViewTransition(async () => { // this.viewTransitionPromise = createControlledPromise() @@ -1628,7 +1627,7 @@ export class Router< return this.latestLoadPromise } - startViewTransition = async (fn: () => Promise) => { + startViewTransition = (fn: () => Promise) => { // Determine if we should start a view transition from the navigation // or from the router default const shouldViewTransition = @@ -1737,11 +1736,25 @@ export class Router< try { // Check each match middleware to see if the route can be accessed // eslint-disable-next-line prefer-const - for (let [index, match] of matches.entries()) { - const parentMatch = matches[index - 1] - const route = this.looseRoutesById[match.routeId]! + for (let [index, { id: matchId, routeId }] of matches.entries()) { + const getMatch = () => getRouteMatch(this.state, matchId)! + const route = this.looseRoutesById[routeId]! const abortController = new AbortController() - let loadPromise = match.loadPromise + let loadPromise = getMatch().loadPromise + + const parentMatchId = matches[index - 1]?.id + + const getParentContext = () => { + if (!parentMatchId) { + return (this.options.context as any) ?? {} + } + + return ( + getRouteMatch(this.state, parentMatchId)!.context ?? + this.options.context ?? + {} + ) + } const pendingMs = route.options.pendingMs ?? this.options.defaultPendingMs @@ -1757,37 +1770,6 @@ export class Router< this.options.defaultPendingComponent) ) - if (shouldPending) { - // If we might show a pending component, we need to wait for the - // pending promise to resolve before we start showing that state - setTimeout(() => { - try { - checkLatest() - // Update the match and prematurely resolve the loadMatches promise so that - // the pending component can start rendering - triggerOnReady() - } catch {} - }, pendingMs) - } - - if (match.isFetching) { - continue - } - - const previousResolve = loadPromise.resolve - // Create a new one - loadPromise = createControlledPromise( - // Resolve the old when we we resolve the new one - previousResolve, - ) - - // Otherwise, load the route - matches[index] = match = updateMatch(match.id, (prev) => ({ - ...prev, - isFetching: 'beforeLoad', - loadPromise, - })) - const handleSerialError = (err: any, routerCode: string) => { // If the error is a promise, it means we're outdated and // should abort the current async operation @@ -1797,17 +1779,17 @@ export class Router< err.routerCode = routerCode firstBadMatchIndex = firstBadMatchIndex ?? index - handleRedirectAndNotFound(match, err) + handleRedirectAndNotFound(getMatch(), err) try { route.options.onError?.(err) } catch (errorHandlerErr) { err = errorHandlerErr - handleRedirectAndNotFound(match, err) + handleRedirectAndNotFound(getMatch(), err) } - matches[index] = match = updateMatch(match.id, () => ({ - ...match, + updateMatch(matchId, (prev) => ({ + ...prev, error: err, status: 'error', updatedAt: Date.now(), @@ -1815,73 +1797,128 @@ export class Router< })) } - if (match.paramsError) { - handleSerialError(match.paramsError, 'PARSE_PARAMS') - } + const runBeforeLoad = async () => { + if (getMatch().beforeLoadPromise) { + return await getMatch().beforeLoadPromise! + } - if (match.searchError) { - handleSerialError(match.searchError, 'VALIDATE_SEARCH') - } + const beforeLoadPromise = + createControlledPromise>() - try { - const parentContext = - parentMatch?.context ?? this.options.context ?? {} + const previousResolve = loadPromise.resolve + loadPromise = createControlledPromise(previousResolve) - // Make sure the match has parent context set before going further - matches[index] = match = { - ...match, + const parentContext = getParentContext() + + updateMatch(matchId, (prev) => ({ + ...prev, + isFetching: 'beforeLoad', + beforeLoadPromise, + loadPromise, routeContext: replaceEqualDeep( - match.routeContext, + prev.routeContext, parentContext, ), - context: replaceEqualDeep(match.context, parentContext), + context: replaceEqualDeep(prev.context, parentContext), abortController, - } + })) + + const { search, params, routeContext, cause } = getMatch() const beforeLoadFnContext = { - search: match.search, + search, abortController, - params: match.params, + params, preload: !!preload, - context: match.routeContext, + context: routeContext, location, navigate: (opts: any) => this.navigate({ ...opts, _fromLocation: location }), buildLocation: this.buildLocation, - cause: preload ? 'preload' : match.cause, + cause: preload ? 'preload' : cause, } - const beforeLoadContext = route.options.beforeLoad - ? (await route.options.beforeLoad(beforeLoadFnContext)) ?? {} - : {} - - checkLatest() + try { + const beforeLoadContext = + (await route.options.beforeLoad?.(beforeLoadFnContext)) ?? + {} - if ( - isRedirect(beforeLoadContext) || - isNotFound(beforeLoadContext) - ) { - handleSerialError(beforeLoadContext, 'BEFORE_LOAD') + beforeLoadPromise.resolve(beforeLoadContext) + } catch (err) { + beforeLoadPromise.reject(err) } - const context = { - ...parentContext, - ...beforeLoadContext, - } + // Remove the beforeLoadPromise so future invocations will + // be fresh and not use the cached promise + updateMatch(matchId, (prev) => ({ + ...prev, + beforeLoadPromise: undefined, + })) - matches[index] = match = { - ...match, - routeContext: replaceEqualDeep( - match.routeContext, - beforeLoadContext, - ), - context: replaceEqualDeep(match.context, context), - abortController, + return await beforeLoadPromise + } + + const handleBeforeLoadContext = ( + beforeLoadContext: Record, + ) => { + try { + if ( + isRedirect(beforeLoadContext) || + isNotFound(beforeLoadContext) + ) { + handleSerialError(beforeLoadContext, 'BEFORE_LOAD') + } + + updateMatch(matchId, (prev) => { + const routeContext = { + ...prev.routeContext, + ...beforeLoadContext, + } + + return { + ...prev, + routeContext: replaceEqualDeep( + prev.routeContext, + routeContext, + ), + context: replaceEqualDeep(prev.context, routeContext), + abortController, + } + }) + } catch (err) { + handleSerialError(err, 'BEFORE_LOAD') } - updateMatch(match.id, () => match) + } + + if (shouldPending) { + // If we might show a pending component, we need to wait for the + // pending promise to resolve before we start showing that state + setTimeout(() => { + try { + checkLatest() + // Update the match and prematurely resolve the loadMatches promise so that + // the pending component can start rendering + triggerOnReady() + } catch {} + }, pendingMs) + } + + const { paramsError, searchError } = getMatch() + + if (paramsError) { + handleSerialError(paramsError, 'PARSE_PARAMS') + } + + if (searchError) { + handleSerialError(searchError, 'VALIDATE_SEARCH') + } + + try { + const beforeLoadContext = await runBeforeLoad() + checkLatest() + handleBeforeLoadContext(beforeLoadContext) } catch (err) { handleSerialError(err, 'BEFORE_LOAD') - break } } @@ -1890,235 +1927,251 @@ export class Router< const validResolvedMatches = matches.slice(0, firstBadMatchIndex) const matchPromises: Array> = [] - validResolvedMatches.forEach((match, index) => { - const createValidateResolvedMatchPromise = async () => { - const parentMatchPromise = matchPromises[index - 1] - const route = this.looseRoutesById[match.routeId]! - - const loaderContext: LoaderFnContext = { - params: match.params, - deps: match.loaderDeps, - preload: !!preload, - parentMatchPromise, - abortController: match.abortController, - context: match.context, - location, - navigate: (opts) => - this.navigate({ ...opts, _fromLocation: location }), - cause: preload ? 'preload' : match.cause, - route, - } + validResolvedMatches.forEach( + ({ id: matchId, routeId, params: p }, index) => { + const getMatch = () => getRouteMatch(this.state, matchId)! + + const createValidateResolvedMatchPromise = async () => { + const parentMatchPromise = matchPromises[index - 1] + const route = this.looseRoutesById[routeId]! + + const { + params, + loaderDeps, + abortController, + context, + cause, + } = getMatch() + + const getLoaderContext = (): LoaderFnContext => ({ + params, + deps: loaderDeps, + preload: !!preload, + parentMatchPromise, + abortController: abortController, + context, + location, + navigate: (opts) => + this.navigate({ ...opts, _fromLocation: location }), + cause: preload ? 'preload' : cause, + route, + }) + + const runLoader = async () => { + let { loaderPromise } = getMatch() + + if (loaderPromise) { + return await loaderPromise + } - const fetchAndResolveInLoaderLifetime = async () => { - const existing = getRouteMatch(this.state, match.id)! - let lazyPromise = Promise.resolve() - let componentsPromise = Promise.resolve() as Promise - let loaderPromise = existing.loaderPromise + loaderPromise = + createControlledPromise>() + + const lazyPromise = + route.lazyFn?.().then((lazyRoute) => { + Object.assign(route.options, lazyRoute.options) + }) || Promise.resolve() + + // If for some reason lazy resolves more lazy components... + // We'll wait for that before pre attempt to preload any + // components themselves. + const componentsPromise = lazyPromise.then(() => + Promise.all( + componentTypes.map(async (type) => { + const component = route.options[type] + + if ((component as any)?.preload) { + await (component as any).preload() + } + }), + ), + ) - // If the Matches component rendered - // the pending component and needs to show it for - // a minimum duration, we''ll wait for it to resolve - // before committing to the match and resolving - // the loadPromise - const potentialPendingMinPromise = async () => { - const latestMatch = getRouteMatch(this.state, match.id) + // Otherwise, load the route + updateMatch(matchId, (prev) => ({ + ...prev, + isFetching: 'loader', + fetchCount: prev.fetchCount + 1, + loaderPromise, + componentsPromise, + })) - if (latestMatch?.minPendingPromise) { - await latestMatch.minPendingPromise + // Lazy option can modify the route options, + // so we need to wait for it to resolve before + // we can use the options + await lazyPromise - checkLatest() + checkLatest() - updateMatch(latestMatch.id, (prev) => ({ - ...prev, - minPendingPromise: undefined, - })) + // Kick off the loader! + try { + const loaderData = + await route.options.loader?.(getLoaderContext()) + loaderPromise.resolve(loaderData) + } catch (err) { + loaderPromise.reject(err) } + + // Otherwise, load the route + updateMatch(matchId, (prev) => ({ + ...prev, + loaderPromise: undefined, + })) + + return await loaderPromise } - try { - if (match.isFetching === 'beforeLoad') { - // If the user doesn't want the route to reload, just - // resolve with the existing loader data - - // if (match.fetchCount && match.status === 'success') { - // resolve() - // } - - // Otherwise, load the route - matches[index] = match = updateMatch( - match.id, - (prev) => ({ - ...prev, - isFetching: 'loader', - fetchCount: match.fetchCount + 1, - }), - ) - - lazyPromise = - route.lazyFn?.().then((lazyRoute) => { - Object.assign(route.options, lazyRoute.options) - }) || Promise.resolve() - - // If for some reason lazy resolves more lazy components... - // We'll wait for that before pre attempt to preload any - // components themselves. - componentsPromise = lazyPromise.then(() => - Promise.all( - componentTypes.map(async (type) => { - const component = route.options[type] - - if ((component as any)?.preload) { - await (component as any).preload() - } - }), - ), - ) - - // Lazy option can modify the route options, - // so we need to wait for it to resolve before - // we can use the options - await lazyPromise + const fetchAndResolveInLoaderLifetime = async () => { + // If the Matches component rendered + // the pending component and needs to show it for + // a minimum duration, we''ll wait for it to resolve + // before committing to the match and resolving + // the loadPromise + const potentialPendingMinPromise = async () => { + const latestMatch = getRouteMatch(this.state, matchId) - checkLatest() + if (latestMatch?.minPendingPromise) { + await latestMatch.minPendingPromise - // Kick off the loader! - loaderPromise = route.options.loader?.(loaderContext) + checkLatest() - matches[index] = match = updateMatch( - match.id, - (prev) => ({ + updateMatch(latestMatch.id, (prev) => ({ ...prev, - loaderPromise, - }), - ) + minPendingPromise: undefined, + })) + } } - let loaderData = await loaderPromise - if (this.serializeLoaderData) { - loaderData = this.serializeLoaderData(loaderData, { - router: this, - match, - }) - } - checkLatest() + try { + let loaderData = await runLoader() + if (this.serializeLoaderData) { + loaderData = this.serializeLoaderData(loaderData, { + router: this, + match: getMatch(), + }) + } + checkLatest() - handleRedirectAndNotFound(match, loaderData) + handleRedirectAndNotFound(getMatch(), loaderData) - await potentialPendingMinPromise() - checkLatest() + await potentialPendingMinPromise() + checkLatest() - const meta = route.options.meta?.({ - matches, - params: match.params, - loaderData, - }) + const meta = route.options.meta?.({ + matches, + params: getMatch().params, + loaderData, + }) - const headers = route.options.headers?.({ - loaderData, - }) + const headers = route.options.headers?.({ + loaderData, + }) - matches[index] = match = updateMatch(match.id, (prev) => ({ - ...prev, - error: undefined, - status: 'success', - isFetching: false, - updatedAt: Date.now(), - loaderData, - meta, - headers, - })) - } catch (e) { - checkLatest() - let error = e + updateMatch(matchId, (prev) => ({ + ...prev, + error: undefined, + status: 'success', + isFetching: false, + updatedAt: Date.now(), + loaderData, + meta, + headers, + })) + } catch (e) { + checkLatest() + let error = e - await potentialPendingMinPromise() - checkLatest() + await potentialPendingMinPromise() + checkLatest() - handleRedirectAndNotFound(match, e) + handleRedirectAndNotFound(getMatch(), e) - try { - route.options.onError?.(e) - } catch (onErrorError) { - error = onErrorError - handleRedirectAndNotFound(match, onErrorError) + try { + route.options.onError?.(e) + } catch (onErrorError) { + error = onErrorError + handleRedirectAndNotFound(getMatch(), onErrorError) + } + + updateMatch(matchId, (prev) => ({ + ...prev, + error, + status: 'error', + isFetching: false, + })) } - matches[index] = match = updateMatch(match.id, (prev) => ({ - ...prev, - error, - status: 'error', - isFetching: false, - })) - } + // Last but not least, wait for the the component + // to be preloaded before we resolve the match + await getMatch().componentsPromise - // Last but not least, wait for the the component - // to be preloaded before we resolve the match - await componentsPromise + checkLatest() - checkLatest() + getMatch().loadPromise.resolve() + } - match.loadPromise.resolve() - } + // This is where all of the stale-while-revalidate magic happens + const age = Date.now() - getMatch().updatedAt + + const staleAge = preload + ? route.options.preloadStaleTime ?? + this.options.defaultPreloadStaleTime ?? + 30_000 // 30 seconds for preloads by default + : route.options.staleTime ?? + this.options.defaultStaleTime ?? + 0 + + const shouldReloadOption = route.options.shouldReload + + // Default to reloading the route all the time + // Allow shouldReload to get the last say, + // if provided. + const shouldReload = + typeof shouldReloadOption === 'function' + ? shouldReloadOption(getLoaderContext()) + : shouldReloadOption + + updateMatch(matchId, (prev) => ({ + ...prev, + preload: + !!preload && + !this.state.matches.find((d) => d.id === matchId), + })) + + const fetchWithRedirectAndNotFound = async () => { + try { + await fetchAndResolveInLoaderLifetime() + } catch (err) { + checkLatest() + handleRedirectAndNotFound(getMatch(), err) + } + } - // This is where all of the stale-while-revalidate magic happens - const age = Date.now() - match.updatedAt - - const staleAge = preload - ? route.options.preloadStaleTime ?? - this.options.defaultPreloadStaleTime ?? - 30_000 // 30 seconds for preloads by default - : route.options.staleTime ?? - this.options.defaultStaleTime ?? - 0 - - const shouldReloadOption = route.options.shouldReload - - // Default to reloading the route all the time - // Allow shouldReload to get the last say, - // if provided. - const shouldReload = - typeof shouldReloadOption === 'function' - ? shouldReloadOption(loaderContext) - : shouldReloadOption - - matches[index] = match = { - ...match, - preload: - !!preload && - !this.state.matches.find((d) => d.id === match.id), - } + // If the route is successful and still fresh, just resolve + const { status, invalid } = getMatch() + + if ( + status === 'success' && + (invalid || (shouldReload ?? age > staleAge)) + ) { + ;(async () => { + try { + await fetchWithRedirectAndNotFound() + } catch (err) {} + })() + return + } - const fetchWithRedirectAndNotFound = async () => { - try { - await fetchAndResolveInLoaderLifetime() - } catch (err) { - checkLatest() - handleRedirectAndNotFound(match, err) + if (status !== 'success') { + await fetchWithRedirectAndNotFound() } - } - // If the route is successful and still fresh, just resolve - if ( - match.status === 'success' && - (match.invalid || (shouldReload ?? age > staleAge)) - ) { - ;(async () => { - try { - await fetchWithRedirectAndNotFound() - } catch (err) {} - })() return } - if (match.status !== 'success') { - await fetchWithRedirectAndNotFound() - } - - return - } - - matchPromises.push(createValidateResolvedMatchPromise()) - }) + matchPromises.push(createValidateResolvedMatchPromise()) + }, + ) await Promise.all(matchPromises) @@ -2357,7 +2410,7 @@ export class Router< } } - hydrate = async () => { + hydrate = () => { // Client hydrates from window let ctx: HydrationCtx | undefined @@ -2555,3 +2608,14 @@ export function defaultSerializeError(err: unknown) { data: err, } } + +export function getRouteMatch( + state: RouterState, + id: string, +): undefined | MakeRouteMatch { + return [ + ...state.cachedMatches, + ...(state.pendingMatches ?? []), + ...state.matches, + ].find((d) => d.id === id) +} diff --git a/packages/react-router/tests/redirects.test.tsx b/packages/react-router/tests/redirects.test.tsx index 6f0d2eb95d6..61e34f55cd4 100644 --- a/packages/react-router/tests/redirects.test.tsx +++ b/packages/react-router/tests/redirects.test.tsx @@ -1,11 +1,11 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { + type RouterHistory, createMemoryHistory, createRootRoute, createRoute, createRouter, - type RouterHistory, } from '../src' afterEach(() => { @@ -402,6 +402,7 @@ describe('router.navigate navigation using layout routes resolves correctly', as to: '/u/$username', params: { username: 'tkdodo' }, }) + await router.invalidate() expect(router.state.location.pathname).toBe('/u/tkdodo') @@ -437,6 +438,7 @@ describe('router.navigate navigation using layout routes resolves correctly', as to: '/g/$username', params: { username: 'tkdodo' }, }) + await router.invalidate() expect(router.state.location.pathname).toBe('/g/tkdodo') diff --git a/packages/react-router/tests/route.test-d.tsx b/packages/react-router/tests/route.test-d.tsx index 630f4f2a343..1ecce570825 100644 --- a/packages/react-router/tests/route.test-d.tsx +++ b/packages/react-router/tests/route.test-d.tsx @@ -5,6 +5,7 @@ import { createRoute, createRouter, } from '../src' +import type { ControlledPromise } from '../src' test('when creating the root', () => { const rootRoute = createRootRoute() @@ -610,7 +611,9 @@ test('when creating a child route with context, search, params, loader, loaderDe search: TExpectedSearch context: TExpectedContext loaderDeps: { detailPage: number; invoicePage: number } - loaderPromise: Promise + beforeLoadPromise?: ControlledPromise> + loaderPromise?: ControlledPromise + componentsPromise?: Promise> loaderData?: TExpectedLoaderData routeContext: TExpectedRouteContext } diff --git a/packages/router-plugin/src/code-splitter.ts b/packages/router-plugin/src/code-splitter.ts index 850fd2688a4..bd6c1f3e60a 100644 --- a/packages/router-plugin/src/code-splitter.ts +++ b/packages/router-plugin/src/code-splitter.ts @@ -219,7 +219,7 @@ export const unpluginRouterCodeSplitterFactory: UnpluginFactory< compiler.options.mode === 'production' ) { compiler.hooks.done.tap(PLUGIN_NAME, (stats) => { - console.log('✅ ' + PLUGIN_NAME + ': code-splitting done!') + console.info('✅ ' + PLUGIN_NAME + ': code-splitting done!') setTimeout(() => { process.exit(0) }) diff --git a/packages/router-plugin/src/router-generator.ts b/packages/router-plugin/src/router-generator.ts index 0617f25aa87..065b7e9263d 100644 --- a/packages/router-plugin/src/router-generator.ts +++ b/packages/router-plugin/src/router-generator.ts @@ -119,7 +119,7 @@ export const unpluginRouterGeneratorFactory: UnpluginFactory< if (compiler.options.mode === 'production') { compiler.hooks.done.tap(PLUGIN_NAME, (stats) => { - console.log('✅ ' + PLUGIN_NAME + ': route-tree generation done') + console.info('✅ ' + PLUGIN_NAME + ': route-tree generation done') setTimeout(() => { process.exit(0) }) diff --git a/packages/start/src/client/renderRSC.tsx b/packages/start/src/client/renderRSC.tsx index 656315fa6a5..fbe5d0cbdb8 100644 --- a/packages/start/src/client/renderRSC.tsx +++ b/packages/start/src/client/renderRSC.tsx @@ -13,7 +13,7 @@ export function renderRsc(input: any): JSX.Element { input.state = { status: 'pending', promise: Promise.resolve() - .then(async () => { + .then(() => { let element // We're in node From 27860864485a08b9757cd8c8ff74d632be5f4207 Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Sat, 6 Jul 2024 23:51:44 +0200 Subject: [PATCH 02/34] remove await (and eslint-disable comment) --- packages/react-router/src/router.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/react-router/src/router.ts b/packages/react-router/src/router.ts index 79cc7c04234..73ae51dd2a5 100644 --- a/packages/react-router/src/router.ts +++ b/packages/react-router/src/router.ts @@ -1543,8 +1543,7 @@ export class Router< location: next, checkLatest: () => this.checkLatest(promise), onReady: async () => { - // eslint-disable-next-line ts/require-await - await this.startViewTransition(async () => { + this.startViewTransition(async () => { // this.viewTransitionPromise = createControlledPromise() // Commit the pending matches. If a previous match was From 591d412527212a7dbef004e56ceff60594213cc0 Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Sat, 6 Jul 2024 23:52:50 +0200 Subject: [PATCH 03/34] copied test from https://github.com/TanStack/router/pull/1891 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit this currently fails! AssertionError: expected "spy" to be called 2 times, but got 1 times ❯ tests/routeContext.test.tsx:417:18 415| 416| // Expect double call: once from preload, once from navigate 417| expect(mock).toHaveBeenCalledTimes(2) | ^ 418| }) 419| --- .../react-router/tests/routeContext.test.tsx | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/packages/react-router/tests/routeContext.test.tsx b/packages/react-router/tests/routeContext.test.tsx index 455020a4c07..eb6aadea6b9 100644 --- a/packages/react-router/tests/routeContext.test.tsx +++ b/packages/react-router/tests/routeContext.test.tsx @@ -381,6 +381,42 @@ describe('beforeLoad in the route definition', () => { expect(mock).toHaveBeenCalledTimes(1) }) + test("on navigate (with preload), loader isn't invoked with undefined context if beforeLoad is pending when navigation happens", async () => { + const mock = vi.fn() + + const rootRoute = createRootRoute() + const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + }) + const aboutRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/about', + beforeLoad: async () => { + await sleep(WAIT_TIME) // Use a longer delay here + return { mock } + }, + loader: async ({ context }) => { + await sleep(WAIT_TIME) + context.mock() + }, + }) + + const routeTree = rootRoute.addChildren([aboutRoute, indexRoute]) + const router = createRouter({ routeTree, context: { foo: 'bar' } }) + + await router.load() + + // Don't await, simulate user clicking before preload is done + router.preloadRoute(aboutRoute) + + await router.navigate(aboutRoute) + await router.invalidate() + + // Expect double call: once from preload, once from navigate + expect(mock).toHaveBeenCalledTimes(2) + }) + // Check if context returned by /nested/about, is the same as its parent route /nested on navigate test('nested destination on navigate, route context in the /nested/about route is correctly inherited from the /nested parent', async () => { const mock = vi.fn() From ad4b820fef7dc8d9e7494b358196f8270023183a Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Sun, 7 Jul 2024 00:00:39 +0200 Subject: [PATCH 04/34] copied test from https://github.com/TanStack/router/pull/1876 --- .../tests/createLazyRoute.test.tsx | 58 +++++++++++++++++++ packages/react-router/tests/lazy/heavy.tsx | 8 +++ .../tests/lazy/mockHeavyDependenciesRoute.tsx | 7 +++ 3 files changed, 73 insertions(+) create mode 100644 packages/react-router/tests/createLazyRoute.test.tsx create mode 100644 packages/react-router/tests/lazy/heavy.tsx create mode 100644 packages/react-router/tests/lazy/mockHeavyDependenciesRoute.tsx diff --git a/packages/react-router/tests/createLazyRoute.test.tsx b/packages/react-router/tests/createLazyRoute.test.tsx new file mode 100644 index 00000000000..bb3baeebf66 --- /dev/null +++ b/packages/react-router/tests/createLazyRoute.test.tsx @@ -0,0 +1,58 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + RouterHistory, + createMemoryHistory, + createRootRoute, + createRoute, + createRouter, +} from '../src' +import { cleanup } from '@testing-library/react' + +afterEach(() => { + vi.resetAllMocks() + cleanup() +}) + +function createTestRouter(initialHistory?: RouterHistory) { + const history = + initialHistory ?? createMemoryHistory({ initialEntries: ['/'] }) + + const rootRoute = createRootRoute({}) + const indexRoute = createRoute({ getParentRoute: () => rootRoute, path: '/' }) + + const heavyRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/heavy', + }).lazy(() => import('./lazy/heavy').then((d) => d.default('/heavy'))) + + const routeTree = rootRoute.addChildren([indexRoute, heavyRoute]) + + const router = createRouter({ routeTree, history }) + + return { + router, + routes: { indexRoute, heavyRoute }, + } +} + +describe('preload: matched routes', { timeout: 20000 }, () => { + it('should wait for lazy options to be streamed in before ', async () => { + const { router } = createTestRouter( + createMemoryHistory({ initialEntries: ['/'] }), + ) + + await router.load() + + // Preload the route and navigate to it + router.preloadRoute({ to: '/heavy' }) + await router.navigate({ to: '/heavy' }) + + await router.invalidate() + + expect(router.state.location.pathname).toBe('/heavy') + + const lazyRoute = router.routesByPath['/heavy'] + + expect(lazyRoute.options.component).toBeDefined() + }) +}) diff --git a/packages/react-router/tests/lazy/heavy.tsx b/packages/react-router/tests/lazy/heavy.tsx new file mode 100644 index 00000000000..d7ca39c40f3 --- /dev/null +++ b/packages/react-router/tests/lazy/heavy.tsx @@ -0,0 +1,8 @@ +import { createLazyRoute } from '../../src' +import HeavyComponent from './mockHeavyDependenciesRoute' + +export default function Route(id: string) { + return createLazyRoute(id)({ + component: HeavyComponent, + }) +} diff --git a/packages/react-router/tests/lazy/mockHeavyDependenciesRoute.tsx b/packages/react-router/tests/lazy/mockHeavyDependenciesRoute.tsx new file mode 100644 index 00000000000..2a7ac6e299e --- /dev/null +++ b/packages/react-router/tests/lazy/mockHeavyDependenciesRoute.tsx @@ -0,0 +1,7 @@ + +// This mimicks the waiting of heavy dependencies, which need to be streamed in before the component is available. +await new Promise((resolve) => setTimeout(resolve, 2500)) + +export default function HeavyComponent() { + return

I am sooo heavy

+} \ No newline at end of file From 1e78f0c9d5b0bdb210da648920a903e5f8f34615 Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Sun, 7 Jul 2024 00:07:41 +0200 Subject: [PATCH 05/34] fix formatting --- .../react-router/tests/lazy/mockHeavyDependenciesRoute.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/react-router/tests/lazy/mockHeavyDependenciesRoute.tsx b/packages/react-router/tests/lazy/mockHeavyDependenciesRoute.tsx index 2a7ac6e299e..68fafcd3776 100644 --- a/packages/react-router/tests/lazy/mockHeavyDependenciesRoute.tsx +++ b/packages/react-router/tests/lazy/mockHeavyDependenciesRoute.tsx @@ -1,7 +1,6 @@ - // This mimicks the waiting of heavy dependencies, which need to be streamed in before the component is available. await new Promise((resolve) => setTimeout(resolve, 2500)) export default function HeavyComponent() { return

I am sooo heavy

-} \ No newline at end of file +} From 26e64b177e0f016e434745d5728a2d83c031c0e3 Mon Sep 17 00:00:00 2001 From: Sean Cassiere <33615041+SeanCassiere@users.noreply.github.com> Date: Sun, 7 Jul 2024 15:09:53 +1200 Subject: [PATCH 06/34] for the context issues pr (#1908) * chore(examples): cleanup the about route in "quickstart-file-based" * style(react-router): eslint on "createLazyRoute.test.tsx" * test(react-router): add a test to "createLazyRoute" to check that the heavy component is rendered in the DOM using testing-library * test(react-router): duplicate the preload test using screen rendering from testing library * test(react-router): forgot to add preload intent * chore(react-router): add the comment above the `runBeforeLoad` call stating that we're actually running it * chore(react-router): add comment above the `runLoader` call * chore(react-router): remove the destructuring of `p` * style(react-router): disable these eslint warnings in the `router.ts` file * chore(react-router): undo the `params: p` destucturing change --- .../src/routes/about.tsx | 8 --- packages/react-router/src/router.ts | 4 ++ .../tests/createLazyRoute.test.tsx | 50 +++++++++++++- .../react-router/tests/routeContext.test.tsx | 68 ++++++++++++++++++- 4 files changed, 117 insertions(+), 13 deletions(-) diff --git a/examples/react/quickstart-file-based/src/routes/about.tsx b/examples/react/quickstart-file-based/src/routes/about.tsx index 0eda76df7cb..492e6b85c25 100644 --- a/examples/react/quickstart-file-based/src/routes/about.tsx +++ b/examples/react/quickstart-file-based/src/routes/about.tsx @@ -3,14 +3,6 @@ import { createFileRoute } from '@tanstack/react-router' export const Route = createFileRoute('/about')({ component: AboutComponent, - beforeLoad: async () => { - await new Promise((resolve) => setTimeout(resolve, 2000)) - return { someData: 'hello' } - }, - loader: async ({ context }) => { - await new Promise((resolve) => setTimeout(resolve, 1000)) - console.debug(context.someData) - }, }) function AboutComponent() { diff --git a/packages/react-router/src/router.ts b/packages/react-router/src/router.ts index 73ae51dd2a5..8b07e1244de 100644 --- a/packages/react-router/src/router.ts +++ b/packages/react-router/src/router.ts @@ -1542,7 +1542,9 @@ export class Router< matches: pendingMatches, location: next, checkLatest: () => this.checkLatest(promise), + // eslint-disable-next-line ts/require-await onReady: async () => { + // eslint-disable-next-line ts/require-await this.startViewTransition(async () => { // this.viewTransitionPromise = createControlledPromise() @@ -1912,6 +1914,7 @@ export class Router< handleSerialError(searchError, 'VALIDATE_SEARCH') } + // Actually run the beforeLoad function and get the context try { const beforeLoadContext = await runBeforeLoad() checkLatest() @@ -2041,6 +2044,7 @@ export class Router< } } + // Actually run the loader and handle the result try { let loaderData = await runLoader() if (this.serializeLoaderData) { diff --git a/packages/react-router/tests/createLazyRoute.test.tsx b/packages/react-router/tests/createLazyRoute.test.tsx index bb3baeebf66..ce33c4c6bb8 100644 --- a/packages/react-router/tests/createLazyRoute.test.tsx +++ b/packages/react-router/tests/createLazyRoute.test.tsx @@ -1,12 +1,21 @@ +import React, { act } from 'react' import { afterEach, describe, expect, it, vi } from 'vitest' +import '@testing-library/jest-dom/vitest' +import { cleanup, configure, render, screen } from '@testing-library/react' import { - RouterHistory, + Link, + RouterProvider, + createBrowserHistory, createMemoryHistory, createRootRoute, createRoute, createRouter, } from '../src' -import { cleanup } from '@testing-library/react' +import type { RouterHistory } from '../src' + +// TODO: Move this setup logic including the '@testing-library/jest-dom/vitest' into its own setup file +// @ts-expect-error +global.IS_REACT_ACT_ENVIRONMENT = true afterEach(() => { vi.resetAllMocks() @@ -18,7 +27,16 @@ function createTestRouter(initialHistory?: RouterHistory) { initialHistory ?? createMemoryHistory({ initialEntries: ['/'] }) const rootRoute = createRootRoute({}) - const indexRoute = createRoute({ getParentRoute: () => rootRoute, path: '/' }) + const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + component: () => ( +
+

Index Route

+ Link to heavy +
+ ), + }) const heavyRoute = createRoute({ getParentRoute: () => rootRoute, @@ -36,6 +54,8 @@ function createTestRouter(initialHistory?: RouterHistory) { } describe('preload: matched routes', { timeout: 20000 }, () => { + configure({ reactStrictMode: true }) + it('should wait for lazy options to be streamed in before ', async () => { const { router } = createTestRouter( createMemoryHistory({ initialEntries: ['/'] }), @@ -55,4 +75,28 @@ describe('preload: matched routes', { timeout: 20000 }, () => { expect(lazyRoute.options.component).toBeDefined() }) + + it('should render the heavy/lazy component', async () => { + const { router } = createTestRouter(createBrowserHistory()) + + await act(() => render()) + + const linkToHeavy = await screen.findByText('Link to heavy') + expect(linkToHeavy).toBeInTheDocument() + + expect(router.state.location.pathname).toBe('/') + expect(window.location.pathname).toBe('/') + + // click the link to navigate to the heavy route + act(() => linkToHeavy.click()) + + const heavyElement = await screen.findByText('I am sooo heavy') + expect(heavyElement).toBeInTheDocument() + + expect(router.state.location.pathname).toBe('/heavy') + expect(window.location.pathname).toBe('/heavy') + + const lazyRoute = router.routesByPath['/heavy'] + expect(lazyRoute.options.component).toBeDefined() + }) }) diff --git a/packages/react-router/tests/routeContext.test.tsx b/packages/react-router/tests/routeContext.test.tsx index eb6aadea6b9..48a1222ed2f 100644 --- a/packages/react-router/tests/routeContext.test.tsx +++ b/packages/react-router/tests/routeContext.test.tsx @@ -381,7 +381,8 @@ describe('beforeLoad in the route definition', () => { expect(mock).toHaveBeenCalledTimes(1) }) - test("on navigate (with preload), loader isn't invoked with undefined context if beforeLoad is pending when navigation happens", async () => { + // TODO: Move this test to the loader section + test("on navigate (with preload using router methods), loader isn't invoked with undefined context if beforeLoad is pending when navigation happens", async () => { const mock = vi.fn() const rootRoute = createRootRoute() @@ -403,7 +404,10 @@ describe('beforeLoad in the route definition', () => { }) const routeTree = rootRoute.addChildren([aboutRoute, indexRoute]) - const router = createRouter({ routeTree, context: { foo: 'bar' } }) + const router = createRouter({ + routeTree, + context: { foo: 'bar' }, + }) await router.load() @@ -417,6 +421,66 @@ describe('beforeLoad in the route definition', () => { expect(mock).toHaveBeenCalledTimes(2) }) + // TODO: Move this test to the loader section + test("on navigate (with preload), loader isn't invoked with undefined context if beforeLoad is pending when navigation happens", async () => { + const mock = vi.fn() + + const rootRoute = createRootRoute() + const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + component: () => { + return ( +
+

Index page

+ + link to about + +
+ ) + }, + }) + const aboutRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/about', + beforeLoad: async () => { + await sleep(WAIT_TIME) // Use a longer delay here + return { mock } + }, + loader: async ({ context }) => { + await sleep(WAIT_TIME) + context.mock() + }, + component: () =>
About page
, + }) + + const routeTree = rootRoute.addChildren([aboutRoute, indexRoute]) + const router = createRouter({ + routeTree, + defaultPreload: 'intent', + context: { foo: 'bar' }, + }) + + await act(() => render()) + + const linkToAbout = await screen.findByRole('link', { + name: 'link to about', + }) + expect(linkToAbout).toBeInTheDocument() + + // Don't await, simulate user clicking before preload is done + linkToAbout.focus() + linkToAbout.click() + + const aboutElement = await screen.findByText('About page') + expect(aboutElement).toBeInTheDocument() + + expect(window.location.pathname).toBe('/about') + + // Expect double call: once from preload, once from navigate + expect(mock).toHaveBeenCalledTimes(2) + }) + // Check if context returned by /nested/about, is the same as its parent route /nested on navigate test('nested destination on navigate, route context in the /nested/about route is correctly inherited from the /nested parent', async () => { const mock = vi.fn() From f0a5f54540cdedccdadb0572372227b0c528ae9b Mon Sep 17 00:00:00 2001 From: Tanner Linsley Date: Sat, 6 Jul 2024 22:22:54 -0600 Subject: [PATCH 07/34] fix(router): it's now safe to preload an active route; isLoading is always reset now --- packages/react-router/src/router.ts | 29 +++++++---------------------- 1 file changed, 7 insertions(+), 22 deletions(-) diff --git a/packages/react-router/src/router.ts b/packages/react-router/src/router.ts index 8b07e1244de..8e9ae9e25fa 100644 --- a/packages/react-router/src/router.ts +++ b/packages/react-router/src/router.ts @@ -2157,16 +2157,18 @@ export class Router< status === 'success' && (invalid || (shouldReload ?? age > staleAge)) ) { - ;(async () => { + return (async () => { try { await fetchWithRedirectAndNotFound() } catch (err) {} })() - return - } - - if (status !== 'success') { + } else if (status !== 'success') { await fetchWithRedirectAndNotFound() + } else { + updateMatch(matchId, (prev) => ({ + ...prev, + isFetching: false, + })) } return @@ -2292,23 +2294,6 @@ export class Router< }) }) - // If the preload leaf match is the same as the current or pending leaf match, - // do not preload as it could cause a mutation of the current route. - // The user should specify proper loaderDeps (which are used to uniquely identify a route) - // to trigger preloads for routes with the same pathname, but different deps - - const leafMatch = last(matches) - const currentLeafMatch = last(this.state.matches) - const pendingLeafMatch = last(this.state.pendingMatches ?? []) - - if ( - leafMatch && - (currentLeafMatch?.id === leafMatch.id || - pendingLeafMatch?.id === leafMatch.id) - ) { - return undefined - } - try { matches = await this.loadMatches({ matches, From 18c4429617397d682b2f59659282fdc66cd5497b Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Sun, 7 Jul 2024 11:32:42 +0200 Subject: [PATCH 08/34] do not export `getRouteMatch` --- packages/react-router/src/index.tsx | 1 - packages/react-router/src/router.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/react-router/src/index.tsx b/packages/react-router/src/index.tsx index ca5c951eb4d..76990d9c37a 100644 --- a/packages/react-router/src/index.tsx +++ b/packages/react-router/src/index.tsx @@ -183,7 +183,6 @@ export { PathParamError, getInitialRouterState, defaultSerializeError, - getRouteMatch, type Register, type AnyRouter, type RegisteredRouter, diff --git a/packages/react-router/src/router.ts b/packages/react-router/src/router.ts index 8e9ae9e25fa..0de3ef8da03 100644 --- a/packages/react-router/src/router.ts +++ b/packages/react-router/src/router.ts @@ -2597,7 +2597,7 @@ export function defaultSerializeError(err: unknown) { } } -export function getRouteMatch( +function getRouteMatch( state: RouterState, id: string, ): undefined | MakeRouteMatch { From e43c69f951bdb6d1738a6b36c83f210ea193c188 Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Sun, 7 Jul 2024 11:46:36 +0200 Subject: [PATCH 09/34] fix tests --- .../react-router/tests/routeContext.test.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/react-router/tests/routeContext.test.tsx b/packages/react-router/tests/routeContext.test.tsx index 48a1222ed2f..db003eb908c 100644 --- a/packages/react-router/tests/routeContext.test.tsx +++ b/packages/react-router/tests/routeContext.test.tsx @@ -394,11 +394,12 @@ describe('beforeLoad in the route definition', () => { getParentRoute: () => rootRoute, path: '/about', beforeLoad: async () => { - await sleep(WAIT_TIME) // Use a longer delay here + await sleep(WAIT_TIME) return { mock } }, loader: async ({ context }) => { await sleep(WAIT_TIME) + expect(context.mock).toBe(mock) context.mock() }, }) @@ -417,8 +418,8 @@ describe('beforeLoad in the route definition', () => { await router.navigate(aboutRoute) await router.invalidate() - // Expect double call: once from preload, once from navigate - expect(mock).toHaveBeenCalledTimes(2) + // Expect only a single call as the one from preload and the one from navigate are deduped + expect(mock).toHaveBeenCalledOnce() }) // TODO: Move this test to the loader section @@ -444,11 +445,12 @@ describe('beforeLoad in the route definition', () => { getParentRoute: () => rootRoute, path: '/about', beforeLoad: async () => { - await sleep(WAIT_TIME) // Use a longer delay here + await sleep(WAIT_TIME) return { mock } }, loader: async ({ context }) => { await sleep(WAIT_TIME) + expect(context.mock).toBe(mock) context.mock() }, component: () =>
About page
, @@ -461,7 +463,7 @@ describe('beforeLoad in the route definition', () => { context: { foo: 'bar' }, }) - await act(() => render()) + render() const linkToAbout = await screen.findByRole('link', { name: 'link to about', @@ -477,8 +479,8 @@ describe('beforeLoad in the route definition', () => { expect(window.location.pathname).toBe('/about') - // Expect double call: once from preload, once from navigate - expect(mock).toHaveBeenCalledTimes(2) + // Expect only a single call as the one from preload and the one from navigate are deduped + expect(mock).toHaveBeenCalledOnce() }) // Check if context returned by /nested/about, is the same as its parent route /nested on navigate From 1df34b5f65ae82a1aac28bc665222c128ac7ebf2 Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Sun, 7 Jul 2024 16:25:55 +0200 Subject: [PATCH 10/34] rename file to match tests --- .../react-router/tests/{redirects.test.tsx => navigate.test.tsx} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename packages/react-router/tests/{redirects.test.tsx => navigate.test.tsx} (100%) diff --git a/packages/react-router/tests/redirects.test.tsx b/packages/react-router/tests/navigate.test.tsx similarity index 100% rename from packages/react-router/tests/redirects.test.tsx rename to packages/react-router/tests/navigate.test.tsx From d00653d8f76518c5a3f626eb5074cfee9614243d Mon Sep 17 00:00:00 2001 From: Manuel Schiller Date: Sun, 7 Jul 2024 16:30:42 +0200 Subject: [PATCH 11/34] move redirect tests into redirect.test.tsx --- packages/react-router/tests/loaders.test.tsx | 128 -------------- packages/react-router/tests/redirect.test.tsx | 165 ++++++++++++++++++ 2 files changed, 165 insertions(+), 128 deletions(-) create mode 100644 packages/react-router/tests/redirect.test.tsx diff --git a/packages/react-router/tests/loaders.test.tsx b/packages/react-router/tests/loaders.test.tsx index 24cc5bff7cc..e13cce14689 100644 --- a/packages/react-router/tests/loaders.test.tsx +++ b/packages/react-router/tests/loaders.test.tsx @@ -114,132 +114,4 @@ describe('loaders are being called', () => { expect(nestedLoaderMock).toHaveBeenCalled() expect(nestedFooLoaderMock).toHaveBeenCalled() }) - - test('both are called on /nested/foo when redirected in "beforeLoad" from /about', async () => { - const nestedLoaderMock = vi.fn() - const nestedFooLoaderMock = vi.fn() - - const rootRoute = createRootRoute({}) - const indexRoute = createRoute({ - getParentRoute: () => rootRoute, - path: '/', - component: () => { - return ( -
-

Index page

- link to about -
- ) - }, - }) - const aboutRoute = createRoute({ - getParentRoute: () => rootRoute, - path: '/about', - beforeLoad: async () => { - await sleep(WAIT_TIME) - throw redirect({ to: '/nested/foo' }) - }, - }) - const nestedRoute = createRoute({ - getParentRoute: () => rootRoute, - path: '/nested', - loader: async () => { - await sleep(WAIT_TIME) - nestedLoaderMock('nested') - }, - }) - const fooRoute = createRoute({ - getParentRoute: () => nestedRoute, - path: '/foo', - loader: async () => { - await sleep(WAIT_TIME) - nestedFooLoaderMock('foo') - }, - component: () =>
Nested Foo page
, - }) - const routeTree = rootRoute.addChildren([ - nestedRoute.addChildren([fooRoute]), - aboutRoute, - indexRoute, - ]) - const router = await act(() => createRouter({ routeTree })) - - await act(() => render()) - - const linkToAbout = await screen.findByText('link to about') - await act(() => fireEvent.click(linkToAbout)) - - const fooElement = await screen.findByText('Nested Foo page') - expect(fooElement).toBeInTheDocument() - - expect(router.state.location.href).toBe('/nested/foo') - expect(window.location.pathname).toBe('/nested/foo') - - expect(nestedLoaderMock).toHaveBeenCalled() - expect(nestedFooLoaderMock).toHaveBeenCalled() - }) - - test('both are called on /nested/foo when redirected in "loader" from /about', async () => { - const nestedLoaderMock = vi.fn() - const nestedFooLoaderMock = vi.fn() - - const rootRoute = createRootRoute({}) - const indexRoute = createRoute({ - getParentRoute: () => rootRoute, - path: '/', - component: () => { - return ( -
-

Index page

- link to about -
- ) - }, - }) - const aboutRoute = createRoute({ - getParentRoute: () => rootRoute, - path: '/about', - loader: async () => { - await sleep(WAIT_TIME) - throw redirect({ to: '/nested/foo' }) - }, - }) - const nestedRoute = createRoute({ - getParentRoute: () => rootRoute, - path: '/nested', - loader: async () => { - await sleep(WAIT_TIME) - nestedLoaderMock('nested') - }, - }) - const fooRoute = createRoute({ - getParentRoute: () => nestedRoute, - path: '/foo', - loader: async () => { - await sleep(WAIT_TIME) - nestedFooLoaderMock('foo') - }, - component: () =>
Nested Foo page
, - }) - const routeTree = rootRoute.addChildren([ - nestedRoute.addChildren([fooRoute]), - aboutRoute, - indexRoute, - ]) - const router = await act(() => createRouter({ routeTree })) - - await act(() => render()) - - const linkToAbout = await screen.findByText('link to about') - await act(() => fireEvent.click(linkToAbout)) - - const fooElement = await screen.findByText('Nested Foo page') - expect(fooElement).toBeInTheDocument() - - expect(router.state.location.href).toBe('/nested/foo') - expect(window.location.pathname).toBe('/nested/foo') - - expect(nestedLoaderMock).toHaveBeenCalled() - expect(nestedFooLoaderMock).toHaveBeenCalled() - }) }) diff --git a/packages/react-router/tests/redirect.test.tsx b/packages/react-router/tests/redirect.test.tsx new file mode 100644 index 00000000000..3162b06f4b0 --- /dev/null +++ b/packages/react-router/tests/redirect.test.tsx @@ -0,0 +1,165 @@ +import React, { act } from 'react' +import '@testing-library/jest-dom/vitest' +import { + cleanup, + configure, + fireEvent, + render, + screen, +} from '@testing-library/react' + +import { afterEach, describe, expect, test, vi } from 'vitest' + +import { + Link, + RouterProvider, + createRootRoute, + createRoute, + createRouter, + redirect, +} from '../src' + +import { sleep } from './utils' + +afterEach(() => { + vi.clearAllMocks() + vi.resetAllMocks() + window.history.replaceState(null, 'root', '/') + cleanup() +}) + +const WAIT_TIME = 100 + +describe('redirect', () => { + configure({ reactStrictMode: true }) + + describe('`beforeLoad` and `loader` are called when redirecting', () => { + test('when `redirect` is thrown in `beforeLoad`', async () => { + const nestedLoaderMock = vi.fn() + const nestedFooLoaderMock = vi.fn() + + const rootRoute = createRootRoute({}) + const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + component: () => { + return ( +
+

Index page

+ link to about +
+ ) + }, + }) + const aboutRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/about', + beforeLoad: async () => { + await sleep(WAIT_TIME) + throw redirect({ to: '/nested/foo' }) + }, + }) + const nestedRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/nested', + loader: async () => { + await sleep(WAIT_TIME) + nestedLoaderMock('nested') + }, + }) + const fooRoute = createRoute({ + getParentRoute: () => nestedRoute, + path: '/foo', + loader: async () => { + await sleep(WAIT_TIME) + nestedFooLoaderMock('foo') + }, + component: () =>
Nested Foo page
, + }) + const routeTree = rootRoute.addChildren([ + nestedRoute.addChildren([fooRoute]), + aboutRoute, + indexRoute, + ]) + const router = await act(() => createRouter({ routeTree })) + + await act(() => render()) + + const linkToAbout = await screen.findByText('link to about') + await act(() => fireEvent.click(linkToAbout)) + + const fooElement = await screen.findByText('Nested Foo page') + expect(fooElement).toBeInTheDocument() + + expect(router.state.location.href).toBe('/nested/foo') + expect(window.location.pathname).toBe('/nested/foo') + + expect(nestedLoaderMock).toHaveBeenCalled() + expect(nestedFooLoaderMock).toHaveBeenCalled() + }) + + test('when `redirect` is thrown in `loader`', async () => { + const nestedLoaderMock = vi.fn() + const nestedFooLoaderMock = vi.fn() + + const rootRoute = createRootRoute({}) + const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + component: () => { + return ( +
+

Index page

+ link to about +
+ ) + }, + }) + const aboutRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/about', + loader: async () => { + await sleep(WAIT_TIME) + throw redirect({ to: '/nested/foo' }) + }, + }) + const nestedRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/nested', + loader: async () => { + await sleep(WAIT_TIME) + nestedLoaderMock('nested') + }, + }) + const fooRoute = createRoute({ + getParentRoute: () => nestedRoute, + path: '/foo', + loader: async () => { + await sleep(WAIT_TIME) + nestedFooLoaderMock('foo') + }, + component: () =>
Nested Foo page
, + }) + const routeTree = rootRoute.addChildren([ + nestedRoute.addChildren([fooRoute]), + aboutRoute, + indexRoute, + ]) + const router = await act(() => createRouter({ routeTree })) + + await act(() => render()) + + const linkToAbout = await screen.findByText('link to about') + await act(() => fireEvent.click(linkToAbout)) + + const fooElement = await screen.findByText('Nested Foo page') + expect(fooElement).toBeInTheDocument() + + expect(router.state.location.href).toBe('/nested/foo') + expect(window.location.pathname).toBe('/nested/foo') + + expect(nestedLoaderMock).toHaveBeenCalled() + expect(nestedFooLoaderMock).toHaveBeenCalled() + }) + }) +}) From a7eb59913830dbb50009d505d2b4d0a7e8eeab2b Mon Sep 17 00:00:00 2001 From: Tanner Linsley Date: Sun, 7 Jul 2024 23:17:05 -0600 Subject: [PATCH 12/34] fix: automatically reset route-controlled error boundaries with `router.invalidate()` Fixes #1674 --- docs/framework/react/guide/data-loading.md | 28 +++++++++++++++++-- .../react/guide/external-data-loading.md | 4 +-- .../src/routes/posts.$postId.tsx | 11 ++++---- examples/react/basic-react-query/src/main.tsx | 13 ++++----- packages/react-router/src/CatchBoundary.tsx | 10 +++++-- packages/react-router/src/Match.tsx | 2 +- packages/react-router/src/Matches.tsx | 2 +- packages/react-router/src/router.ts | 12 +++++++- 8 files changed, 58 insertions(+), 24 deletions(-) diff --git a/docs/framework/react/guide/data-loading.md b/docs/framework/react/guide/data-loading.md index 220ae2a15c6..b3fa6fcb7b4 100644 --- a/docs/framework/react/guide/data-loading.md +++ b/docs/framework/react/guide/data-loading.md @@ -450,7 +450,7 @@ export const Route = createFileRoute('/posts')({ }) ``` -The `reset` function can be used to show a `retry` button. If you want to retry the route loading, you need to additionally call `router.invalidate()`: +The `reset` function can be used to allow the user to retry rendering the error boundaries normal children: ```tsx // routes/posts.tsx @@ -466,7 +466,31 @@ export const Route = createFileRoute('/posts')({ onClick={() => { // Reset the router error boundary reset() - // Invalidate the route to reload the loader + }} + > + retry + + + ) + }, +}) +``` + +If the error was the result of a route load, you should instead call `router.invalidate()`, which will coordinate both a router reload and an error boundary reset: + +```tsx +// routes/posts.tsx +export const Route = createFileRoute('/posts')({ + loader: () => fetchPosts(), + errorComponent: ({ error, reset }) => { + const router = useRouter() + + return ( +
+ {error.message} +