From 19b6d07a519eebca77b6b26b52ef836c895426dd Mon Sep 17 00:00:00 2001 From: "Gianmarco Rengucci (freshgiammi)" Date: Wed, 3 Jul 2024 17:37:40 +0200 Subject: [PATCH 1/3] test: add test for issue --- .../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 455020a4c0..eb6aadea6b 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 9355e972c0a4bab2cdf49bfe239710926ccbc3dc Mon Sep 17 00:00:00 2001 From: "Gianmarco Rengucci (freshgiammi)" Date: Thu, 4 Jul 2024 17:55:51 +0200 Subject: [PATCH 2/3] fix: ensure context is consistent on concurrent match loads --- packages/react-router/src/Matches.tsx | 1 + packages/react-router/src/router.ts | 30 +++++++++++++++++++-------- 2 files changed, 22 insertions(+), 9 deletions(-) diff --git a/packages/react-router/src/Matches.tsx b/packages/react-router/src/Matches.tsx index d842b7507e..3f10605653 100644 --- a/packages/react-router/src/Matches.tsx +++ b/packages/react-router/src/Matches.tsx @@ -48,6 +48,7 @@ export interface RouteMatch< searchError: unknown updatedAt: number loadPromise: ControlledPromise + beforeLoadPromise: ControlledPromise loaderPromise: Promise loaderData?: TLoaderData routeContext: TRouteContext diff --git a/packages/react-router/src/router.ts b/packages/react-router/src/router.ts index 203c7d9263..b2011303d3 100644 --- a/packages/react-router/src/router.ts +++ b/packages/react-router/src/router.ts @@ -1082,6 +1082,7 @@ export class Router< isFetching: false, error: undefined, paramsError: parseErrors[index], + beforeLoadPromise: createControlledPromise(), loaderPromise: Promise.resolve(), loadPromise, routeContext: undefined!, @@ -1785,7 +1786,9 @@ export class Router< matches[index] = match = updateMatch(match.id, (prev) => ({ ...prev, isFetching: 'beforeLoad', + beforeLoadPromise: createControlledPromise(), loadPromise, + abortController, })) const handleSerialError = (err: any, routerCode: string) => { @@ -1869,7 +1872,7 @@ export class Router< ...beforeLoadContext, } - matches[index] = match = { + matches[index] = match = updateMatch(match.id, () => ({ ...match, routeContext: replaceEqualDeep( match.routeContext, @@ -1877,8 +1880,9 @@ export class Router< ), context: replaceEqualDeep(match.context, context), abortController, - } - updateMatch(match.id, () => match) + })) + + match.beforeLoadPromise.resolve() } catch (err) { handleSerialError(err, 'BEFORE_LOAD') break @@ -1895,6 +1899,18 @@ export class Router< const parentMatchPromise = matchPromises[index - 1] const route = this.looseRoutesById[match.routeId]! + // In case the beforeLoad isn't done (such as multiple load routines running concurently) + // we need to await it, to ensure context is correctly generated + if (match.beforeLoadPromise.status === 'pending') { + await match.beforeLoadPromise + const existing = getRouteMatch(this.state, match.id)! + matches[index] = match = { + ...match, + routeContext: existing.routeContext, + context: existing.context, + } + } + const loaderContext: LoaderFnContext = { params: match.params, deps: match.loaderDeps, @@ -1910,10 +1926,8 @@ export class Router< } const fetchAndResolveInLoaderLifetime = async () => { - const existing = getRouteMatch(this.state, match.id)! let lazyPromise = Promise.resolve() let componentsPromise = Promise.resolve() as Promise - let loaderPromise = existing.loaderPromise // If the Matches component rendered // the pending component and needs to show it for @@ -1982,18 +1996,16 @@ export class Router< checkLatest() // Kick off the loader! - loaderPromise = route.options.loader?.(loaderContext) - matches[index] = match = updateMatch( match.id, (prev) => ({ ...prev, - loaderPromise, + loaderPromise: route.options.loader?.(loaderContext), }), ) } - let loaderData = await loaderPromise + let loaderData = await match.loaderPromise if (this.serializeLoaderData) { loaderData = this.serializeLoaderData(loaderData, { router: this, From 11e02da3120c98ccea508b813ebaccd45feb538d Mon Sep 17 00:00:00 2001 From: "Gianmarco Rengucci (freshgiammi)" Date: Fri, 5 Jul 2024 16:20:48 +0200 Subject: [PATCH 3/3] fixup! fix: ensure context is consistent on concurrent match loads --- packages/react-router/src/router.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-router/src/router.ts b/packages/react-router/src/router.ts index b2011303d3..89a6781067 100644 --- a/packages/react-router/src/router.ts +++ b/packages/react-router/src/router.ts @@ -1996,11 +1996,14 @@ export class Router< checkLatest() // Kick off the loader! + const loaderPromise = + route.options.loader?.(loaderContext) + matches[index] = match = updateMatch( match.id, (prev) => ({ ...prev, - loaderPromise: route.options.loader?.(loaderContext), + loaderPromise, }), ) }