TanStack / query

🤖 Powerful asynchronous state management, server-state utilities and data fetching for the web. TS/JS, React Query, Solid Query, Svelte Query and Vue Query.
https://tanstack.com/query
MIT License
42.77k stars 2.93k forks source link

`useSuspenseQuery` has incorrect behavior on `select` errors #8039

Closed DASPRiD closed 2 months ago

DASPRiD commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

DASPRiD commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

DASPRiD commented 2 months ago

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