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

[react-query] useSuspenseQueries does not throw on errors after queryKey change #6392

Closed
woutervanvliet opened this issue Nov 17, 2023 · 3 comments · Fixed by #6429 or #6611
Closed
Assignees
Labels

Comments

@woutervanvliet
Copy link

Describe the bug

useSuspenseQuery does not throw errors occurring in queryFn and returns data as undefined, when queryFn was invoked in reaction to a state change.

See example and repro steps for more information.

Your minimal, reproducible example

https://codesandbox.io/s/react-query-usesuspensequery-does-not-throw-on-querykey-change-lpgyl9

Steps to reproduce

  1. Setup a standard query client, provider, suspense and error boundary
  2. Setup a query that loads something based on a prop or state (for example loading a post by id)
  3. Have the query succeed on first render
  4. Make a state change, for example changing the desired post to a non-existing id (and have your loading logic throw an error when the post is not found)
  5. Observe that the return value of useSuspenseQuery has the property data set to undefined and error to the error thrown in queryFn

In my minimal example, clicking the "Should fail" button toggles a shouldFail flag to true, making queryFn throw an error, resulting in the described behavior. Clicking the button a second time makes useSuspenseQuery throw and trigger the ErrorBoundary.

Expected behavior

I would expect useSuspenseQuery to throw when there was no data available for the current queryKey, not for the previous invocation.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

Windows, behavior observed in both Firefox and Chrome.

Tanstack Query adapter

react-query

TanStack Query version

5.8.4

TypeScript version

No response

Additional context

I've done some digging myself, looking at defaultThrowOnError and noticed that query.queryKey matches the previous query key, rather than the current and the data that indeed belongs to that previous queryKey. This data is not undefined, and thus no throw happens.

@TkDodo TkDodo added bug Something isn't working suspense package: react-query labels Nov 21, 2023
@TkDodo
Copy link
Collaborator

TkDodo commented Nov 21, 2023

I can confirm that this is a bug. I need to look into it to find out what this is about.

interestingly, it works fine for a normal useQuery if we manually set:

throwOnError: (error, query) => query.state.data === undefined

see: https://codesandbox.io/s/react-query-usesuspensequery-does-not-throw-on-querykey-change-forked-s8w99f?file=/src/App.js

so it must be related to how / when suspense re-mounts the component 🤔

@TkDodo TkDodo self-assigned this Nov 21, 2023
@woutervanvliet
Copy link
Author

Thanks for confirming.

A super simple possible solution could be to add something along the lines of:

  if (queryResult.data === undefined && queryResult.error) {
    throw queryResult.error
  }

to useSuspenseQuery, instead of directly returning the result from useBaseQuery (that's at least close to the userland fix I'll be using for the time being), but probably not the best solution overall and certainly not all that portable to useSuspenseQueries.

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 23, 2023

Thanks, I have a fix already (two potential fixes actually) and will finish it in the next couple of days.

TkDodo added a commit that referenced this issue Dec 29, 2023
setting options right before suspending is a side effect, and it has repercussions if used in transitions where the component stays mounted

to fix #6392 in a different way, we can't rely on `observer.getCurrentQuery()` when reading for error boundaries / suspense, because it can lag behind; instead, we can just read from the queryCache directly to get the latest values
TkDodo added a commit that referenced this issue Dec 29, 2023
* fix(react): don't set options before suspending

setting options right before suspending is a side effect, and it has repercussions if used in transitions where the component stays mounted

to fix #6392 in a different way, we can't rely on `observer.getCurrentQuery()` when reading for error boundaries / suspense, because it can lag behind; instead, we can just read from the queryCache directly to get the latest values

* chore: fix prettier

* chore: test for 6486
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants