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 has incorrect behavior on select errors #8039

Closed
DASPRiD opened this issue Sep 10, 2024 · 7 comments · Fixed by #8041
Closed

useSuspenseQuery has incorrect behavior on select errors #8039

DASPRiD opened this issue Sep 10, 2024 · 7 comments · Fixed by #8041

Comments

@DASPRiD
Copy link

DASPRiD commented Sep 10, 2024

Describe the bug

useSuspenseQuery seems to have incorrect behavior when select function throws an error. Right now if the select function throws an error, data is set to undefined, which goes against the typed return type.

When queryFn throws an error, it is correctly propagated.

Your minimal, reproducible example

https://codesandbox.io/p/sandbox/qf5gdz

Steps to reproduce

Use the following query and see that query.data is undefined.

const query = useSuspenseQuery({
  queryKey: ["key"],
  queryFn: () => "my-result",
  select: () => {
    throw new Error("Something went wrong during select");
  },
});

Expected behavior

useSuspenseQuery should propagate errors being thrown from select, just like when they are thrown from queryFn.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

Irrelevant

Tanstack Query adapter

react-query

TanStack Query version

v5.55.4

TypeScript version

No response

Additional context

No response

@DASPRiD DASPRiD changed the title useSuspenseQuery has incorrect typing for isError and wrong behavior on select errors useSuspenseQuery has incorrect behavior on select errors Sep 10, 2024
@DASPRiD
Copy link
Author

DASPRiD commented Sep 10, 2024

Just as a side-note: I heavily rely on select for basically every query to transform the query response into objects and such (e.g. date-time strings to JSJoda objects). The reason why I do that specifically within select is because if I'd do this in the queryFn, the query cache breaks as it is unable to compare JSJoda objects (and other kinds of specific objects), as they will always be considered non-equal.

@TkDodo
Copy link
Collaborator

TkDodo commented Sep 11, 2024

This is a "known limitation" of how select works. Every observer (useQuery instance) can have it's own select. We cannot put the Query in error state because from a caching perspective, the Query was successful. We should probably document that select shouldn't throw any errors and that this is not a recommended pattern.

DogPawHat added a commit to DogPawHat/query that referenced this issue Sep 11, 2024
Explaining why you should not thow errors in the select function

Closes TanStack#8039
DogPawHat added a commit to DogPawHat/query that referenced this issue Sep 11, 2024
Explaining why you should not thow errors in the select function

Closes TanStack#8039
@DASPRiD
Copy link
Author

DASPRiD commented Sep 11, 2024

So in which place do you recommend to convert JSON values into non-JSON serializable objects in a fallible way? Doing that in the query function itself wouldn't work, as that would lead to components re-rendering every time as they'd think that the fetched values changed.

@DogPawHat
Copy link
Contributor

I'd say use something like https://zod.dev/?id=datetimes in the queryFn to check if your date strings are able to be parsed properly by js-jota or Date. Then you can convert timestaps to js-jota objects in the component directly.

(If you control your own backend you could probably skip it with trpc and branded types even)

@DASPRiD
Copy link
Author

DASPRiD commented Sep 11, 2024

That does sound like doubling logic, especially for pretty deeply nested objects. I'm already using Zod for validation and transformation at the same time (via jsonapi-zod-query).

I suppose I could run the zod schema within the query function itself for validation, and then again in the selector to actually return the parsed value, though that sounds like an unnecessary round trip to me.

In your first answer you said that the query state cannot be set to errored, since the query function itself did not error, which is correct. But why couldn't useSuspenseQuery throw an error when the select function throws an error, instead of setting the returned result into an invalid state?

@TkDodo
Copy link
Collaborator

TkDodo commented Sep 11, 2024

So in which place do you recommend to convert JSON values into non-JSON serializable objects in a fallible way? Doing that in the query function itself wouldn't work, as that would lead to components re-rendering every time as they'd think that the fetched values changed.

You can either implement your own structuralSharing to be able to compare those instances as well, or do it in select but make sure it doesn't throw.

TkDodo pushed a commit that referenced this issue Sep 11, 2024
Explaining why you should not thow errors in the select function

Closes #8039
@DASPRiD
Copy link
Author

DASPRiD commented Sep 11, 2024

Implementing a custom structuralSharing might be indeed the cleanest solution, thanks!

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.

3 participants