Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 91 additions & 0 deletions packages/react-router/tests/routeContext.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3125,4 +3125,95 @@ describe('useRouteContext in the component', () => {

expect(content).toBeInTheDocument()
})

test("reproducer #4998 - on navigate (with preload), route component isn't rendered with undefined context if beforeLoad is pending", async () => {
const beforeLoad = vi.fn()
const select = vi.fn()
let resolved = 0
const rootRoute = createRootRoute()
const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => <Link to="/foo">To foo</Link>,
})
const fooRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/foo',
beforeLoad: async (...args) => {
beforeLoad(...args)
await sleep(WAIT_TIME)
resolved += 1
return { foo: resolved }
},
component: () => {
fooRoute.useRouteContext({ select })
return <h1>Foo index page</h1>
},
pendingComponent: () => 'loading',
})
const routeTree = rootRoute.addChildren([indexRoute, fooRoute])
const router = createRouter({
routeTree,
history,
defaultPreload: 'intent',
defaultPendingMs: 0,
})

render(<RouterProvider router={router} />)
const linkToFoo = await screen.findByText('To foo')

fireEvent.focus(linkToFoo)
expect(beforeLoad).toHaveBeenCalledTimes(1)
expect(resolved).toBe(0)

await sleep(WAIT_TIME)

expect(beforeLoad).toHaveBeenCalledTimes(1)
expect(resolved).toBe(1)
expect(beforeLoad).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
cause: 'preload',
preload: true,
}),
)

expect(select).not.toHaveBeenCalled()

fireEvent.click(linkToFoo)
expect(beforeLoad).toHaveBeenCalledTimes(2)
expect(resolved).toBe(1)

await screen.findByText('Foo index page')

expect(beforeLoad).toHaveBeenCalledTimes(2)
expect(beforeLoad).toHaveBeenNthCalledWith(
2,
expect.objectContaining({
cause: 'enter',
preload: false,
}),
)
expect(select).toHaveBeenNthCalledWith(
1,
expect.objectContaining({
foo: 1,
}),
)
expect(select).not.toHaveBeenCalledWith({})

await sleep(WAIT_TIME)
expect(beforeLoad).toHaveBeenCalledTimes(2)
expect(resolved).toBe(2)

// ensure the context has been updated once the beforeLoad has resolved
expect(select).toHaveBeenLastCalledWith(
expect.objectContaining({
foo: 2,
}),
)

// the route component will be rendered multiple times, ensure it always has the context
expect(select).not.toHaveBeenCalledWith({})
})
})
6 changes: 5 additions & 1 deletion packages/router-core/src/load-matches.ts
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,11 @@ const executeBeforeLoad = (
const parentMatchContext =
parentMatch?.context ?? inner.router.options.context ?? undefined

const context = { ...parentMatchContext, ...match.__routeContext }
const context = {
...match.__beforeLoadContext,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not correct IMO as this will feed the previous beforeLoad context into beforeLoad again.

Copy link
Author

@longzheng longzheng Aug 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schiller-manuel yes that is the intention, as otherwise the route component will be rendered while the beforeLoad is resolving without the beforeLoad context, which leads to the runtime undefined issues in #4998 .

Or are you suggesting that the route component shouldn't be rendered at all while the beforeLoad is resolving? If so I couldn't pinpoint when that behaviour was changed since I went back to over a year ago and it seems to have been always the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schiller-manuel this also makes sense to me, FWIW. I'm not sure what the behaviour was before, but from a type perspective it definitely makes more sense to include the context generated by the route preload. Perhaps there's another approach that would fix this. Perhaps preload needs to load data into different context so it's unique compared to __beforeLoadContext? Feels a little clunky but this is blocking our usage of preload so i'll take any ideas. Alternatively, what if we have a flag to indicate that __beforeLoadContext was loaded by preload and only merge it then, would that satisfy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created #5701 that I think solves the core issue by making sure we only merge when updating context for pending state after a preload. Ideally we could somehow not render component during pending but that's not how suspense works so at least this way we maintain the runtime typesafety of context.

...parentMatchContext,
...match.__routeContext,
}

let isPending = false
const pending = () => {
Expand Down