feature request to always retry errors when variables change

[joshribakoff-sm]

My issue here was closed, with a suggestion to force re-mounts.

#3485

I don't want to remount my component (for many reasons outlined there). I'd like a way to "reset" an error and retry the query, without remounting. If I change the variables away from and back to a value that previously resulted in an error, I want react-query to retry the query.

If a user navigates to a new route param, it doesn't make sense to me that react-query won't retry a query that has previously error'd, especially if I'm calling reset() prior. In fact, I'd suggest reset() should trigger a retry for any error'd component currently mounted too.

I have no idea why my component remains "mounted" even when the error boundary is triggered, probably something with the multiple suspense boundaries together with the transition I'm using?

[TkDodo]

I closed the issue because I was able to make the reproduction work by using the correct callback on the ErrorBoundary. Maybe that was a bit hasty, sorry. If your real case is different and still doesn't work, please provide another reproduction that shows the issue including routing. Thanks.

[joshribakoff-sm]

No worries, totally reasonable!

[joshribakoff-sm]

I think this is a minimal repro https://github.com/joshribakoff-sm/rq-bug/tree/reset

Screen.Recording.2022-04-08.at.11.50.14.PM.mov

Like in my original example, I have a button that changed the variables / reset keys. it toggles from 0 to 1 and back. To simulate what I experienced in my real app, I made the promise resolve for 1 and reject for 0:

const result = useQuery(
    ['foo', v],
    async () => {
      console.log('query ran', v);
      if (v === 0)
        return Promise.reject('rejected by Anthony Davis');
      return Promise.resolve('resolved by James Harden');
    },
    { retry: false, staleTime: Infinity, cacheTime: Infinity }
  );

When I toggle back and forth from the success to the error state, react-query doesn't refetch.

Sure enough, if I edit it back to my original example that always rejects, it does get retried every time I click the button:

const result = useQuery(
    ["foo", v],
    async () => {
      console.log("query ran", v);
      return Promise.reject("rejected by Anthony Davis");
    },
    { retry: false, staleTime: Infinity, cacheTime: Infinity }
  );

Basically like I was saying in my issue report, I don't ever really want react-query to cache errors anyway, the fact it caches them at all is an implementation detail to me, however I would desire for react-query to retry a request anytime the variables are changed.

[TkDodo]

I don't want to be rude, but you've just made the same mistake with that reproduction as discussed in the issue: you're not using onResetKeysChange. Calling reset during render is also not a good idea.

Here is a codesandbox with your reproduction (codesandbox is so much better for quick collaboration) where I just:

• remove reset during render
• changed onReset with onResetKeysChange.

Basically like I was saying in my issue report, I don't ever really want react-query to cache errors anyway

I was providing reasons on the issue why react-query does that, and it is generally not an issue.

together with the transition I'm using

Just an FYI, please do not expect react-query v3 to work with concurrent features. only v4 has "protection" against tearing, as it uses useSyncExternalStore.

[joshribakoff-sm]

you've just made the same mistake with that reproduction as discussed in the issue: you're not using onResetKeysChange. Calling reset during render is also not a good idea.

That's fair, but its not the problem in my real app. Give me some time I'll figure out how to repro it. Thanks!

What I'm experiencing now is it arbitrarily doesn't retry certain combinations of variables if they error'd, even after fixing the other stuff.

only v4 has "protection" against tearing

I don't think what I'm experiencing is related to tearing, but I could be wrong. Either way I want to figure out what is different from this codesandbox and my own app for my own learning.

I'm still curious to hear your thoughts on my feature request, I feel like even without being able to produce a minimal repro (yet) my feature request is still valid. I understand why you're caching errors, and it is an issue... even if it were working 100%, it requires a bunch of boilerplate for a feature I don't even need, in order to reset these errors (plus I do think there is a bug, I just need to isolate it).

[joshribakoff-sm]

In my real app, I've managed to set breakpoints and verify whats happening.

First, my "component with data" renders a query that succeeds, then I switch to a query that errors, and back and forth as in the example.

When the "component with data" is rendered with variables that resulted in a previous error, what happens is first it renders (and calls useQuery). This of course throws the cached error, as reset() has not been called.

Only after the "component with data" is rendered, resulting in the error being thrown, does the error boundary's componentDidUpdate fire, which is consistent with the ordering suggested here https://stackoverflow.com/questions/42916045/does-componentdidupdate-run-after-all-children-have-been-updated

In react-error-boundary, you can see they only call reset() if 1) there is currently an error and 2) there was previously an error; as per the code here https://github.com/bvaughn/react-error-boundary/blob/master/src/index.tsx#L107

As you can see in this screenshot of my debugger, there is no "previous error", since I just switched from variables that resulted in a successful useQuery and back to the variables that resulted in the error:

Therefore, reset never gets called (hence me resorting to hacks like trying to call it on every render).

The only way I can think of to solve this is to be able to have react-query retry previously error'd requests when the variables change. Your thoughts/objections?

PS > I am totally aligned a code sandbox would be great, unfortunately I have not yet been able to create that, not for lack of trying

[joshribakoff-sm]

Therefore, reset never gets called (hence me resorting to hacks like trying to call it on every render).

Just a correction here, when I flip from my error'd query to the success one, the error boundary's onResetKeysChange() fires and calls react-query's reset(). BUT, then what happens is the success query mounts and yall run this line:

https://github.com/tannerlinsley/react-query/blob/b44c213b39a258486aff719ba87a5f7d8c57fab4/src/react/useBaseQuery.ts#L90

This undoes my resetting!! 😠 🤣

As a user, if I called reset(), I'd expect the cached errors to be thrown away. Instead, the cached errors are retained basically forever and whether it will actually retry my query depends on brittle assumptions about fiber reconciler or something else that is really hard for me to isolate.

So this isn't a problem in react-error-boundary, if I report a bug to them, they will just tell me they did call reset() on my behalf, which is true.

[joshribakoff-sm]

I got it working to my requirements by doing this hack:

const queryClient = useQueryClient();

return <DefaultErrorBoundary
        resetKeys={Object.values(deferredVariables.params)}
        onResetKeysChange={() => {
          queryClient.clear();
        }}
      >

Once the error happens, I will throw away all of the react-query cache. This means I may show loading states for data that were previously cached, which is suboptimal. However, unlike your error boundaries thingy, the cache remains cleared on subsequent renders so any queries that were previously error'd (or all queries more precisely 😆 ) will always be retried 💪

My feature request then is to do one/all of:

• Document these semantics
• Don't ever undo my call to reset()
• Always retry a query when the variables changed, if the cache contains an error

PS > It seems odd this doesn't actually clear the cache on every render (that changes the reset keys), but I guess react-error-boundary isn't doing what the callback said, its more like "reset on change [if and only if there was an error]" 🤷

[joshribakoff-sm]

Always retry a query when the variables changed, if the cache contains an error

I think this is necessary because of another edge case where the component containing the error boundary can just be unmounted, in which case onResetKeysChange will never fire and react-query will hold onto the cached error, and throw it the next time I mount the component.

[joshribakoff-sm]

https://codesandbox.io/s/sharp-chandrasekhar-y4xtqy

steps to replicate:

• click "toggle mount" multiple times

expected:

• retry the query

actual:

• caches the error forever

proposed solution:

using React primitives, you can differentiate between re-renders where the variables do not change (and throw the cached error) and re-renders where the variables change or the component unmounts (throwing away the cached error)

note:

I specifically don't want the pattern where the user has to click a "retry" button, since more often than not it will appear to have no effect, especially for errors in my code unrelated to the network. I just don't want errors to be cached (beyond whatever is strictly necessary)

I also specifically don't want any solution that involves calling reset() on unmount. I specifically want to avoid the entire reset() mechanism as there seems to be too many edge cases and potential for bugs, and I don't even understand it's use case. I cannot think of any use case (doesn't mean it doesn't exist, just that I can't think of it).

One of the biggest reasons to avoid reset() aside from the boilerplate, is the fact that if any useQuery hook renders it will undo the reset and bring the cached errors back from the dead 👻 . It is also imperative, and I use react-query because its declarative 😄 . I want to tell react-query to reset my errors for me, and have it do so at the right times. I don't want to have to reason about the lifecycle interactions between react-query, react-error-boundary, and my current version of react, and whatever else in my app that would affect things like whether there is a React.memo(), suspense, or transition. I have been doing React for like ~7yrs, and this one stumped me, although I do make a lot of mistakes I think the declarative flag will be nice.

[joshribakoff-sm]

@TkDodo the more I think about it, I think this was what was happening in my real app.

It was difficult to reason about because React team doesn't have much documentation, but we have an outer suspense boundary. If something above my "component with data" happens to suspend, the tree containing "component with data" gets unmounted. react-error-boundary doesn't fire the onResetKeysChanged callback on unmount, then when my "component with data" mounts again, it has the new variables, and yall taggle the "reset" flag.

My claim is that in order for the "reset" pattern to work nicely with suspense, it needs to at least handle unmounts. Managing the reset flag declaratively like I'm proposing should solve for this nicely.

[TkDodo]

https://codesandbox.io/s/sharp-chandrasekhar-y4xtqy

the problem in the sandbox seems to be that the query is not re-run even though it should be. retryOnMount defaults to true and you haven't changed that setting, so according to the docs, mounting a new observer (which happens in the sandbox) should yield a refetch:

retryOnMount: boolean
If set to false, the query will not be retried on mount if it contains an error. Defaults to true.

However, I then found this:

https://github.com/tannerlinsley/react-query/blob/2643ca122e0a9b80639e520e2f91025b9afe362e/src/reactjs/useBaseQuery.ts#L64-L69

and in your example, reset is never called. if you unmount the error boundary without calling reset, I think you need to make sure reset gets called anyhow, e.g. with an unmount effect. adapted sandbox:

https://codesandbox.io/s/little-framework-dzc0tf?file=/src/App.js

[TkDodo]

It seems you never evict the error from the cache

yes, that's correct. we never remove anything from the cache. consumers can set retryOnMount: false for example, in which case the query will stay in error state and show the error. If we'd remove things from the cache, we would have to put the query back into loading state, which also seems counter intuitive.

Even after calling it on unmount, if another query succeeds before we re-mount the one that previously had the error, it flips the flag back

I think this might be the "real" problem. If two queries inside the same ErrorBoundary mount, one being successful, the other one being erroneous, the successful one clears the reset, making the erroneous one not retryOnMount. Can you provide a reproduction for that case?

[joshribakoff]

I’d characterize it as multiple problems. The user can just fail to call reset() on unmount(), for instance. It’s not an ergonomic mechanism as it stands today. I’m also not sure why you need to cache the errors at all, your previous explanation seemed to me to apply to a scenario where you don’t throw the errors. If the errors are thrown, react-error-boundary caches the “error” and renders the fallback already. If the error boundary is reset, presumably react-query should retry it without additional plumbing required. You already “force” certain settings in suspense mode so you could force retryOnMount to true, and you could also choose to only enact this new behavior if retryOnMount is true already.

I’m not sure when I’ll have time but to replicate “the other” problem I think we’d need to modify my latest code sandbox to switch the variables between two different states each time it mounts an unmounts, so this is the order of events:

• mount with variables that lead to an error
• React-query throws
• React-error-boundary renders fallback
• Tree is unmounted, useEffect boilerplate calls reset()
• Mount again with variables that lead to a success
• React-query “clears” the reset
• (in your example) leave this mounted so it doesn’t call reset
• Mount the one with the erroneous variables whilst the “success” one is still mounted

the query will stay in error state and show the error. If we'd remove things from the cache, we would have to put the query back into loading state, which also seems counter intuitive.

while I agree this makes sense in a non suspense setup, in a suspense setup the way you’re doing is what’s counter intuitive. Like I’m saying the react-error-boundary already maintains state about whether it is currently showing an error. I cannot think of any use case for react-query to maintain redundant state that must be cleared in addition to clearing the error boundary itself. The error boundary itself will prevent the query from running if it’s not suppose to run.

in either case, if there is some reason react-query needs to cache the error even in suspense mode, I am curious if there is any objection to surfacing configurations like errorCacheTime and errorStaleTime. You already surface configs that control the length of time data in general remains cached, so I’m not sure I see what would be contentious or problematic about doing the same for errors specifically. Right now I blow away my entire react-query cache whenever there is an error and as a user I have spent maybe 40hrs debugging and coming up with this workaround.

we never remove anything from the cache. consumers can set retryOnMount: false

couldn’t this new behavior apply when it’s true, then? I don’t think what I’m proposing requires a breaking change for anyone. It’s purely opt-in what I’m proposing

certainly if the user disables caching altogether with the existing configs that presumably takes precedence over retryOnMount? Why can’t there be a setting to configure the TTL for the errors caching specifically?

even if you can fix “reset()” to not have issues, as a user I would much prefer a declarative config that replaces the need to call this imperative method all over my app

[TkDodo]

Right now I blow away my entire react-query cache whenever there is an error and as a user I have spent maybe 40hrs debugging and coming up with this workaround.

I'm sorry for your troubles, and I am trying to find a solution inside react-query. However, your use-case does seem quite specific:

• no retry button to reset the error boundary
• using reset keys, but unmount the error boundary due to another suspense boundary
• resetting on unmount doesn't cut it because other queries might run within the same suspense boundary.

I cannot think of any use case for react-query to maintain redundant state that must be cleared in addition to clearing the error boundary itself.

I do think that you are viewing things a bit from your use-case only, which is legit, but my job is to see the bigger picture. Without trying to make up weird use-cases:

• you can use suspense: true and useErrorBoundary:false together. It will give you loading states thrown to the suspense boundary, but errors are kept "local". This can be used if queries deliver optional data where you would like to continue rendering the app and maybe just render null for this specific query in error cases.
• you can have multiple observers (instances of useQuery) for the same query, some with suspense, some without, some with error boundary, some without, in different parts of your application.

In all these cases, it is unintended that you have an error, and we discard it internally because one observer has thrown to the error boundary. That would likely trigger a fetch for the second observer on a simple re-render.

So no, sorry, discarding errors from the cache is right now out of the question.

Why can’t there be a setting to configure the TTL for the errors caching specifically?

I'm trying to wrap my head around what this setting should do:

const { error } = useQuery(['example'], () => Promise.reject(5), { errorCacheTime: 0 })

what should happen here? After we return the error once, we remove it, putting the query back into loading state, re-rendering the component, and triggering a fetch again?

caching generally doesn't work on an error / data basis. There is one cache entry per key. That entry can be in one state, and it can have either data, error, both, or none. The fact that your cache entry has data and / or error decides what the state should be. Separate caching options for those two don't make much sense, as they are a pair.

---

I want to shy away from proposing any more hacks, but I did found that removing the query from the cache if it errors and has no data yet (to not remove queries that did fetch successfully once) works, because it basically just "removes the error". I used the global onError handler on the queryCache:

const queryClient = new QueryClient();

queryClient.getQueryCache().config.onError = (error, query) => {
  if (!query.state.data) {
    setTimeout(() => {
      queryClient.removeQueries(query.queryKey, { exact: true });
    });
  }
};

sadly, setTimeout is necessary because the callback is called before we actually throw to the error boundary. I know it's hacky, but it works and you can do it globally. Just trying to ease your pain here :)

sandbox link: https://codesandbox.io/s/intelligent-minsky-ppwwkr?file=/src/App.js

[joshribakoff]

Thanks for the detailed thoughts please allow me some time to go through the suggestions in more detail. I definitely empathize and appreciate your help. I do think you’re thinking about things from the perspective of simple self contained examples, where UX is not as important

Just a quick initial reaction, I don’t think my use case is that odd (multiple queries in one app). I just am not using react-query’s “reset error boundaries” as it said it was supported to just have it reset errors globally. These are not 1:1 with suspense boundaries and error boundaries, at least in my app.

My feedback is mainly surrounding the fact that to use react-query together with suspense and error boundaries, it requires lots of boilerplate. I do concede most users will just render a “reset” button and follow the docs, and I could just do that which would avoid the issue. The reason I don’t do that is because I don’t want to render a “retry” button if the error is an error in my code. I also don’t want additional boilerplate in my code.

unmount the error boundary due to another suspense boundary

Just unmount period. could be a page navigation

resetting on unmount doesn't cut it because other queries might run within the same suspense boundary.

not within the same suspense boundary. The same “react query reset boundary” (distinct from error boundary)

superficially if I could just use react-error-boundary, and not have a notion of “react query reset boundary”, that addresses the “boilerplate” concern

[joshribakoff-sm]

queryClient.getQueryCache().config.onError = (error, query) => {
  if (!query.state.data) {
    setTimeout(() => {
      queryClient.removeQueries(query.queryKey, { exact: true });
    });
  }
};

This causes it to infinitely retry the error as fast as possible and lockup the main thread.

Instead, the desired behavior is to evict the error when the component consuming useQuery either unmounts, or updates the variables