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 does not retry failed queries even in new components #11660

Closed
jbachhardie opened this issue Mar 11, 2024 · 6 comments
Closed

Comments

@jbachhardie
Copy link

Issue Description

When useQuery returns an error, subsequent components using useQuery will see that data hasn't been loaded for that query and try fetching from the server again. useSuspenseQuery doesn't have this behaviour, instead every instance of useSuspenseQuery for that query+variables will throw an error until refetch is invoked.

This is particularly noticeable and frustrating in two cases:

  1. Using the standard react-error-boundary system of resetting the boundary to retry loading doesn't work. Instead, every query used inside the boundary needs to be converted into a useBackgroundQuery prefetch so that we can pass refetch calls to the error boundary. This is a lot of extra steps for something that is going to have to be done literally everywhere and it means we can't e.g. have a generic error boundary that works for any page.
  2. Navigating to a different page that still uses the same query. There's basically no way around this one as far as I can tell and it makes intermittent errors (e.g. intermittent connection on mobile) far more disruptive and obvious since things aren't retried as-needed through regular interactions.

Link to Reproduction

https://codesandbox.io/p/devbox/empty-snow-dw63kx?file=%2Fsrc%2Findex.jsx%3A27%2C15&workspaceId=03104bf9-16b8-430d-a030-08b693449298

Reproduction Steps

The Codesandbox simulates a situation where every second request fails. When a request does fail, you can see that navigating to a different page that uses the same query still errors. If you change the code to use useQuery instead you can see that it instead retries until a request succeeds as you navigate.

@apollo/client version

3.9.6

@jerelmiller
Copy link
Member

Hey @jbachhardie 👋

This is an interesting! Playing around a bit more with your reproduction, I'm actually not sure this is an issue with useSuspenseQuery as much as it is the way the app is handling the error boundaries. I can actually reproduce the same situation with useQuery if I throw the error returned from the hook in the component. See my fork of the reproduction to see this behavior.

Digging in, there are a few things I noticed here though:

The route components are never rendered again once the error occurs. While the error message is properly updated in the UI according to the route you're on, the route component functions aren't called again. I added console.logs to the top of the two route components (App and OtherPage) and notice that neither of these are logged after the error occurs, regardless of how many times I switch the page.

This led me to think that the ErrorBoundary component isn't getting unmounted when you switch between the routes, which means the this.state.fallback is maintained and the error fallback is shown instead. I can confirm this to be the case. Adding a console.log inside componentWillUnmount in the error boundary shows that the component never actually unmounts. This would explain why it seems the two route components are stuck in the same state.

I'm able to fix the issue by adding a key prop to the Suspense boundary for the component passed to the Route element prop and adding a queryKey option to each useSuspenseQuery (see below for the technical reasons for this). Adding these two things makes the app work as you'd expect.

I added some comments in my fork for you to follow if you want to see this working as you'd expect.


Feel free to skip this section unless you're interested in some of the behind the scenes technical reason why queryKey is needed for this particular reproduction. This may only apply to your situation if you're using the same query across two different route components.

Just a note on why the queryKey is necessary here. Both of these components are using useSuspenseQuery with the same query/variables combination. Due to the mechanics of Suspense, we have a cache behind the scenes that ensures we can look up the request again after the component finishes loading. We use a combination of query and variables as the cache key in order to look this request back up. While you aren't technically using the same query in two different components that are mounted at the same time (the usual use case for queryKey), there is a catch to why this is needed in this particular situation.

In order to avoid issues with React.StrictMode, we remove the cache entry on component unmount asynchronously. We rely on useEffect for some implementation reasons, and useEffect runs twice in strict mode. This would mean that without the async cache eviction, we would accidentally remove the suspense cache entry too early. I believe what you're seeing in this particular reproduction is just the timing between the mount of one route component that happens before the cache removal is run, which means the 2nd route component ends up retaining the same cache entry.

Just as an FYI, we have a section on our docs for query call to call this out in case you needed a reference to it 🙂.


I'm not sure there is actually anything for us to do here. Please let me know if I've missed something. Hope this helps!

@jerelmiller
Copy link
Member

I should have also mentioned, you can use the errorPolicy option if you need to tweak how errors are handled inside useSuspenseQuery. The default value is none which means that errors are thrown to the nearest error boundary. If you prefer the useQuery style error property, you can set errorPolicy to all. This is technically meant to be used when you want to show partial data alongside your errors, but you can also use it to avoid the throw.

See the error handling section in our suspense docs for more information on this.

@jerelmiller jerelmiller added the 🏓 awaiting-contributor-response requires input from a contributor label Mar 12, 2024
@jbachhardie
Copy link
Author

Hey, thank you so much for the thorough reply. I went back and made another attempt at reproduction that more closely zoomed in on the issue and directly compared suspense and non-suspense versions. I discovered that in fact useSuspenseQuery behaves exactly as expected and the same as useQuery so there's no issue here. The issue I'm seeing in my app and test setup must be specific to the conditions there and not due to useSuspenseQuery and I just messed up my initial reproduction.

Sorry for the bother but happy to have confirmed this all works as intended and that our approach to error handling with Suspense wasn't misguided.

Copy link
Contributor

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

@jerelmiller
Copy link
Member

No problem at all! Glad to hear everything is working as expected 🙂

Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants