-
-
Notifications
You must be signed in to change notification settings - Fork 635
fix(tanstack-router): remove Suspense gate around RouterProvider during hydration #3213
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
10c1822
12662dd
27355e9
2ee58c9
c835ce7
77b1845
839ecbf
72ce926
87d1e0d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||||||||
| import { Suspense, createElement, useEffect, useRef, type ReactElement } from 'react'; | ||||||||||||||||||||||||||
| import { createElement, useEffect, useRef, type ReactElement } from 'react'; | ||||||||||||||||||||||||||
| import type { | ||||||||||||||||||||||||||
| DehydratedRouterState, | ||||||||||||||||||||||||||
| TanStackHistory, | ||||||||||||||||||||||||||
|
|
@@ -22,32 +22,6 @@ type TanStackRouterChunkPreloadInternals = TanStackRouter & { | |||||||||||||||||||||||||
| looseRoutesById?: Record<string, unknown>; | ||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| interface RouteChunkPreloadGateProps { | ||||||||||||||||||||||||||
| preloadPromise: Promise<void> | null; | ||||||||||||||||||||||||||
| preloadSettledRef: { current: boolean }; | ||||||||||||||||||||||||||
| isHydrating?: boolean; | ||||||||||||||||||||||||||
| children?: ReactElement; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| function RouteChunkPreloadGate({ | ||||||||||||||||||||||||||
| preloadPromise, | ||||||||||||||||||||||||||
| preloadSettledRef, | ||||||||||||||||||||||||||
| isHydrating, | ||||||||||||||||||||||||||
| children, | ||||||||||||||||||||||||||
| }: RouteChunkPreloadGateProps): ReactElement { | ||||||||||||||||||||||||||
| // During SSR hydration (first render), skip the suspension gate to avoid a | ||||||||||||||||||||||||||
| // hydration mismatch: the server rendered RouterProvider content directly, | ||||||||||||||||||||||||||
| // so throwing a promise here would cause Suspense to render the null fallback | ||||||||||||||||||||||||||
| // instead of matching the server HTML. After hydration completes (the | ||||||||||||||||||||||||||
| // post-mount effect sets didTriggerPostHydrationLoadRef), the gate activates | ||||||||||||||||||||||||||
| // normally for any subsequent re-renders. | ||||||||||||||||||||||||||
| if (!isHydrating && preloadPromise && !preloadSettledRef.current) { | ||||||||||||||||||||||||||
| // eslint-disable-next-line @typescript-eslint/only-throw-error -- Suspense boundaries intentionally suspend on thrown Promise. | ||||||||||||||||||||||||||
| throw preloadPromise; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| return children as ReactElement; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| function extractDehydratedData(dehydratedRouter: unknown): unknown { | ||||||||||||||||||||||||||
| if (!dehydratedRouter || typeof dehydratedRouter !== 'object') { | ||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||
|
|
@@ -183,8 +157,9 @@ function TanStackHydrationApp({ | |||||||||||||||||||||||||
| const routerRef = useRef<TanStackRouter | null>(null); | ||||||||||||||||||||||||||
| const didTriggerPostHydrationLoadRef = useRef(false); | ||||||||||||||||||||||||||
| const didSetSsrFlagRef = useRef(false); | ||||||||||||||||||||||||||
| // Set during render-phase SSR init; awaited in runPostHydrationLoad before | ||||||||||||||||||||||||||
| // router.load() so post-hydration navigation waits for matched lazy chunks. | ||||||||||||||||||||||||||
| const routeChunkPreloadPromiseRef = useRef<Promise<void> | null>(null); | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment explains the history of what was removed rather than explaining the current invariant. Since future readers won't know what was removed, the backward-looking framing ("since the Suspense/gate wrapper was removed") is confusing. Consider simplifying to just the forward-looking contract:
Suggested change
|
||||||||||||||||||||||||||
| const routeChunkPreloadSettledRef = useRef(true); | ||||||||||||||||||||||||||
| const hydrationCallbackPromiseRef = useRef<Promise<void> | null>(null); | ||||||||||||||||||||||||||
| const didWarnPrivateInternalsRef = useRef(false); | ||||||||||||||||||||||||||
| const warnedMissingSsrMatchIdsRef = useRef<Set<string>>(new Set()); | ||||||||||||||||||||||||||
|
|
@@ -259,14 +234,6 @@ function TanStackHydrationApp({ | |||||||||||||||||||||||||
| router as TanStackRouterChunkPreloadInternals, | ||||||||||||||||||||||||||
| rawMatches, | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| if (routeChunkPreloadPromiseRef.current) { | ||||||||||||||||||||||||||
| routeChunkPreloadSettledRef.current = false; | ||||||||||||||||||||||||||
| void routeChunkPreloadPromiseRef.current.finally(() => { | ||||||||||||||||||||||||||
| routeChunkPreloadSettledRef.current = true; | ||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||
| routeChunkPreloadSettledRef.current = true; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| const ssrMatches = dehydratedState?.ssrRouter?.matches; | ||||||||||||||||||||||||||
| const matches = ssrMatches?.length | ||||||||||||||||||||||||||
| ? applyDehydratedMatchData(rawMatches, ssrMatches, warnMissingSsrMatch) | ||||||||||||||||||||||||||
|
|
@@ -397,29 +364,11 @@ function TanStackHydrationApp({ | |||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||
| }, [hasSsrPayload, router]); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Always use RouterProvider directly — matching the server-rendered tree. | ||||||||||||||||||||||||||
| // RouterClient is NOT used because it wraps RouterProvider in <Await> which | ||||||||||||||||||||||||||
| // introduces a Suspense boundary that doesn't exist in the server HTML, | ||||||||||||||||||||||||||
| // causing React hydration mismatch errors. | ||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||
| // RouteChunkPreloadGate blocks re-renders until matched lazy chunks finish | ||||||||||||||||||||||||||
| // preloading (when preload support is available). During the initial SSR | ||||||||||||||||||||||||||
| // hydration render, the gate is skipped to match the server-rendered tree | ||||||||||||||||||||||||||
| // and avoid a hydration mismatch. After the post-mount effect runs | ||||||||||||||||||||||||||
| // (didTriggerPostHydrationLoadRef becomes true), the gate activates normally. | ||||||||||||||||||||||||||
| let app: ReactElement = createElement( | ||||||||||||||||||||||||||
| Suspense, | ||||||||||||||||||||||||||
| { fallback: null }, | ||||||||||||||||||||||||||
| createElement( | ||||||||||||||||||||||||||
| RouteChunkPreloadGate, | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| preloadPromise: routeChunkPreloadPromiseRef.current, | ||||||||||||||||||||||||||
| preloadSettledRef: routeChunkPreloadSettledRef, | ||||||||||||||||||||||||||
| isHydrating: hasSsrPayload && !didTriggerPostHydrationLoadRef.current, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| createElement(RouterProvider, { router }), | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| // Render RouterProvider directly — matching the server-rendered tree | ||||||||||||||||||||||||||
| // (AppWrapper > RouterProvider). Any extra Suspense boundary here produces | ||||||||||||||||||||||||||
| // a shape mismatch during hydration and React bails to full client-side | ||||||||||||||||||||||||||
| // rendering. | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The old Worth leaving a note in the comment so a future reader does not assume the gate needs to be re-introduced:
Suggested change
|
||||||||||||||||||||||||||
| let app: ReactElement = createElement(RouterProvider, { router }); | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the right fix. Even with the old One thing worth adding to the comment:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the correct fix. One follow-up worth considering: there is no test that asserts this path never suspends. If a future change re-adds a
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the right fix. Worth noting the behavioral shift: the old
Seifeldin7 marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment explains the hydration-mismatch side of this decision well. Worth adding one sentence about the re-render behavior change:
Suggested change
|
||||||||||||||||||||||||||
| if (options.AppWrapper) { | ||||||||||||||||||||||||||
| const wrapperProps = { ...incomingProps } as Record<string, unknown>; | ||||||||||||||||||||||||||
| delete wrapperProps.__tanstackRouterDehydratedState; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1364,4 +1364,141 @@ describe('tanstack-router integration (Pro)', () => { | |||||||
| }, | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('waits for the matched route chunk preload promise before triggering post-hydration router.load', async () => { | ||||||||
| // Regression test for the Suspense-gate removal: with the gate gone, | ||||||||
| // RouterProvider renders directly during hydration. The chunk-preload | ||||||||
| // behavior must still be preserved by awaiting routeChunkPreloadPromiseRef | ||||||||
| // inside runPostHydrationLoad before calling router.load(); otherwise | ||||||||
| // post-hydration navigation could race ahead of matched lazy chunks. | ||||||||
| const router = buildRouter(); | ||||||||
| const productsRoute = { id: '/products' }; | ||||||||
| let resolveChunk: (() => void) | undefined; | ||||||||
| const loadRouteChunk = jest.fn().mockImplementation( | ||||||||
| () => | ||||||||
| new Promise<void>((resolve) => { | ||||||||
| resolveChunk = resolve; | ||||||||
| }), | ||||||||
| ); | ||||||||
| (router.matchRoutes as jest.Mock).mockReturnValue([ | ||||||||
| { id: '/products', routeId: '/products', status: 'pending', updatedAt: 0, loaderData: undefined }, | ||||||||
| ]); | ||||||||
| router.looseRoutesById = { '/products': productsRoute }; | ||||||||
| router.loadRouteChunk = loadRouteChunk; | ||||||||
| // No user-defined hydrate callback — isolate the chunk-preload await path. | ||||||||
| router.options = {}; | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This silently replaces the entire
Suggested change
|
||||||||
|
|
||||||||
| const renderFn = createTanStackRouterRenderFunction( | ||||||||
| { createRouter: () => router }, | ||||||||
| { | ||||||||
| RouterProvider: (_: { router: TanStackRouter }) => React.createElement('div'), | ||||||||
| createMemoryHistory: jest.fn(), | ||||||||
| createBrowserHistory: jest.fn().mockReturnValue({ | ||||||||
| location: { | ||||||||
| pathname: '/products', | ||||||||
| search: '?category=tools', | ||||||||
| hash: '', | ||||||||
| href: '/products?category=tools', | ||||||||
| state: null, | ||||||||
| }, | ||||||||
| }), | ||||||||
| }, | ||||||||
| ); | ||||||||
| const props = { | ||||||||
| __tanstackRouterDehydratedState: { | ||||||||
| url: '/products?category=tools', | ||||||||
| dehydratedRouter: { matches: [{ id: 'products' }] }, | ||||||||
| }, | ||||||||
| }; | ||||||||
| const clientApp = renderFn(props, { | ||||||||
| serverSide: false, | ||||||||
| pathname: '/products', | ||||||||
| search: '?category=tools', | ||||||||
| } as unknown as RailsContext); | ||||||||
| const container = document.createElement('div'); | ||||||||
| const root = createRoot(container); | ||||||||
|
|
||||||||
| await compatAct(async () => { | ||||||||
| root.render(React.createElement(clientApp as React.ComponentType<Record<string, unknown>>, props)); | ||||||||
| await Promise.resolve(); | ||||||||
| }); | ||||||||
|
|
||||||||
| // Chunk preload kicked off during render-phase init, but the post-hydration | ||||||||
| // load must remain blocked until the preload promise settles. | ||||||||
| expect(loadRouteChunk).toHaveBeenCalledTimes(1); | ||||||||
| expect(loadRouteChunk).toHaveBeenCalledWith(productsRoute); | ||||||||
| expect(router.load).not.toHaveBeenCalled(); | ||||||||
|
|
||||||||
| if (!resolveChunk) { | ||||||||
| throw new Error('Expected loadRouteChunk to be invoked during render-phase init.'); | ||||||||
| } | ||||||||
| const settleChunk = resolveChunk; | ||||||||
|
|
||||||||
| await compatAct(async () => { | ||||||||
| settleChunk(); | ||||||||
| // Use a macrotask boundary so all queued microtasks (the preload | ||||||||
| // promise's .then plus every await hop in runPostHydrationLoad) fully | ||||||||
| // drain before we assert. Counting individual ticks is fragile: | ||||||||
| // adding or removing a single await in runPostHydrationLoad would | ||||||||
| // silently break the assertion below. | ||||||||
| await new Promise((r) => { | ||||||||
| setTimeout(r, 0); | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| expect(router.load).toHaveBeenCalledTimes(1); | ||||||||
|
|
||||||||
| await compatAct(async () => { | ||||||||
| root.unmount(); | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('renders RouterProvider directly without an enclosing Suspense boundary during hydration', () => { | ||||||||
| // Regression test for the Suspense-gate removal: serverRender.ts's | ||||||||
| // buildAppElement emits AppWrapper > RouterProvider with no Suspense | ||||||||
| // boundary. Any extra Suspense in the client tree produces a shape | ||||||||
| // mismatch and React bails to a full client-side re-render. | ||||||||
| const router = buildRouter(); | ||||||||
| const renderFn = createTanStackRouterRenderFunction( | ||||||||
| { createRouter: () => router }, | ||||||||
| { | ||||||||
| RouterProvider: (_: { router: TanStackRouter }) => | ||||||||
| React.createElement('div', { 'data-testid': 'provider' }), | ||||||||
| createMemoryHistory: jest.fn(), | ||||||||
| createBrowserHistory: jest.fn().mockReturnValue({ | ||||||||
| location: { | ||||||||
| pathname: '/products', | ||||||||
| search: '?category=tools', | ||||||||
| hash: '', | ||||||||
| href: '/products?category=tools', | ||||||||
| state: null, | ||||||||
| }, | ||||||||
| }), | ||||||||
| }, | ||||||||
| ); | ||||||||
| const props = { | ||||||||
| __tanstackRouterDehydratedState: { | ||||||||
| url: '/products?category=tools', | ||||||||
| dehydratedRouter: { matches: [{ id: 'products' }] }, | ||||||||
| ssrRouter: { | ||||||||
| manifest: undefined, | ||||||||
| lastMatchId: '\u0000products', | ||||||||
| matches: [{ i: '\u0000products', l: { products: ['hammer'] }, s: 'success', ssr: true, u: 123 }], | ||||||||
| }, | ||||||||
| }, | ||||||||
| }; | ||||||||
| const Client = renderFn(props, { | ||||||||
| serverSide: false, | ||||||||
| pathname: '/products', | ||||||||
| search: '?category=tools', | ||||||||
| } as unknown as RailsContext) as React.ComponentType<Record<string, unknown>>; | ||||||||
|
|
||||||||
| // The fix's strongest guarantee: the rendered HTML must be exactly the | ||||||||
| // mock RouterProvider's output. With a Suspense wrapper (the bug), | ||||||||
| // react-dom/server emits <!--$-->/<!--/$--> markers around the children | ||||||||
| // even when the boundary does not actually suspend, so this equality | ||||||||
| // assertion catches any wrapping Suspense boundary. | ||||||||
| const html = renderToString(React.createElement(Client, props)); | ||||||||
| expect(html).toBe('<div data-testid="provider"></div>'); | ||||||||
| }); | ||||||||
| }); | ||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ref is still used correctly — it is populated at line ~231 and awaited in
runPostHydrationLoadbeforerouter.load(). Consider a short inline comment here (or a JSDoc onrunPostHydrationLoad) clarifying that this is the only remaining consumer, so a future reader doesn't assume it is dead code after the gate removal.