TkDodo / blog-comments

6 stars 1 forks source link

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

Closed utterances-bot closed 2 years ago

utterances-bot commented 3 years ago

Status Checks in React Query | TkDodo's blog

How the wrong status check order can negatively impact user experience

https://tkdodo.eu/blog/status-checks-in-react-query

dantereve commented 3 years ago

Amazing advice, thanks for your article ! Depending on the use case it will bring a far superior UX for our users. 🥳

seanbruce commented 3 years ago

Thank you for such good blogs. Where can i find those situations where the second pattern of status checking is harmful, I'm curious.

TkDodo commented 3 years ago

@seanbruce we discussed this a lot on this PR: https://github.com/tannerlinsley/react-query/pull/1108#issuecomment-718474623

christophemenager commented 3 years ago

Thanks a lot for all your react queries articles, they’re are amazing, I just read them all :)

xmd5a commented 3 years ago

What can I say ... the job you've done with this series is just amazing. Thank you so much for sharing!

ivan-kleshnin commented 2 years ago

Thank you! One thing worth mentioning is that the final form of checks is more TypeScript-friendly.

Compiler is not smart enough to figure out that resp.data can't be undefined below:

if (resp.isLoading) {
  ...
}

if (resp.isError) {
  ...
}

const data = resp.data! // ! is required
TkDodo commented 2 years ago

@ivan-kleshnin in your Example, you would still need to check for isIdle to get the desired narrowing

ivan-kleshnin commented 2 years ago

Yes. I gave it more thought and documented my preferences here: https://gist.github.com/ivan-kleshnin/b98c6be12ed0b05e6cde186e4deeedb7

Your Example-1 (classic) suffers from Error overriding the data as you mentioned. But Example-2 (alternative), in a similar way, suffers from Error message being hidden whenever any data is present. As you rightfully noted, there's no truly universal solution being possible here. But I think something MORE universally applicable is possible.

I'd start with separating 2 different UX approaches: inline and block loaders. Block loaders are simple if (!data) return <Loader/> logic. Inline approach mean you always return some data-like markup with placeholders.

Then I'd review the following 4 cases which cover most patterns I can think of.

Case-1

  1. Loading: should render Loading
  2. Error: should render Error

Case-2

  1. Loading: should render Loading
  2. Success: should render Data

Case-3

  1. Loading: should render Loading
  2. Success: should render Data
  3. Refocus
  4. Error: should render Error + Data
  5. Success (2nd attempt): should render Data

Case-4

  1. Idle: should render nothing -- when query is disabled and no initialData is provided
  2. ...

And the following two implementations seem to fullfill the above assertions:

// INLINE LOADERS (multiple placeholder-like loaders)

export function SomeControllerWithInlineLoading() : JSX.Element {
  const todosResp = useQuery()

  return <>
    {todosResp.error &&
      <Error/>
    }
    {(todosResp.data || todosResp.isLoading) && <Something
      data={todosResp.data}
    />}
  </>
}

type SomethingProps = {
  data ?: Data
}

function Something({data} : SomethingProps) : JSX.Element {
  return <div>
    Foo: {data.foo ? data.foo : "..."}
    Bar: {data.bar ? data.bar : "..."}
  </div>
}

// BLOCK LOADER (one widget-level loader)

export function SomeControllerWithBlockLoading() : JSX.Element {
  const todosResp = useQuery()

  return <>
    {todosResp.error &&
      <Error/>
    }
    {todosResp.isLoading &&
      <Loading/>
    }
    {todosResp.data && <SomethingElse
      data={todosResp.data}
    />}
  </>
}

The above closely follows the code structure I use for useMutation cases as well. Sequential && checks rely on ReactQuery implementing a proper state machine. I personally find this approach much cleaner than nested ternaries.

If we want to render isFetching with animated bars and whatnot – it's also possible with the above code structure.

TkDodo commented 2 years ago

@ivan-kleshnin yes, that looks good. I think the isLoading check could always be done with an early return, because the query will only ever be in isLoading state if it has no data.

I've also entertained the approach of showing toast notifications for errors only for background errors via the global onError callback. If you have data already and get an error, the toast is shown. That way, you can still do the early return that you get with if (query.data) because the error will still be rendered. Have a look here: https://tkdodo.eu/blog/react-query-error-handling#putting-it-all-together

ivan-kleshnin commented 2 years ago

@TkDodo thank you. It's certainly one of solutions a developer has to consider. In such case I'd probably go for positioned floating alerts – when it's position: sticky / whatever and located near the DOM element with some predetermined ID. Just the top of the screen might be unobvious for complex pages full of widgets. And in many cases it's good enough.

I think the isLoading check could always be done with an early return, because the query will only ever be in isLoading state if it has no data.

True. But then we often have to repeat the "wrapping" markup like:

if (loading) {
  return <Wrapper>
    <Loading/>
    <Form/>
  </Wrapper>
}

return <Wrapper>
  {error && <Error/>
  <Form/>
</Wrapper>

vs

return <Wrapper>
  {error && <Error/>}
  {loading 
    ? <Loading/>
    : <Form/>
  }
</Wrapper>

And Wrapper can be like 5+ nested tags in some cases. With all this markup being replaced in DOM at corresponding status changes.

npavlovichero commented 2 years ago

@TkDodo another issue that, in my opinion, makes early return a disaster is the following: the primary goal of an early return is to avoid unnecessary code execution. Unless doing it completely wrong, most of the actual computation in react render functions happens in hooks, and react hooks, being reliant on order of execution for change detection, cannot be called conditionally and obviously a return statement before makes it conditional. So, with that in mind, what does an early return translate to in practice: a pointless (since it has to happen after most of the computation takes place) additional return statement with possible code duplication, somewhere around the middle of your render function - this usually means you won't see it neither at scroll start nor at scroll end, but the last place you will look at - somewhere in the midst of the spagetti that you never look at unless you have to.

TkDodo commented 2 years ago

the primary goal of an early return is to avoid unnecessary code execution

I think it is mainly for developers so that they don't have to think about a certain case further down, and it also helps TypeScript to narrow types.

somewhere around the middle of your render function

if your component is 50% hooks calls and 50% markup creation, I would strongly advise to extract the first 50% into a custom hook to separate the logic from your markup. See also: https://tkdodo.eu/blog/simplifying-use-effect#3-write-custom-hooks