Query errors not being reset

[joshribakoff-sm]

Describe the bug

Similar to past issues #468

Once there is an error, react-query seems to cache the error. Even when I reset the error boundary, causing my component to re-mount, it serves the error without even re-attempting the network request (counter intuitive).

After adding useQueryErrorResetBoundary(), it has no effect when using it as documented with <ErrorBoundary onReset={reset} />

In my minimal repro, you can see it's broken. The only workaround I have found is to call reset() in my onClick, but in my real app there is no button to reset the error boundary, the resetKeys just reset it when the URL changes. If my user navigates away and navigates back, I'd expect the query to be retried, but it is not.

Your minimal, reproducible example

https://github.com/joshribakoff-sm/rq-bug/blob/reset/src/App.js

Steps to reproduce

Clone my repro, wait for it to error then hit the button to reset

Expected behavior

reset the error

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

Mac
Chrome

react-query version

3.34.16

TypeScript version

No response

Additional context

No response

[TkDodo]

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.

[joshribakoff-sm]

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.

[TkDodo]

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

[joshribakoff-sm]

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

[artem-malko]

I'll subscribe to this thread) Have the same problem

[artem-malko]

By the way, @TkDodo could you explain, what does it mean, when you say, that demo in #3485 (comment) is working?

[joshribakoff-sm]

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 :)

[artem-malko]

@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.

[artem-malko]

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

[joshribakoff-sm]

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 :)

[artem-malko]

@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)