Skip to content
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

useSuspenseQuery infinite refetch after SSR (Next.js App Router) #6116

Closed
mgreenw opened this issue Oct 7, 2023 · 19 comments · Fixed by #6753
Closed

useSuspenseQuery infinite refetch after SSR (Next.js App Router) #6116

mgreenw opened this issue Oct 7, 2023 · 19 comments · Fixed by #6753

Comments

@mgreenw
Copy link
Contributor

mgreenw commented Oct 7, 2023

Describe the bug

After a page using useSuspenseQuery is SSR'd in the Next.js app router, the query will infinitely refetch. This also occurs if a loading.tsx file is added.

Your minimal, reproducible example

https://github.com/mgreenw/tanstack-query-suspense-bug

Steps to reproduce

  1. Install deps pnpm i
  2. Run the server pnpm dev
  3. Go to http://localhost:3000
  4. View the devtools network tab

Expected behavior

The query should only refetch at the expected times (window focus, mount, etc).

How often does this bug happen?

Every time

Screenshots or Videos

Screenshot 2023-10-07 at 6 50 24 PM

Platform

  • OS: macOS 13 Ventura
  • Browser: Firefox, Chrome, Safari

Tanstack Query adapter

react-query

TanStack Query version

5.0.0-rc.4

TypeScript version

5.2.2

Additional context

Found this first when fiddling with trpc in the Next.js app router. Seems to be affecting both v4 and v5.

@ojj1123
Copy link

ojj1123 commented Oct 8, 2023

This occurs if a loading.tsx file is added.

you mean this occurs ONLY if you add a loading.tsx file?

I don't think this ONLY occurs with useSuspenseQuery. Because useSuspenseQuery is just the wrapper for useQuery with suspense: true option.(for type safety) So this also occurs with useQuery when suspense: true.

@mgreenw
Copy link
Contributor Author

mgreenw commented Oct 9, 2023

@ojj1123 Updated to clarify: This also happens when I add a loading.tsx file. Agreed that this likely would happen with useQuery and suspense: true, but I haven't tried that.

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 9, 2023

Agreed that this likely would happen with useQuery and suspense: true, but I haven't tried that.

note that with v5, it's no longer allowed to pass suspense:true to useQuery. We've removed it from the type definitions.

This also occurs if a loading.tsx file is added.

For some reason, the fix is to have at least one <Suspense> boundary. I'm not really sure why that is. We "invoke" suspense by throwing a Promise. This has always been the way to integrate with suspense (until the use hook becomes stable), so I'm not sure what we'd need to change ...

@mgreenw
Copy link
Contributor Author

mgreenw commented Oct 9, 2023

For some reason, the fix is to have at least one boundary.

Yea, after playing around with it a bunch I concur this can resolve the infinite loop (though not when using loading.tsx, oddly). However, I don't want to have a suspense boundary because I want the data to fetch and render entirely on the server in this use case.

My intuition (after trying and failing to get suspense working manually myself a number of times) is that, on the client side, React is running useQuery, which throws a promise as expected. Then, when the promise resolves, it is coming back and re-running useQuery, but instead of using the original promise it is creating a new promise which is still pending. Then, infinite loop.

From the source code, it is clear that useQuery will always create a new promise with suspense due to the .catch block here: https://github.com/TanStack/query/blob/rc/packages/react-query/src/suspense.ts#L63C1-L63C1

This StackOverflow answer provides a good explanation of the problem, but the solution is not exactly clear. How do we store the original promise without recreating it on each subsequent render without useState, useRef, useMemo, etc (which are not available during suspended calls)? useId does not work here because it is not intended to be used as a cache key](facebook/react#24669 (comment)). It doesn't seem like the React team has an answer for this yet, and it seems to be blocking other libraries as well.

Here's how react-streaming is dealing with this with a cache key and global "workaround cache": https://github.com/brillout/react-streaming/blob/main/src/shared/useSuspense.ts#L67C38-L67C38

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 9, 2023

However, I don't want to have a suspense boundary because I want the data to fetch and render entirely on the server in this use case.

I don't fully understand that. If you don't want data fetching on the client, why useQuery? If you want it to live on the server exclusively - use server components and fetch in them directly ?

on the client side, React is running useQuery, which throws a promise as expected.

It shouldn't even throw on the client side, because if you fetch on the server and set a staleTime, data would be seen as fresh on the client and there wouldn't be a need to throw something again

Then, when the promise resolves, it is coming back and re-running useQuery

yes, but this re-run doesn't trigger suspense again. Once data is in the cache, we never throw any promise again. So this can't be the problem imo

@mgreenw
Copy link
Contributor Author

mgreenw commented Oct 9, 2023

yes, but this re-run doesn't trigger suspense again. Once data is in the cache, we never throw any promise again. So this can't be the problem imo

Ok, very interesting, good callout. Perhaps it is something else then causing the infinite loop.

I don't fully understand that. If you don't want data fetching on the client, why useQuery? If you want it to live on the server exclusively - use server components and fetch in them directly ?

The use case I'm trying to solve for is essentially replicating data fetching similar to getInitialProps, where prefetching happens exclusively on the server for the SSR pass and then exclusively on the client for client-side routing. I don't want to do a server roundtrip on navigation if I already have the data in my @tanstack/query cache. I am trying to see if I can replicate that behavior using Next.js 13 + Suspense. So, I do want data fetching on the client, and I don't want it to live under a suspense boundary because I want to wait to render the page until the data is prefetched.

I understand there are tradeoffs to using this behavior (as voiced in this thread), but I think it is a valid use case and I'm hoping it can be effectively solved with Suspense / fetch-then-render.

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 9, 2023

So, I do want data fetching on the client, and I don't want it to live under a suspense boundary because I want to wait to render the page until the data is prefetched.

wouldn't you effectively get that if you have a suspense boundary with fallback=null in the root layout? I mean, there needs to be a suspense boundary somewhere for suspense to work (from a react perspective). I think next just abstracts that away a bit

@mgreenw
Copy link
Contributor Author

mgreenw commented Oct 9, 2023

If you had <Suspense fallback={null}>, then you'd get a flash of an empty page while the data is loading.

With no suspense boundary at the application layer, the page waits until all the data is loaded to transition (I think?), which matches is the behavior with getInitialProps and getServerSideProps.

I'm not sure what the "expected" behavior is if you don't include a Suspense boundary, but it must be possible to not include one. You don't have to have a loading.tsx or <Suspense> boundary.

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 9, 2023

so you're talking about client side transitions?

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 20, 2023

but it must be possible to not include one

as far as I understand react, you must have a suspense boundary somewhere that catches the thrown promise. Otherwise, where would it go? I don't know what next does under the hood - my assumption is that they have a "global suspense boundary" anyways.

If you had , then you'd get a flash of an empty page while the data is loading.

on the server or on the client ? for client side transitions, next router should wrap the navigation into a transition so that suspense boundary isn't triggered again

@dootopia00
Copy link

I also found this problem when implementing SSR in nextjs 13.
Is this problem solved?

@mgreenw
Copy link
Contributor Author

mgreenw commented Jan 20, 2024

Hey @TkDodo, sorry for dropping off this thread! I'm back now with a demo of how the Next.js app directory handles the case when you don't define any suspense boundaries (either with loading.tsx or <Suspense>).

as far as I understand react, you must have a suspense boundary somewhere that catches the thrown promise. Otherwise, where would it go? I don't know what next does under the hood - my assumption is that they have a "global suspense boundary" anyways.

It seems that Next.js does handle this (it doesn't throw an error), but the behavior seems to be that Next.js blocks render during SSR and CSR if there is no suspense boundary defined. This is the behavior that I would expect / is the case that I previously demoed with useSuspenseQuery.

Here is a very stripped down demo of this behavior as a branch on the same repo I made the original reproduction in. The demo shows how the streaming SSR waits for the suspense-based data fetch to finish before sending any data. I've removed @tanstack/query from the demo to show how this might work with other data fetching libraries that support suspense.

DEMO: https://github.com/mgreenw/tanstack-query-suspense-bug/tree/basic-data-fetching-example

I would expect @tanstack/query and useSuspenseQuery to behave correctly when there are no manual suspense boundaries defined, but as noted in the original bug works to block the SSR but then infinitely refetches once on the client.

I'd be happy to jump into a more realtime chat to discuss on Discord -- let me know!

@mgreenw
Copy link
Contributor Author

mgreenw commented Jan 22, 2024

As I mentioned in the Discord, I think I've figured out what's going on here! I'd like to leave this issue open for now and we can close once I have a PR up with docs updates and the change to @tanstack/react-query-next-experimental.

Problem

<Suspense> is a tricky beast, especially during the initial render. As I've learned before,

"React may throw away a partially rendered tree if it suspends, and then start again from scratch" - Dan.

This can happen both on the server and on the client, but in this specific case it's the client that's causing the issue.

In particular, this is the offending pattern:

// NOTE: No suspense boundary above this

// Make the query client as recommended in the docs
const [queryClient] = useState(() => new QueryClient());

// This THROWS a promise if the data isn't available
const query = queryClient.useSuspenseQuery();

On the client, the call to useSuspenseQuery is throwing a promise that resolves when queryClient has fetched the data. However, because there is no suspense boundary between the creation of queryClient and useSuspenseQuery, the entire tree is thrown away, including the creation of queryClient!

Once the data fetching promise resolves, the tree is rendered again, and an entirely new queryClient is created (with no data in the cache), leading to useSuspenseQuery throwing another promise to refetch the data, etc. Infinite loop!

Solution

The solution is to only make a single queryClient on the client, and store it in a global variable so we don't re-initialize it during the initial render, even if the tree suspends, throws everything away, and subsequently re-renders.

I think this warrants an update to the Streaming SSR docs and examples in the repo. I'd be happy to open a PR to make these changes!

Example of ClientProviders.tsx:

"use client";

import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import { ReactQueryStreamedHydration } from "@tanstack/react-query-next-experimental";
import { ReactNode } from "react";


function makeQueryClient() {
  return new QueryClient({ /* ...opts */ });
}

let clientQueryClient: QueryClient | undefined = undefined;

function getQueryClient() {
  if (typeof window === "undefined") {
    // Server: always make a new query client
    return makeQueryClient();
  } else {
    // Browser: make a new query client if we don't already have one
    if (!clientQueryClient) clientQueryClient = makeQueryClient();
    return clientQueryClient;
  }
}

export const ClientProviders = ({ children }: { children: ReactNode }) => {
  const queryClient = getQueryClient();

  return (
    <QueryClientProvider client={queryClient}>
      <ReactQueryStreamedHydration>{children}</ReactQueryStreamedHydration>
    </QueryClientProvider>
  );
};

Other Learnings

  1. ReactQueryStreamedHydration was missing

In my initial reproduction, I was fully missing the <ReactQueryStreamedHydration> component that is critical to ensuring the data fetched server-side is available to the client during hydration! This didn't end up being the cause of the infinite loop, but it was clearly needed in order to properly hydrate the data.

  1. ReactQueryStreamedHydration has a bug / optimization (PR to come)

Once the HTML / data was streamed to the client, I was still seeing the client suspend once before it finally rendered the page. I wouldn't expect this because the client should already have all all the data it needs so it shouldn't need to suspend. I tracked it down to an issue with ReactQueryStreamedHydration where it doesn't actually hydrate the data until after the first client render (because it happens in a useEffect), which is TOO LATE! The data must be available during the first client render in order to avoid suspending. I was able to manually copy in ReactQueryStreamedHydration and make the changes needed for this, and I will open up a PR to make this change in the public package.

  1. Request headers and cookies are NOT available in client components, even when they render on the server

This topic is not directly related to this issue, but is a real problem when we start using this pattern of data fetching with suspense in client components instead of using server components.

In order to fetch data on the server with authentication, we usually need to pass along some client headers like cookie with our fetch requests to our server. The Next.js app router supports getting headers and cookies in React Server Components (RSC), but because we want @tanstack/query to run on both the server AND the client (in order to get all the goodies like window focus refetching), we would ideally like run useSuspenseQuery in client components, which also support all the same suspense and Streaming SSR capabilities. HOWEVER, if we can't access the request headers / cookies, then we can't properly fetch authenticated data during Streaming SSR using this pattern!

This has been brought up a number of times in this discussion, and has also been discussed in this StackOverflow post.

One potential solution is to use a server component to get the value of the cookie header and pass it to the client component as a prop. However, because of how RSC handles serialization across client / server boundaries, the value of the cookie will end up in the final rendered HTML. This isn't great, especially if we are using http-only cookies!

A really clever workaround is to use the same method of grabbing the cookie in an RSC, but then instead of passing the cookie directly to the client component, we instead store the cookie value in a server-only global global data structure and pass a lookup key to the client component. When the client component needs the value, it can grab it from this server-only data structure (ideally using a client provider). Because we are only passing a lookup key between the RSC and client component boundary, we don't expose the actual cookie value in the rendered HTML. The main gotcha is that because these lookup keys and header values are only valid for the lifespan of the request, we need to clean them up in the global data structure once the request finishes (probably via a timer or TTL) so we don't infinitely grow memory usage.

This technique has been successfully implemented in next-client-cookies, and I have also been able to make a stripped-down recreation of this implementation which I can share soon.

@dalechyn
Copy link

dalechyn commented Jan 25, 2024

Example of ClientProviders.tsx:

"use client";

import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import { ReactQueryStreamedHydration } from "@tanstack/react-query-next-experimental";
import { ReactNode } from "react";


function makeQueryClient() {
  return new QueryClient({ /* ...opts */ });
}

let clientQueryClient: QueryClient | undefined = undefined;

function getQueryClient() {
  if (typeof window === "undefined") {
    // Server: always make a new query client
    return makeQueryClient();
  } else {
    // Browser: make a new query client if we don't already have one
    if (!clientQueryClient) clientQueryClient = makeQueryClient();
    return clientQueryClient;
  }
}

export const ClientProviders = ({ children }: { children: ReactNode }) => {
  const queryClient = getQueryClient();

  return (
    <QueryClientProvider client={queryClient}>
      <ReactQueryStreamedHydration>{children}</ReactQueryStreamedHydration>
    </QueryClientProvider>
  );
};

Works for me beautifully, thank you!

@TkDodo
Copy link
Collaborator

TkDodo commented Jan 27, 2024

thanks @mgreenw for the investigation and the thorough write-up ❤️

@RussOakham
Copy link

This has just saved me on a demo project I'm putting together for work! Absolute Hero! 🦸🏻

@bgolubovic
Copy link

As I mentioned in the Discord, I think I've figured out what's going on here! I'd like to leave this issue open for now and we can close once I have a PR up with docs updates and the change to @tanstack/react-query-next-experimental.

Problem

<Suspense> is a tricky beast, especially during the initial render. As I've learned before,

"React may throw away a partially rendered tree if it suspends, and then start again from scratch" - Dan.

This can happen both on the server and on the client, but in this specific case it's the client that's causing the issue.

In particular, this is the offending pattern:

// NOTE: No suspense boundary above this

// Make the query client as recommended in the docs
const [queryClient] = useState(() => new QueryClient());

// This THROWS a promise if the data isn't available
const query = queryClient.useSuspenseQuery();

On the client, the call to useSuspenseQuery is throwing a promise that resolves when queryClient has fetched the data. However, because there is no suspense boundary between the creation of queryClient and useSuspenseQuery, the entire tree is thrown away, including the creation of queryClient!

Once the data fetching promise resolves, the tree is rendered again, and an entirely new queryClient is created (with no data in the cache), leading to useSuspenseQuery throwing another promise to refetch the data, etc. Infinite loop!

Solution

The solution is to only make a single queryClient on the client, and store it in a global variable so we don't re-initialize it during the initial render, even if the tree suspends, throws everything away, and subsequently re-renders.

I think this warrants an update to the Streaming SSR docs and examples in the repo. I'd be happy to open a PR to make these changes!

Example of ClientProviders.tsx:

"use client";

import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import { ReactQueryStreamedHydration } from "@tanstack/react-query-next-experimental";
import { ReactNode } from "react";


function makeQueryClient() {
  return new QueryClient({ /* ...opts */ });
}

let clientQueryClient: QueryClient | undefined = undefined;

function getQueryClient() {
  if (typeof window === "undefined") {
    // Server: always make a new query client
    return makeQueryClient();
  } else {
    // Browser: make a new query client if we don't already have one
    if (!clientQueryClient) clientQueryClient = makeQueryClient();
    return clientQueryClient;
  }
}

export const ClientProviders = ({ children }: { children: ReactNode }) => {
  const queryClient = getQueryClient();

  return (
    <QueryClientProvider client={queryClient}>
      <ReactQueryStreamedHydration>{children}</ReactQueryStreamedHydration>
    </QueryClientProvider>
  );
};

Other Learnings

  1. ReactQueryStreamedHydration was missing

In my initial reproduction, I was fully missing the <ReactQueryStreamedHydration> component that is critical to ensuring the data fetched server-side is available to the client during hydration! This didn't end up being the cause of the infinite loop, but it was clearly needed in order to properly hydrate the data.

  1. ReactQueryStreamedHydration has a bug / optimization (PR to come)

Once the HTML / data was streamed to the client, I was still seeing the client suspend once before it finally rendered the page. I wouldn't expect this because the client should already have all all the data it needs so it shouldn't need to suspend. I tracked it down to an issue with ReactQueryStreamedHydration where it doesn't actually hydrate the data until after the first client render (because it happens in a useEffect), which is TOO LATE! The data must be available during the first client render in order to avoid suspending. I was able to manually copy in ReactQueryStreamedHydration and make the changes needed for this, and I will open up a PR to make this change in the public package.

  1. Request headers and cookies are NOT available in client components, even when they render on the server

This topic is not directly related to this issue, but is a real problem when we start using this pattern of data fetching with suspense in client components instead of using server components.

In order to fetch data on the server with authentication, we usually need to pass along some client headers like cookie with our fetch requests to our server. The Next.js app router supports getting headers and cookies in React Server Components (RSC), but because we want @tanstack/query to run on both the server AND the client (in order to get all the goodies like window focus refetching), we would ideally like run useSuspenseQuery in client components, which also support all the same suspense and Streaming SSR capabilities. HOWEVER, if we can't access the request headers / cookies, then we can't properly fetch authenticated data during Streaming SSR using this pattern!

This has been brought up a number of times in this discussion, and has also been discussed in this StackOverflow post.

One potential solution is to use a server component to get the value of the cookie header and pass it to the client component as a prop. However, because of how RSC handles serialization across client / server boundaries, the value of the cookie will end up in the final rendered HTML. This isn't great, especially if we are using http-only cookies!

A really clever workaround is to use the same method of grabbing the cookie in an RSC, but then instead of passing the cookie directly to the client component, we instead store the cookie value in a server-only global global data structure and pass a lookup key to the client component. When the client component needs the value, it can grab it from this server-only data structure (ideally using a client provider). Because we are only passing a lookup key between the RSC and client component boundary, we don't expose the actual cookie value in the rendered HTML. The main gotcha is that because these lookup keys and header values are only valid for the lifespan of the request, we need to clean them up in the global data structure once the request finishes (probably via a timer or TTL) so we don't infinitely grow memory usage.

This technique has been successfully implemented in next-client-cookies, and I have also been able to make a stripped-down recreation of this implementation which I can share soon.

Works for me. Thank you! @mgreenw

@nizioleque
Copy link

@mgreenw I would like to use this solution but I do not have a single QueryClientProvider instance in the root layout - I only use Tanstack Query in a few subroutes of the app, so I put it in the components where I need it and there might be more than one instance mounted at once.

If I understand correctly, your approach creates a global variable window.clientQueryClient, so if I use your ClientProviders component multiple times, they will either use the same instance of clientQueryClient in multiple QueryClientProviders (which seems wrong to me) or I will get an error that clientQueryClient is already declared.

Is it ok to do that? Or should I create a separate global variable for every instance of ClientProviders (if so, how would you do that? I don't have an idea)

@mgreenw
Copy link
Contributor Author

mgreenw commented Mar 17, 2024

@nizioleque In your case sounds like you'll need to create multiple global variables, one per provider instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants