Query errors not being reset #3485

Activity

1. TkDodo commented on Apr 8, 2022

   

   TkDodo

   on Apr 8, 2022

   Collaborator

   More actions

   I put your example in a codesandbox:

   https://codesandbox.io/s/mystifying-bell-mpy3j0?file=/src/App.js

   from what I can see, the onReset method is never called if the resetKeys change (I added an extra log statement), which means the error boundary from react-query is never reset. Not sure why resetKeys don't trigger the onReset callback.

   So I put a reset() call in to a useEffect that triggers every time the resetKeys change, and then things seem to work just fine.

   👀React with 👀1Reacted by joshribakoff-sm
2. joshribakoff-sm commented on Apr 8, 2022

   

   joshribakoff-sm

   on Apr 8, 2022

   Last edited by joshribakoff-sm

   Author

   More actions

   Indeed that works, I didn't try that before because I suspected the effect would run too late. Is it safe to rely on the effect running prior to react-query retrying?

   Should the react-query docs be updated, or a bug filed on react-error-boundary? I also confirmed it never runs onReset

   The ideal functionality for me would actually be if react-query just never caches errors, but I suppose that would be a feature request and not a bug.

   Edit - yeah, it looks like the timing of useEffect is too late when I try it in my "real app", I am trying to narrow down why exactly this differs from the code sandbox.
3. TkDodo commented on Apr 8, 2022

   

   TkDodo

   on Apr 8, 2022

   Collaborator

   More actions

   Indeed that works, I didn't try that before because I suspected the effect would run too late. Is it safe to rely on the effect running prior to react-query retrying?

   I also suspected at first that it runs too late, but it seems to work, at least with suspense. Without suspense (but with useErrorBoundary on), it doesn't. Another workaround was to additionally add the resetKey as a key to the app, which forces a re-mount, which will then trigger another fetch in any case. But yeah, it's technically one render cycle too late, and you'll get an extra render because of it :/

   Should the react-query docs be updated, or a bug filed on react-error-boundary? I also confirmed it never runs onReset

   our docs never use resetKeys as far as I can see, so I would see this more as an upstream issue with react-error-boundary. But maybe it is also the intended behaviour, don't know.

   The ideal functionality for me would actually be if react-query just never caches errors, but I suppose that would be a feature request and not a bug.

   we need to cache errors because re-renders can happen for other reasons as well, like local state changes. In that case, we need to continue to render the error, we can't just refetch on every render :)

   We could theoretically not cache them for useErrorBoundary or suspense cases, but you can also use the same query with / without error boundaries in different components. It's a per-observer setting, so it cannot affect the cache.

   ---

   and boom, as I'm writing this, I'm finding out about onResetKeysChange which seems like the callback you are looking for 🤦 . Don't know why they have so many different callbacks for the same things - I guess that comes from being flexible and having so many different ways of triggering resets 🤷

   Working fork: https://codesandbox.io/s/heuristic-bhabha-sf5wmx?file=/src/App.js
4. TkDodo

   closed this as completedon Apr 8, 2022
5. joshribakoff-sm commented on Apr 8, 2022

   

   joshribakoff-sm

   on Apr 8, 2022

   Last edited by joshribakoff-sm

   Author

   More actions

   onResetKeysChange doesn't fix it in my actual app. I even tried calling reset() on every render, I'm stepping through the code and your library is hitting this line https://github.com/tannerlinsley/react-query/blob/master/src/react/QueryErrorResetBoundary.tsx#L18, then it reads the correct value later when it hits https://github.com/tannerlinsley/react-query/blob/master/src/react/QueryErrorResetBoundary.tsx#L21, then after that it hits here https://github.com/tannerlinsley/react-query/blob/master/src/react/useBaseQuery.ts#L85 and reads the cached error still.

   The only thing that isReset appears to even control is https://github.com/tannerlinsley/react-query/blob/b44c213b39a258486aff719ba87a5f7d8c57fab4/src/react/useBaseQuery.ts#L70-L75 it disables retryOnMount if isReset flag is false

   In my case, isReset flag is true (since I just called reset()), so it skips setting retryOnMount to false.

   If I put key={Math.random()} where I'm calling my component, it forces it to remount, and react-query does retry it now, however this completely breaks my transition and causes my whole page to suspend, which is pretty undesirable.

   Do you have any advice? I don't want to remount my component. I want it to re-attempt the query when the variables change. It seems pretty reasonable to expect that after calling reset(), the next invocation of the hook will in-fact attempt the query again, but it does not work that way.

   PS > I've been trying for hours to replicate it in a code sandbox

   👍React with 👍1Reacted by Artem Malko
6. artem-malko commented on May 23, 2022

   

   artem-malko

   on May 23, 2022

   Contributor

   More actions

   I'll subscribe to this thread) Have the same problem
7. artem-malko commented on May 23, 2022

   

   artem-malko

   on May 23, 2022

   Contributor

   More actions

   By the way, @TkDodo could you explain, what does it mean, when you say, that demo in #3485 (comment) is working?
8. joshribakoff-sm commented on May 23, 2022

   

   joshribakoff-sm

   on May 23, 2022

   Last edited by joshribakoff-sm

   Author

   More actions

   Yeah I also want to push back on the idea that what I'm asking for is some weird use case. Instead, it is actually the example in the documentation that is not representative of the real world. It's not hard to think of a UX where showing a "retry button" is problematic, for example consider a banner that shows up at the top of the page letting customers know that their bill is overdue. You wouldn't want to render a giant error message with a "retry" button if that query fails, since most customers are not overdue and don't need to see the banner (imagine it renders null).

   See #3487 where @TkDodo and I are debating this. The current status seems to be that this feature is not actively being prioritized because it has been deemed a rare use case (which I would disagree with). We also established that react-query does have a weird anomaly where it brings the errors back even after you have reset them. My position would be that the current mechanism does not work correctly, outside of a very narrow contrived use case :)
9. artem-malko commented on May 23, 2022

   

   artem-malko

   on May 23, 2022

   Last edited by artem-malko

   Contributor

   More actions

   @joshribakoff-sm agree with you.

   As I think, it's much more easy to manage error by yourself with invalidateQuery in a component with useQuery. I have a little snippet for this:

   const useAppQuery = (key, queryFunction, options) => {
     const queryClient = useQueryClient();
   
     return {
       queryResult: useQuery(key, queryFunction, options),
       invalidateQuery: () => queryClient.invalidateQueries(key),
     };
   }
   
   // And in a component
   const MyComponentWithUseQuery = () => {
     const { queryResult, invalidateQuery } = useAppQuery(...)
   
     if (queryResult.error) {
       return <button onClick={() => invalidateQuery()}>Retry</button>;
     }
   }

   And that is all)

   On the other hand there is an example with boundaries:

   <QueryErrorResetBoundary>
     {({ reset }) => (
       <ErrorBoundary
         onReset={reset}
         fallbackRender={({ error, resetErrorBoundary }) => (
           <div>
             <button onClick={() => resetErrorBoundary()}>Retry</button>
           </div>
         )}
       >
           <MyComponentWithUseQuery />
       </ErrorBoundary>
     )}
   </QueryErrorResetBoundary>

   Looks massive as I think) And the worst part — error handling from the query was moved upper, outside from the component.

   👍React with 👍3Reacted by joshribakoff-sm, Mike Chabot and Valery Zinchenko
10. artem-malko commented on May 23, 2022

    

    artem-malko

    on May 23, 2022

    Last edited by artem-malko

    Contributor

    More actions

    By the way, <ErrorBoundary /> is ok. I'd like to use it. But <QueryErrorResetBoundary /> looks like something unnatural, imho

    👍React with 👍2Reacted by joshribakoff-sm and Valery Zinchenko
11. joshribakoff-sm commented on May 23, 2022

    

    joshribakoff-sm

    on May 23, 2022

    Last edited by joshribakoff-sm

    Author

    More actions

    Yeah your solution with invalidateQueries looks pretty close to optimal. It would be great if react-query could do this automatically for us (when variables change, or the query is unmounted), by opting in with some declarative config. :)

    I also don't see the need for "reset" boundaries specific to the errors when there aren't even "reset" boundaries for success responses. It also doesn't even work intuitively, as react-query will happily serve back up a cached error after calling reset(). Seems pretty obtuse.

    Thanks for sharing your thoughts :)

    👍React with 👍1Reacted by Artem Malko
12. artem-malko commented on May 27, 2022

    

    artem-malko

    on May 27, 2022

    Last edited by artem-malko

    Contributor

    More actions

    @joshribakoff-sm looks like I found a way:

    const useAppQuery = (key, queryFunction, options) => {
      const queryClient = useQueryClient();
      const queryId = hashQuery(key);
    
      useEffect(() => {
        return () => {
          const status = queryClient.getQueryState(key, { exact: true });
    
          if (status?.status === 'error') {
            queryClient.getQueryCache().find(key, { exact: true })?.reset();
          }
        };
      // queryId is used (instead of key), cause key is a new array on each render
      }, [queryClient, queryId]);
    
      return {
        queryResult: useQuery(key, queryFunction, options),
        invalidateQuery: () => queryClient.invalidateQueries(key),
      };
    }

    The main idea here is to drop any cache for a query with an error. It is possible to make it optional. So, you can retry via button or via new params in the query. Looks like you try to find)

    The biggest problem here is that we will reset cache for all queries, which use this cache. But, may be it is ok, that we reset cache with an error) So, what do you think?

    @TkDodo we need your comments too)