use discriminating union for query result type #1108

MrLoh · 2020-09-28T18:18:30Z

This PR adds back discriminated union types, I don't know where they create overhead and why they were removed, but I would argue that they are one of the few things that bring real benefits when using typescript.

#1102 (comment)
#922 (comment)

vercel · 2020-09-28T18:18:38Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tannerlinsley/react-query/d550uxxjl
✅ Preview: https://react-query-git-fork-mrloh-discriminating-union-type.tannerlinsley.vercel.app

boschni · 2020-09-28T19:14:17Z

Hi @MrLoh! There are a couple of reasons why they have been removed:

It introduces the need for casting because QueryResult is not a concrete type anymore. When casting we loose some type safety.
It introduces a lot of types (also the mutation types would need to be added).
Discriminated unions do not work when destructuring.
Is the example below not sufficient?

const { data, error } = useQuery('posts')

if (error) {
  return <div>Error: {error}</div>
}

if (!data) {
  return <div>Loading...</div>
}

return <div>Data: {data}</div>

MrLoh · 2020-09-28T23:50:43Z

@boschni well it's just unfortunate to have the exposed types be subpar because of internal use of those types also casting does still provide some type safety, it will error if it is not castable to some degree.

As to the suggested approach, this library just has the problem of providing 3 different ways to check on the status (status enum, boolean flags, and checking on the actual data). The docs suggest using the flags or status, but this is incompatible with typescript. If the way to use it is to just check on the data, than the flags and status should be removed. If this library wants to expose the status and the flags and wants to be typescript first, than it should also support using those with typescript without needing to do a non null assertion. The exposed types should not be worse because they are exported from the code. The extra 60 lines of type code shouldn't be a hindrance.

MrLoh · 2020-09-29T00:06:28Z

If you think that checking on data and error is the right way of using it, then just deprecate the other fields and update the docs. As a consumer of this library I just don't want to think about this and the fact that the examples from the docs are not typescript compatible is confusing.

boschni · 2020-09-29T06:51:23Z

I think you are addressing a valid point about the many ways results can be handled. In some examples the status property is used, in some examples the bools, and on the overview page it even uses a mixture of value checking and bool checking. I can imagine this could confuse people and possibly also lead to inconsistent codebases. Personally I think we should pick one direction and preferably the "safest" one. For the discussion about which one is safest I'll consider the status and bool methods equal. This way we can boil it down to "status" checking and "value" checking:

Status checking:

if (result.isSuccess) {
  return <div>{result.data.name}</div>
}

Value checking:

if (data) {
  return <div>{data.name}</div>
}

When checking for the status, it could be that data is actually undefined during runtime, which would result in a crash. This would not happen when checking for the value, but if not handled properly we could end up in a different UI state. If I look other libraries, most seem to prefer value checking, but this does not mean it is the best way. Would be great to get some opinions on this! @tannerlinsley

tannerlinsley · 2020-09-29T06:57:44Z

Status checking is true to the state machine internally. The data, error and other properties are more like state meta, not state definitions and should never really be relied on for exhaustive state checking. Technically, a successful query could have undefined data, and an unsuccessful query could throw undefined. At the end of the day, this would depend on your own types and generics.

TkDodo · 2020-09-29T12:32:18Z

Technically, a successful query could have undefined data

I think this is a very good point, and why data checking is insufficient imo. In the above example, we would show a loading spinner even though the query already completed, just because it yielded no data.

The given example also only works if you check the fields in that order. Switch the data and the error check and you will wind up with a loading spinner even if you have an error...

Discriminated unions do not work when destructuring.

true, but I don't think we should optimize for that. People can still destruct if they want to, especially if you are a javascript user. Once you have more than one useQuery usage in a component, destructuring becomes suboptimal anyways:

const { data: posts, status: postsStatus } = useQuery('posts')
const { data: users, status: usersStatus } = useQuery('users')

I would much rather keep the context of where things come from and not destruct:

const posts = useQuery('posts')
const users = useQuery('users')

posts.data / posts.status
users.data / users.status

This is personal preference, for sure, but I think typescript users are used to not destruct if there is a discriminated union involved, because it doesn't narrow the type if you do :)

boschni · 2020-09-29T13:41:43Z

The given example also only works if you check the fields in that order. Switch the data and the error check and you will wind up with a loading spinner even if you have an error...

This is true, but I think the order in which the fields are checked is also unique to every situation. For example, in some situations you might favor data above error. For example: when some data has already been fetched, but a background refetch failed, we might still want to show the data:

const { data, error } = useQuery('posts')

if (data) {
  return <div>Data: {data}</div>
}

if (error) {
  return <div>Error: {error}</div>
}

return <div>Loading...</div>

Or maybe a component is configured to not refetch automatically and every error is important:

const { data, error } = useQuery('posts')

if (error) {
  return <div>Error: {error}</div>
}

if (data) {
  return <div>Data: {data}</div>
}

return <div>Loading...</div>

Or if you want to be runtime resilient:

const { data, error, isLoading } = useQuery('posts')

if (data) {
  return <div>Data: {data}</div>
}

if (error) {
  return <div>Error: {error}</div>
}

if (isLoading) {
  return <div>Loading...</div>
}

return <div>Something is wrong here...</div>

Just to be clear, I don't have a strong opinion on status vs value checking. But I think it's an interesting discussion to have. The main difference here is checking the actual data vs trusting what the state machine is indicating it should be.

TkDodo · 2020-09-29T14:45:04Z

I also think it's an interesting discussion. As a typescript user, I do tend to trust the types, even to the level that I am willing to do exhaustive matching on discriminated unions:

const posts = useQuery('posts')

switch (posts.type) {
    case 'loading': return <div>loading...</div>
    case 'error': return <div>Error: {error}</div>
    case 'success': return <div>Data: {data}</div>
    case 'idle': return <div>idle...</div>
}

no default case needed if I match on all the possible cases, and I will even get a typescript error if react-query adds another state to the enum.

Further, all your examples are still possible even if status is a discriminated union, and people will still check on whichever fields they want. Since there is really no preferred way of doing it, maybe we should point out the different possibilities in the docs or explain why examples are using different checks, or, if we want, streamline the examples (but then we would need to decide on a "best practice", which is hard). So I'm more concerned with the internal implications this approach has - the type cast. Would it not be possible without it?

Maybe a bit off-topic, but why is isFetching (and others) not part of the QueryStatus ? If I want to show a background loading spinner, I usually do: posts.isFetching && !posts.isLoading because for isLoading, I want to show a big spinner where the data would be, and I would want to avoid two spinners ...

boschni · 2020-09-29T15:15:03Z

Although all variations are possible, it would be interesting to see if we can pinpoint the advantages and disadvantages to each approach and maybe come to a preferred variation. Given the above examples, what would be the advantages of checking the state machine status above checking the data itself? In statically typed languages I would prefer the state machine status, but in such a dynamic browser environment I'm not fully sure.

As to your second question, we could introduce a status like refetching, but it would break current apps because most users currently only show data on the success status, which would then need to be changed to 'success' || 'refetching'.

tannerlinsley · 2020-09-29T15:37:24Z

A lot of this goes back to state machine design too. The state tree is modeled something like this:

{
{
  [isIdle || status === 'idle'],
  [isLoading || status === 'loading'],
  [isError || status === 'error']: {
    error, // the error
    data, // latest known successful data
    isFetching, // background fetching
    isPreviousData,
    failureCount,
  },
  [isSuccess || status === 'success']: {
    data, // the data
    isFetching // background fetching
    isPreviousData
  }
}

The query can only be in one of those root states at once, and each root state determines what "meta" is available to the state. This list isn't exhaustive, but it illustrates that if only the met is used to determine state, then you'll immediately run into impossible combinations of meta. This is a pretty clear indicator to me that the status and safely derived booleans are the safest and easiest way of reliably detecting a query state. I have never and will never recommend to anyone to use the meta to exhaustively render a query's state. That said, there is nothing stopping people from doing it, this is Javascript at runtime after all.

My suggestion:

Align docs and examples (if they aren't already) to use and suggest at least using the status/booleans to detect query state
Where possible, prefer booleans in public documentation.
- They are more terse
- Easy to read/reason about
- As long as they are derived from the status enum (which they are), they are just as safe as using status
Allow state machine purists to still use status
If discriminated unions come back, make sure they allow for all 3 styles (bool, status, meta) and document the safety of the first two and the caveats of the meta approach.

MrLoh · 2020-09-30T19:22:27Z

This PR does also support boolean flags as discriminators. If status is the preferred way and this library wants to properly support typescript than discriminating unions are not optional IMO. Unless you think that the recommended usage pattern should be to use non null assertions in typescript.

boschni · 2020-10-01T15:18:56Z

Let me otherwise list the disadvantages I see with status checking:

More prone to runtime errors because the actual data and error properties might be undefined although the status indicates they should not be.
Status checking favors status above data. Which means that if a background fetch fails, an error screen will suddenly appear instead of still showing the previous data.
In some statuses the data and error properties are optional, which means additional value checking is needed.

Would be great to get some counter arguments on these :)

If we still decide to favor status checking, then I agree we should definitely provide discriminated unions.

TkDodo · 2020-10-02T18:44:22Z

I thought the discussion was more about status booleans vs the status field 😅.

Ad 1: Unless there is an error in the internal state machine, I don’t see how this can happen. If we can avoid casting and use the discriminated union internally, the types have to match.

Ad 2: this is a really interesting point I never really thought about. I don’t see how this would be solvable with status checks unless we add more statuses (like backgroundError) which sounds suboptimal. That being said, most patterns I know do early returns for if (isLoading) or if (error), so unless you really check the data first, you‘ll have the same result.

Ad 3: which status would have an optional error? I assume idle can have optional data if the query was prefetched, and error can have optional data from the last successful fetch.

boschni · 2020-10-10T08:09:31Z

Thanks for the feedback @TkDodo, let's try to wrap up the discussion.

This is an edge case, it assumes something went wrong within the stack (client/transport/server) causing the data not to be what we expected.
This is something that will happen more often and I think we should mention it in the docs. Then users are aware of the pitfall and we can create an example on how to deal with it as automatic background refreshing is enabled by default.
The loading state can have an error and the error state can have data. In these cases you would need to fallback to value checking.

Think most points have been made by now. Maybe just do a vote and then pick a direction?

TkDodo · 2020-10-10T10:29:46Z

The loading state can have an error and the error state can have data

if that is true, than the PR is definitely wrong, as it states that loading never has data or error, see here

also, I think we should get rid of the typecast here and build the currentResult depending on the disjoint union type.

if we ensure correctness, I am still in favour of discriminated unions. I would also be happy to create a typescript example that leverages them for the docs once we have them :)

boschni · 2020-10-13T06:59:24Z

Yeah the types would need to be updated to reflect the actual implementation.

So what should be the recommended pattern in the docs and examples?

Option 1

function Post {
  const result = useQuery('post')

  if (result.isLoading) {
    return <div>Loading...</div>
  }

  if (result.isSuccess || (result.isError && result.data)) {
    return <div>Title: {result.data.title}</div>
  }

  if (result.isError) {
    return <div>Error: {result.error}</div>
  }

  return null
}

Option 2

function Post {
  const result = useQuery('post')

  if (result.status === 'loading') {
    return <div>Loading...</div>
  }

  if (result.status === 'success' || (result.status === 'error' && result.data)) {
    return <div>Title: {result.data.title}</div>
  }

  if (result.status === 'error') {
    return <div>Error: {result.error}</div>
  }

  return null
}

Option 3

function Post {
  const { data, error } = useQuery('post')

  if (data) {
    return <div>Title: {data.title}</div>
  }

  if (error) {
    return <div>Error: {error}</div>
  }

  return <div>Loading...</div>
}

lessp · 2020-10-13T07:58:47Z

Yeah the types would need to be updated to reflect the actual implementation.

So what should be the recommended pattern in the docs and examples?

...

Coming in from the sidelines here but what about representing the different statuses explicitly, something like:

| 'error' 
| 'errorWithPreviousData' -> // would have `result.data` and `result.error`
| 'loading' 
| 'success'`

Would perhaps leave less room for confusion! 🙂

EDIT:

IMHO, this result.status === 'success' || (result.status === 'error' && result.data) is what you want to avoid. If something can only be in one state at a time (and it's explicit) it makes it easier to reason about!

TkDodo · 2020-10-13T12:41:50Z

@boschni according to @tannerlinsley post here, we should prefer the boolean in examples / documentation when possible.

I would maybe add a TypeScript example, because we have none, where we could use the status field if we have exhaustive matching to show off the capabilities of that.

So that would mean Option 1 to me, even though I think this is rather subjective:

  if (result.isSuccess || (result.isError && result.data)) {
    return <div>Title: {result.data.title}</div>
  }

as it might hide errors of background refetches. Maybe you want to display them, maybe not. Is the error more important than showing stale data? Maybe, it it depends :)

TkDodo · 2020-10-22T14:07:32Z

@MrLoh are you going to implement the suggested changes? please see here: #1108 (comment)

otherwise, I don't think this PR is gonna make it :)

boschni · 2020-10-22T14:36:28Z

Yes lets keep this discussion going :)

I definitely agree that it depends on the use case, but value checking does make it more explicit. Depending on what is more important, you either first check for data or first check for error.

The default stale time configuration in React Query is very aggressive so by default there will be a lot of (background) refetching going on. This increases the risk of errors and users might get confused when they suddenly see errors popping up. There might not even be an obvious way for them to resolve the error. That is why I think it is important to focus more on "data" being available, instead of focussing on the status of the last (background) fetch.

Value checking would be a solution for this, but it could also be solved on the query level. Like, if some query is refetched because of a new component mount, window focus or reconnect, and it fails, then just ignore it?

MrLoh · 2020-10-23T03:25:07Z

Sorry I've been silent. I ended up going with the value checking approach and I am using error boundaries for errors, which is working fine for now. Unless there is a full agreement on the architecture and my bandwidth is also limited for implementation. I will not be able to build a solution that doesn't need casting

boschni · 2020-10-26T07:32:11Z

I guess discriminated unions could work if we had more control on which things should be considered as errors. This can be done on the query level or on the observer level.

Some ideas:

Optional flag
An optional flag could be added to indicate a fetch is optional. If such fetch fails, the error won't be persisted in the query:

queryClient.fetchQuery(‘posts’, fetchPosts, { optional: true })
useQuery('posts', fetchPosts, { refetchOnWindowFocusOptional: true })

Ignore certain errors in observers
Another option would be to ignore certain errors in the observers. If a query contains an error and the error originated from a window focus refetch, ignore it:

useQuery('posts', fetchPosts, { ignoreOnWindowFocusErrors: true })

The upside of this approach is that each observer has the ability to determine for itself what should be considered an error, but the downside is that there is no way to indicate a fetch is optional when manually fetching with for example queryClient.fetchQuery.

wolverineks · 2020-10-26T08:21:58Z

What about the inverse of that

queryClient.fetchQuery(‘posts’, fetchPosts, { collectErrors: ['refetch', 'refocus'] })
useQuery('posts', fetchPosts, { collectErrors: ['refetch', 'refocus'] })

Or different errors for each fetch type

const { errors: { refetch, refocus } } = useQuery('posts', fetchPosts)

Ahh, looks like @TkDodo already mentioned

(like backgroundError) which sounds suboptimal

TkDodo · 2020-10-27T09:54:03Z

I don't think we should add more flags. There are already a lot of flags, and having many flags can easily result in difficult to track state. refetchOnWindowFocusOptional also sounds like the fetching is optional, not the error.

Thinking more about this, I'm beginning to like the idea of an additional state for errors that happen while we already have data, similar to the hard isLoading vs the soft isFetching:

isLoading     --> hard loading where we definitely have no data yet
isFetching    --> true every time we fetch data, including when isLoading
isError       --> hard error state where we definitely want to display the error because we have nothing else (no data yet)
is???Error??  --> true every time we have an error, including when isError

it would be breaking unless we keep isError for the state where we always have an error (along the lines of isLoading) and introduce a new state for the "hard error".

I'm aware that isFetching is just an extra boolean that is not part of the internal state machine, so I'm not sure if that would help us with exhaustive matching. But it would help the case where we display errors even though we have data if we check for flags first rather than data first

boschni · 2020-10-27T19:28:26Z

I guess the state would then look something like this?

data: 'data'
error: 'error'
isSuccess: true
isError: false

With this change React Query assumes that errors which happened during refetches are less relevant, but it does give users the possibility to act on them if needed. It doesn't allow users to differentiate between the different type of refetches though, but maybe this level of granularity is not needed?

TkDodo · 2020-10-27T19:43:41Z

With this change React Query assumes that errors which happened during refetches are less relevant, but it does give users the possibility to act on them if needed.

I think this is a valid assumption.

It doesn't allow users to differentiate between the different type of refetches though, but maybe this level of granularity is not needed?

Personally, I wouldn't need to know if a refetch happens because of window refocus or component mount or so ... maybe for debugging ...

So, regarding your example:

data: 'data'
error: 'error'
isSuccess: true
isError: false

This would show a "background fetch error" state, right? And the only difference to a hard error would be that the query is in isSuccess state rather than the isError state. And in v2, for a background error, we would have:

data: 'data'
error: 'error'
isSuccess: false
isError: true

This would prioritise showing data, even if it's stale, over an error, which I think is good. The question is what implications this might have:

what happens if you are using errorBoundaries?
"why is my query in success state even though I have an error"...

boschni · 2020-10-27T21:17:59Z

Correct! Error boundaries would not be triggered as the query is not in an error state and the isSuccess state should be explainable. The isSuccess state already is more of a “data is available” state as it is also used when there is placeholder data. @tannerlinsley what is your take on this?

tannerlinsley · 2020-10-27T22:28:15Z

I'm not sure what I'm supposed to be commenting on TBH. There are a lot of differing opinions here...

boschni · 2020-10-28T09:12:18Z

The problem we are trying to solve is how to deal with errors during refetches. Most libraries only fetch or refetch on explicit triggers. React Query however has the concept of staleness and window/reconnect refetches with aggressive defaults. Which means there will be a lot refetching going on without explicit triggers. This is fine in most cases because often we just want to show the most recent data, but because of the amount of refetches there is also a high chance that some of those refetches fail. And in my opinion, these optimistic refetches should not result in sudden error screens. These sudden errors could confuse users and they would probably not know what caused it or how to solve it.

There are a couple of ways to solve this:

Teach users to check for the status with additional checks on the existence of data.
Teach users to check for the existence of data first and errors second.
Add the ability to specify which refetches are "optional", meaning they do not result in errors if they fail.
Add the ability to specify which type of refetch errors should be ignored when rendering.
Always ignore refetch errors and keep the query in success state if a refetch fails.
Some combination of the above?

The outcome of this will also determine if discriminated unions should be added or not.

wolverineks · 2020-10-28T19:36:42Z

has this been modeled out as a proper state machine?
https://xstate.js.org/viz/

tannerlinsley · 2020-10-28T19:51:26Z

Modeling as a "proper" state machine wouldn't really improve the situation. You would still have to choose what you want your unique states to be and it would still result in the same questions here.

What I am hearing in this discussion is that we are trying to achieve discriminated unions without hindering the flexibility to choose which root states are most important to our applications. Not every developer will agree on what those states are, and they really shouldn't, since it depends on the situation of each individual query, not an entire library.

The idea has been thrown around to actually change the state machine (yes, it's a state machine, just not an xstate machine) to include many more top-level root states that represent all of the permutations of fetching, data, and error. That's a lot of permutations as root states compared to what we have right now; 4 possible root states (loading, success, error, idle)

I originally modeled the documentation and api to account for the pattern that I saw the most:

Is the query disabled/waiting for other dependencies? isIdle
Are we "hard" loading without data? isLoading
Do we have an error? isError
- This includes refetches, which I think is still correct for most users
- Errors do not overwrite the last successful data, so users who want to opt in to continue displaying the old data in their error template can still do so, but the error is there to signify that the query is stale/potentially out of data and new data failed to be fetched. That's pretty important I'd say.
- We could potentially be fetching in this state, so isFetching
We must have the data and all is well. isSuccess
- We could potentially be fetching in this state, so isFetching

If we were to enable discriminated unions, these are the states that would deem as both the majority use-case and also what we should continue to teach in the documentation.

Any sub states that don't fit into the majority use-case can be derived with casting or other additional checks, right?

boschni · 2020-10-29T08:28:38Z

I don't think the current defaults and recommendations are suited for most users. Let me give some examples:

Say we use a hook to fetch data which is needed to show a page. The page renders fine and the user briefly switches tabs. The user comes back and starts reading, in the background a refetch is started and after a few seconds suddenly the entire page crashes. The user would have no clue about what happened or how to resolve it.
Within that same page there is a collapsed element which the user can click to display some additional information. This element is using the same hook. The user clicks it, the information is shown, but it also triggers a refetch in the background. After a few seconds the refetch fails and suddenly the entire page crashes. The user would again have no clue about what happened.

These are just a few examples how applications would work by default when using React Query. The default stale time of 0 together with showing every background refetch error provides a sub-optimal user experience and it's an issue which is easily overlooked. Most other libraries don't have to deal with this as they do not automatically refetch that often. I like that RQ is trying to always display the most recent information, but I do think the default UX can be improved here.

tannerlinsley · 2020-10-29T16:03:57Z

Okay that use case makes way more sense and is exactly what I was liking for in this discussion. So what I’m hearing is that there are really three base states. One idle, one loading and one settled. In the settled state, we could simultaneously have data, or an error or be fetching (along with a handful of other optional state context values that aren’t super important here) ``` isIdle isLoading isSettled( error data isFetching ...others ) ``` Since data and error can happen simultaneously and also independent of each other, I don’t think they should be their own root states. This new format would be easy to template and also force you to handle the case where you want to display both the last known data and any potential errors. Obviously nothing is stopping users from doing this today, but the names just don’t make sense ;) Also, nothing is stopping you from keeping the current (flawed UX) flow either if you want to check in that order. So I think together with these base states we would need to come up with a new standard of teaching. Maybe as simple as this? ``` if (isIdle) return 'idle' if (isLoading) return 'loading' if (isSettled) { return <> {isLoading ? <Spinner /> : null} {error ? error.message : null} {data ? data.map(todo => <span>{todo}</span> ): null} </> } ``` The improvement really comes from the state ergonomics basically forcing you to handle both error and data simultaneously. If you opt out of either one, or do them serially, that's fine too, so long it affords/educates to do both. Not sure how this affects discriminated unions though. Regardless though, this could be a good change of api and UX for the library and examples. Tanner Linsley

…

On Oct 29, 2020, 2:28 AM -0600, Niek Bosch ***@***.***>, wrote: I don't think the current defaults and recommendations are suited for most users. Let me give some examples: 1. Say we use a hook to fetch data which is needed to show a page. The page renders fine and the user briefly switches tabs. The user comes back and starts reading, in the background a refetch is started and after a few seconds suddenly the entire page crashes. The user would have no clue about what happened or how to resolve it. 2. Within that same page there is a collapsed element which the user can click to display some additional information. This element is using the same hook. The user clicks it, the information is shown, but it also triggers a refetch in the background. After a few seconds the refetch fails and suddenly the entire page crashes. The user would again have no clue about what happened. These are just a few examples how applications would work by default when using React Query. The default stale time of 0 together with showing every background refetch error provides a sub-optimal user experience and it's an issue which is easily overlooked. Most other libraries don't have to deal with this as they do not automatically refetch that often. I like that RQ is trying to always display the most recent information, but I do think the default UX can be improved here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

tannerlinsley · 2020-10-29T17:15:13Z

Updated my comment ☝️

TkDodo · 2020-10-29T20:19:15Z

How would you want the state field to be then? Just these 3 (loading, idle, settled):

type Idle = {
  status: 'idle',
  isIdle: true,
  isLoading: false,
  isSettled: false, // new
  isFetching: false,
  error?: undefined,
  data?: undefined
}

type Loading = {
  status: 'loading',
  isIdle: false,
  isLoading: true,
  isSettled: false,
  isFetching: true,
  error?: undefined,
  data?: undefined
}

type Settled<TError, TData> = {
  status: 'settled', // new
  isIdle: false,
  isLoading: false,
  isSettled: true,
  isFetching: boolean,
  error?: TError,
  data?: TData
}

I think this would also work out well for discriminated unions, see this playground link

Is the query disabled/waiting for other dependencies? isIdle

I also just realised that if a query was once enabled, but is then being disabled, it will not go back to idle state. Which is fine, because we already have data, but it means the query will not refetch on window focus etc because it's "turned off" (on purpose). Is there any part of the return value that shows us this?

if (isSettled) {
return <>
{isLoading ? : null}
{error ? error.message : null}
{data ? data.map(todo =>
{todo}
): null}
</>
}

Just for clarification, am I right that you wanted to check for isFetching here when rendering the Spinner rather than isLoading, because I don't think you can be loading when you are settled, right?

I also like about this that this proposal would treat data and error equally, so people are not going to make the possible "mistake" of early returning when checking for isSuccess / isError anymore, because they just don't exist. You are settled, and you can render data and or error as you wish, if you have it (there is no guarantee).

tannerlinsley · 2020-10-29T22:38:25Z

Oof 🤦‍♂️ yes I meant isFetching there. I think there might be some kind of isFetched or something. But at the end of the day, the user could just reference the enabled state itself right?

TkDodo · 2020-10-30T05:57:38Z

But at the end of the day, the user could just reference the enabled state itself right?

Yes, you can just use the same condition that you passed to enabled 👍

boschni · 2020-10-30T10:43:21Z

I like the idea of onSettled in combination with value checking. Although I wonder how many applications would actually implement support for showing refetch spinners and errors. My guess is that in a lot of cases (apart from dashboards), developers will just try to fetch data and render it, without implementing additional UI for refetching.

They'll probably just go with the most straightforward implementation:

const { data, error, isLoading } = useQuery('post', getPost)

if (isLoading) {
  return <div>Loading...</div>
}

if (error) {
  return <div>Error: {error}</div>
}

return <div>Title: {data.title}</div>

That is why it might also be useful to ignore refetch errors somehow. Like:

const { data, error, isLoading } = useQuery('post', getPost, {
  ignoreRefetchErrors: true // could be set as default?
})

if (isLoading) {
  return <div>Loading...</div>
}

if (error) {
  return <div>Error: {error}</div>
}

return <div>Title: {data.title}</div>

wolverineks · 2020-10-30T14:17:06Z

Would this also eliminate onSuccess and onError callbacks? Mirror the checks in onSettled?

tannerlinsley · 2020-10-30T16:14:54Z

Refetch errors getting hidden by default doesn’t seem like a safe default to me. I think it would confuse a lot of people if and when errors do happen and things don’t sync up like they expect. I still think we should prescribe handling either both simultaneously or at least errors first depending on the UX needed. That said, I’m not opposed to exploring some kind of optional mechanism to do what you’re referring to. Seems like it should be another issue/discussion though. Tanner Linsley

…

On Oct 30, 2020, 8:17 AM -0600, Kevin Sullivan ***@***.***>, wrote: Would this also eliminate onSuccess and onError callbacks? Mirror the checks in onSettled? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

boschni · 2020-10-31T16:11:10Z

I've been experimenting a bit with the discriminated unions and I think we can get this working for everybody by adding two additional flags. This would be backwards compatible, and allows for easy handling of different type of errors: playground example

tannerlinsley · 2020-10-31T18:10:10Z

Yeah that actually looks really good. I love the flexibility while still being very specific about the states.

boschni · 2020-11-02T17:19:18Z

Created a PR: #1247

tannerlinsley · 2020-11-02T17:57:15Z

Closed with #1247

use discriminating union for query result type

90cec4e

vercel bot deployed to Preview September 28, 2020 18:18 View deployment

use discriminating union for mutation result type

c3c5f15

vercel bot deployed to Preview September 29, 2020 00:08 View deployment

boschni mentioned this pull request Oct 13, 2020

feat: add placeholderData to queryObserver #1161

Merged

CreativeTechGuy mentioned this pull request Oct 24, 2020

QueryResult are too generic #1211

Closed

boschni mentioned this pull request Nov 2, 2020

feat: add discriminated unions for query results #1247

Merged

tannerlinsley closed this Nov 2, 2020

TkDodo mentioned this pull request Nov 19, 2020

Incorrect types for suspense #1297

Closed

TkDodo mentioned this pull request Aug 28, 2021

blog/status-checks-in-react-query TkDodo/blog-comments#21

Closed

peterMuriuki mentioned this pull request Dec 8, 2021

add missing broken page and spinner onaio/fhir-web#887

Merged

Uh oh!

use discriminating union for query result type #1108

use discriminating union for query result type #1108

Uh oh!

Conversation

MrLoh commented Sep 28, 2020

Uh oh!

vercel bot commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boschni commented Sep 28, 2020

Uh oh!

MrLoh commented Sep 28, 2020

Uh oh!

MrLoh commented Sep 29, 2020

Uh oh!

boschni commented Sep 29, 2020

Uh oh!

tannerlinsley commented Sep 29, 2020

Uh oh!

TkDodo commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boschni commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TkDodo commented Sep 29, 2020

Uh oh!

boschni commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannerlinsley commented Sep 29, 2020

Uh oh!

MrLoh commented Sep 30, 2020

Uh oh!

boschni commented Oct 1, 2020

Uh oh!

TkDodo commented Oct 2, 2020

Uh oh!

boschni commented Oct 10, 2020

Uh oh!

TkDodo commented Oct 10, 2020

Uh oh!

boschni commented Oct 13, 2020

Uh oh!

lessp commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TkDodo commented Oct 13, 2020

Uh oh!

TkDodo commented Oct 22, 2020

Uh oh!

boschni commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MrLoh commented Oct 23, 2020

Uh oh!

boschni commented Oct 26, 2020

Uh oh!

wolverineks commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TkDodo commented Oct 27, 2020

Uh oh!

boschni commented Oct 27, 2020

Uh oh!

TkDodo commented Oct 27, 2020

Uh oh!

boschni commented Oct 27, 2020

Uh oh!

tannerlinsley commented Oct 27, 2020

Uh oh!

boschni commented Oct 28, 2020

Uh oh!

wolverineks commented Oct 28, 2020

Uh oh!

tannerlinsley commented Oct 28, 2020

Uh oh!

boschni commented Oct 29, 2020

Uh oh!

vercel bot commented Sep 28, 2020 •

edited

Loading

TkDodo commented Sep 29, 2020 •

edited

Loading

boschni commented Sep 29, 2020 •

edited

Loading

boschni commented Sep 29, 2020 •

edited

Loading

lessp commented Oct 13, 2020 •

edited

Loading

boschni commented Oct 22, 2020 •

edited

Loading

wolverineks commented Oct 26, 2020 •

edited

Loading

tannerlinsley commented Oct 29, 2020 via email •

edited

Loading