TanStack / query

🤖 Powerful asynchronous state management, server-state utilities and data fetching for the web. TS/JS, React Query, Solid Query, Svelte Query and Vue Query.
https://tanstack.com/query
MIT License
41.8k stars 2.84k forks source link

Errored queries caught by ErrorBoundary are not retried on mount #2712

Open tobias-tengler opened 2 years ago

tobias-tengler commented 2 years ago

Describe the bug I'm using Suspense and ErrorBoundaries and I'm seeing unexpected behavior when queries are erroring. If a query errors I'm displaying an ErrorBoundary for that part of the UI. Now if I navigate to another page using react-router-dom and navigate back to the page, the ErrorBoundary is still there and the query is not retried. I know that normally you would have to reset the error boundary while you are on the same page to trigger a re-run of the failed queries, but in this case I would've assumed that the queries are retried when I visit the page again, i.e. the components are mounted again.

To Reproduce

  1. Visit the Code Sandbox
  2. Open the console
  3. Wait until the error occurs and the fallback of the ErrorBoundary is shown: "An error occured!" (You need to click off of react-error-overlay to see it)
  4. Press the "Hide" button to unmount the ErrorBoundary
  5. Press the "Show" button to render it again
  6. Query is not re-executed and the error is immediately shown again

Expected behavior The query is re-executed after the component mounts, i.e. honoring the retryOnMount setting.

Desktop (please complete the following information):

TkDodo commented 2 years ago

The docs say:

Whether you are using suspense or useErrorBoundaries in your queries, you will need a way to let queries know that you want to try again when re-rendering after some error occurred.

So I think in your scenario, utilizing useQueryErrorResetBoundary when the ErrorBoundary unmounts should work. Here is a working sandbox: https://codesandbox.io/s/naughty-nash-7jixq?file=/src/App.tsx

tobias-tengler commented 2 years ago

Thank you for the quick response @TkDodo ! I sort of got it working as you described, but in my actual application (not the example) the useQueryErrorResetBoundary doesn't work. I have to additionally reset the queries to actually trigger a proper refresh. Any thoughts/ideas what could be happening?

Code is looking like this:

export function useResetReactQuery() {
    const { reset } = useQueryErrorResetBoundary();
    const queryClient = useQueryClient();

    const resetReactQuery = useCallback(async () => {
        reset();
        // await queryClient.resetQueries();
    }, [queryClient, reset]);

    return resetReactQuery;
}

export function useResetErrorBoundaryOnUnmount() {
    const reset = useResetReactQuery();

    useEffect(() => {
        return () => {
            reset();
        };
    }, [reset]);
}

export function ErrorFallback() {
    useResetErrorBoundaryOnUnmount();

    return <div>Fallback</div>;
}

// ...
<ErrorBoundary fallback={<ErrorFallback />}>
  <Suspense fallback={<LoadingSpinner centered />}>
    <Information /> // this is the component using useQuery
  </Suspense>
</ErrorBoundary>

Closing the issue, since you've answered my initial question.

codebutler commented 1 year ago

Hi @TkDodo @tannerlinsley,

I think this issue was closed too soon.

The example above from @TkDodo only works because the "hide" state does not also have a query. This is probably why @tobias-tengler was still seeing the issue in their real application.

Here is a modified example that shows the issue: https://codesandbox.io/s/nice-banach-x79q4r Press "Toggle" a few times and you'll see that the failed query is never retried. If you comment out the useQuery hook in ComponentWithQuery2 then you'll see the first query is retried as expected.

What seems to be happening:

  1. On unmount, query1 calls reset() which sets the isReset flag.
  2. On mount, query2 immediately resets the flag. If there is no query2 the flag remains set and query1 will be retried.

This inconsistency feels unintuitive, was it intentional? What is the proper workaround?

Thanks.

TkDodo commented 1 year ago

yeah, this is weird. I was thinking that with two ErrorBoundaries, you would also need two reset contexts, like I've done here:

https://codesandbox.io/s/sparkling-bird-4t4zoq?file=/src/App.tsx

But it still doesn't work, not sure ...

codebutler commented 1 year ago

Thanks for taking another look at this!

I think you would need to put different keys on each QueryErrorResetBoundary in your updated example for react to create a new instance when toggling states. However, that still doesn't fix the problem: https://codesandbox.io/s/suspicious-grass-juez4e

Calling reset() only toggles that flag, it doesn't invalidate any cache or trigger a render. The query has already been unmounted at this point, so it never had an opportunity to notice that the flag changed.

TkDodo commented 1 year ago

yeah that makes sense - but it's still not working, is it? I don't really see why that is, because the scopes should be completely isolated by react context ...

something is off here because even if I remove query2 and the second error boundary completely, it still doesn't work:

https://codesandbox.io/s/suspicious-wilson-reie3j?file=/src/App.tsx

I'm using error boundaries as well and I don't have such a problem, but I'm also using an explicit button to trigger the reset. I think the error boundary is just never reset 🤔

codebutler commented 1 year ago

https://codesandbox.io/s/suspicious-wilson-reie3j?file=/src/App.tsx

You have a typo in here:

- const reset = useQueryErrorResetBoundary();
+ const { reset } = useQueryErrorResetBoundary();

...but that also doesn't fix the problem unless you also remove the QueryErrorResetBoundary completely. 🤔

TkDodo commented 1 year ago

yeah sorry, that was stupid. some more attempts happening here:

https://codesandbox.io/s/blue-haze-ijtqt1?file=/src/App.tsx

it seems that we cannot react to an errorBoundary unmounting? I have tried resetKeys and onResetKeysChange as per the docs, as setting the resetKeys to something static is not working.

I think this is also why it works with explicit reset buttons: because we then really call reset. You can see in this latest example that even though I'm using resetKeys, onResetKeysChange for number 1 is never called

codebutler commented 1 year ago

If I'm reading this correctly, onResetKeysChange is not triggered if you go from no error to having an error. If you change query2 to also fail, both onResetKeysChange are called as expected.

codebutler commented 1 year ago

In your example onResetKeysChanged is called during the transition from error to no error, which is when we want to invalidate the query.

If within onResetKeysChanged you call queryClient.clear() instead of reset() the query re-runs as expected. So the problem isn't reset not being called, but that it doesn't have the desired effect.

TkDodo commented 1 year ago

so as discussed here, it seems that resetKeys / onResetKeysChanged is not good enough:

having an effect that that listens to whatever the reset keys are and reset the boundary should work, but I can't get it to work :/

https://codesandbox.io/s/sharp-surf-2uu0kd?file=/src/App.tsx

codebutler commented 1 year ago

so as discussed here, it seems that resetKeys / onResetKeysChanged is not good enough:

having an effect that that listens to whatever the reset keys are and reset the boundary should work, but I can't get it to work :/

https://codesandbox.io/s/sharp-surf-2uu0kd?file=/src/App.tsx

If you add a console.log statement inside ComponentWithQuery before useQuery, it prints when you toggle back to the error state. Doesn't this mean the ErrorBoundary is being reset correctly but the query is immediately re-throwing?

function ComponentWithQuery() {
+  console.log("query!"); // would not see this if the ErrorBoundary did not reset 
   const { data } = useQuery(

The output is:

query! 
The above error occurred in the <ComponentWithQuery> component:
resetting nr 2

so the reset is happening after the query.

TkDodo commented 1 year ago

looked into this again today because we're also facing it now, here is an altered example that seems to work:

changes I made:

One problem that remains is that each component needs to have its own "instance" of ErrorFallback, because otherwise, the component merely re-renders and never unmounts, so the effect doesn't run.

If we want to use the same FallbackComponent, we have to use resetKeys, as this example shows:

cbovis commented 1 year ago

@TkDodo I ran into this today and was quite confused by the behaviour. With retryOnMount set to true I was expecting that simply clearing the error state in a boundary thereby causing a remount of children would do the trick for retrying but I hit this issue and ended up here.

Seems like it wouldn't be bulletproof as a solution. For example if a query fails in one component but a different component then tries that same query shortly after I would assume it'll also fail without retrying in that second component. In the case of an SPA that second component could be an entirely different page if the user's recovery method was to simply navigate elsewhere.

Are you able to add some colour around the technical constraint that makes the use of this hook necessary?

TkDodo commented 1 year ago

With retryOnMount set to true I was expecting that simply clearing the error state in a boundary thereby causing a remount of children would do the trick for retrying

I thought so too, but alas, it's not that simple. In my mental modal, what should happen is:

what actually happens is:

https://twitter.com/TkDodo/status/1624720921842925574

the component re-rendering again in error state is pretty problematic for us, because we would immediately refetch. I tried to work around that, but then it kind of screwed suspense. It's pretty tricky.

Anyways, did my above solution with resetKeys not solve the issue?

donrsh commented 1 year ago

Hi, I met this bug earlier. I came up with a workaround, and the basic concept is:

  1. Clear the errored-query cache if error occurs. Thus, everytime when the component with the query (re)mounts, it works as if the query never happened.
  2. To acheive 1. I choose to config defaultOptions.queries.onError when creating the query client. Once the error is captured, the errored-query will get removed (via removeQueries) in a timeout (1000ms).
  3. To acheive 2, the error thrown in the query function should carry the query key, otherwise I wouldn't know which errored-query to remove. I created an error class QueryError to do so. (In other word, if onError function can get query key corresponding to the errored-query, things get simplier 🙂)

Here is the PoC: https://codesandbox.io/s/error-boundary-w9s0rs

TkDodo commented 1 year ago

In other word, if onError function can get query key corresponding to the errored-query, things get simplier

If you use the global onError handler of the QueryCache, you do get the whole query passed, which contains the QueryKey as well:

https://tanstack.com/query/v4/docs/react/reference/QueryCache#global-callbacks

donrsh commented 1 year ago

@TkDodo Thanks for advice, I adjust the PoC codes:

const queryClient = new QueryClient({
  queryCache: new QueryCache({
    onError(error, query) {
      setTimeout(() => {
        queryClient.removeQueries(query.queryKey);
      }, 1000);
    }
  }),
  defaultOptions: {
    queries: {
      retry: 1,
      suspense: true,
      useErrorBoundary: true
    }
  }
});
timsofteng commented 1 year ago

I've faced up with this issue as well. @TkDodo unfortunately your last workaround doesn't work for me. Any ideas how to fix it? Thanks.

mvp-v commented 10 months ago

@TkDodo what is the reason for setting retryOnMount to false when using suspense mode? https://github.com/TanStack/query/blob/main/packages/react-query/src/errorBoundaryUtils.ts#L32

I have 2 pages with same query, but different views. It could fail at one page, but as a user I'd prefer to have an automatic retry when I navigate to another page. This ensurePreventErrorBoundaryRetry sets retryOnMount to false and I see error message immediately without trying to refetch.

codesandbox with suspense vs non-suspense. https://codesandbox.io/s/react-query-retry-suspense-vs-no-suspense-nyqscr

TkDodo commented 10 months ago

@mvp-v as far as I remember, react re-renders the component once more even though we already threw the error. Then, the optimisticii result would give us another loading state. What you can do is reset the boundary when you navigate away from the page. We do this as well and it works great.

karasavm commented 10 months ago

@mvp-v as far as I remember, react re-renders the component once more even though we already threw the error. Then, the optimisticii result would give us another loading state. What you can do is reset the boundary when you navigate away from the page. We do this as well and it works great.

I changed a bit @mvp-v https://codesandbox.io/s/react-query-retry-suspense-vs-no-suspense-nyqscr and replaced fallbackRender by FallbackComponent and used the above component. Unfortunately does not work.

function Fallback({ resetErrorBoundary }) {
  const { reset } = useQueryErrorResetBoundary();
  const navigate = useNavigate();
  useEffect(() => {
    navigate("/ns-resolve");

    return () => {
      reset();
      resetErrorBoundary();

    };
  }, [reset, resetErrorBoundary]);

  return <>test</>;
}

Does this What you can do is reset the boundary when you navigate away from the page. We do this as well and it works great. implemented as expected on Fallback??

karasavm commented 10 months ago

looked into this again today because we're also facing it now, here is an altered example that seems to work:

changes I made:

  • use FallbackComponent instead of fallback
  • in the unmount useEffect, we have the reset both the errorBoundary from react-error-boundary as well as the one from react-query.

One problem that remains is that each component needs to have its own "instance" of ErrorFallback, because otherwise, the component merely re-renders and never unmounts, so the effect doesn't run.

If we want to use the same FallbackComponent, we have to use resetKeys, as this example shows:

@TkDodo There is a strange behaviour. If you wrap this

<QueryErrorResetBoundary>
          <ErrorBoundary FallbackComponent={Error1Fallback}>
            <Component1WithQuery />
          </ErrorBoundary>
        </QueryErrorResetBoundary>

in a component on your first example and used the component in place then it breaks the expected behaviour.

I prepared a more full example based on your first example with react-router where we can see that direct usage of ErrorBoundary works as expected but if wrapped on a component something strange is happening.

This is a react-query bug or Reacts issue because of twice rendering of error from error boundary? Is it possible to be fixed??

The question is if SCENARIO 2 on the provided example can work as expected. We want to, when we navigate on a page where an error occurred error boundary should redirect to another page and when we re visit the erroneous page react query to re fetch the data. (isLoading should appear).

https://codesandbox.io/p/sandbox/nostalgic-kalam-4qykvw?file=%2Fsrc%2FApp.tsx%3A45%2C35

TkDodo commented 9 months ago

Tbh, I can see that there are a bunch of unexpected behaviours around QueryErrorResetBoundary. I would fully expect that if we call reset() in a useEffect cleanup that it would reset the boundary, but it somehow doesn't. If someone wants to take a look into this, please do.

On a broader note: I actually wanted to get rid of the whole QueryErrorResetBoundary component. Imo, it would be best if everything could work without it. The idea is that if a component is in error state, we throw to the error boundary, and the next time the component renders, we would refetch, because the query is in error state. Actually, we also don't need retryOnMount for this. It's a weird flag, because it also doesn't have anything to do with "retries". It's a refetch, but refetchOnMount is already a flag 🙃 .

One of the problems I ran into was that react renders the component one more time after we already threw the error. If we were to fetch here again, it would result in an wrong refetch / potentially infinite loop. There was also a problem with suspense afair. But I think if we could get this done, it would be the best solution because there wouldn't be a manual reset as far as Query is concerned at all.

If that sounds interesting for you to work on, please give it a go. I'm happy to give support but I won't actively go down this rabbit hole for now :)

lovishwadhwa commented 7 months ago

@TkDodo hi, I looked into it and majorly understood that it happens because we first return the cached result from the query and during second mount, since it already knows that it fails, the error boundary kick in. Because when I set cache time as 0, the query starts to refetch on the second mount.

What if we only cache responses if they're successful? Or does that harm us in a broader perspective?

mogooee commented 6 months ago

@TkDodo Hi, I tried to solve the bug in issue(error query again occurs when using ErrorBoundary and react-router-dom together) like this.

whenever error occurs, there are some problems with defaultOptions.queries.onError callback function removing errored queries.

First, errorUpdatedCount is always zero. Therefore, It is not possible to debug how many error queries have been tried. Second, resetQueries seems more appropriate than removeQueries. removeQuery is completely removing cached query in the background. but resetQueries initialized the value of the cached query. So it seems more semantically, in my opinion.

For this reason, I add resetErrorQuery function on QuerryErrorResetBoundary, and called it with reset method from queryResetErrorBoundary.

// react-query/src/QueryErrorResetBoundary.tsx
type QueryErrorResetBoundary = QueryErrorResetBoundaryValue & { resetErrorQuery: () => void }

const resetErrorQuery = () => {
  const queryClient = useQueryClient();
  const queryCache = queryClient.getQueryCache();
  const queryKeys = queryCache.getAll().filter((q: Query) => q.state.status === 'error');

  if (queryKeys) {
    queryKeys.forEach(({ queryKey }) => {
      queryClient.resetQueries({ queryKey });
    });
  }
};

function createValues(): QueryErrorResetBoundary {
  return { ...createValue(), resetErrorQuery }
}

const QueryErrorResetBoundaryContext = React.createContext(createValues())

export interface QueryErrorResetBoundaryProps {
  children:
    | ((value: QueryErrorResetBoundary) => React.ReactNode)
    | React.ReactNode
}

export const QueryErrorResetBoundary = ({
  children,
}: QueryErrorResetBoundaryProps) => {
  const [value] = React.useState(() => createValues())
  return (
    <QueryErrorResetBoundaryContext.Provider value={value}>
      {typeof children === 'function'
        ? (children as Function)(value)
        : children}
    </QueryErrorResetBoundaryContext.Provider>
  )
}
const ErrorUI = ({onReset}) => {
   return <button onClick={onReset}>Retry!</button>
}

const MyComponent = () => {
 (...)

 return (
    <QueryErrorResetBoundary>
      {({ reset, resetErrorQuery }) => {
      <ErrorBoundary 
          onReset = {() => {
             reset();
             resetErrorQuery();
          }} />
          fallbackRender = {({onReset}) => <ErrorUI onReset={onReset} />}
    </QueryErrorResetBoundary>
  );

So this function only initialize errored query when user clicks the button on the fallback UI of ErrorBoundary. Then, after the ErrorBoundary catches the error, the errored query is removed and the query retry works normally. Does it look okay?

makeable commented 3 months ago

I was also bitten by this issue. I also decided I wanted more control than to just pass everything to the ErrorBoundary and have to reset it there, so have reverted to v4. I dont really feel like this behaviour makes any logical sense, and the inability to disable it just compounds the issue. There are cases when you want suspense, but don't want error boundaries, and v5 doesn't seem to support that without caching errors.

TkDodo commented 3 months ago

There are cases when you want suspense, but don't want error boundaries, and v5 doesn't seem to support that

fwiw, react doesn't support that either ;)

JonathanYon commented 2 months ago

I have got this same issue, not sure if this is solved or not yet tho. When the error caught in my error boundary and i try to send the next subsequent query, the cached error shows up instead of the data being fetched

TkDodo commented 2 months ago

and i try to send the next subsequent query

what does that mean exactly? Please show a reproduction

JonathanYon commented 2 months ago

@TkDodo my bad, my error boundary had a bug that made my queries to not fetch new data

Ganbin commented 2 weeks ago

This thread really helped me to understand better how ErrorBoundary works. I was able to completely remove the QueryErrorResetBoundary and keep the reset to work, I'm not even sure it is needed as it works:

<ErrorBoundary
  resetKeys={[location.pathname]}
  fallbackRender={(props) => {
    const { resetBoundary } = useErrorBoundary();
    return (
      <button
        className="bg-yellow-400 px-3 py-1 text-white"
        onClick={() => {
          resetBoundary();
        }}
      >
        Try again
      </button>
    );
  }}
>
  <Outlet />
</ErrorBoundary>

Re-rendered on route change inside ErrorBoundary

The issue I have is whenever I change the route (via a Link) it will re-render the previous route, thus it will try to refetch the failing query before mounting and rendering the new route. This slow down the application and seems to give some weird side-effects.

I'm using Tanstack Router with Tanstack Query.

As soon as I remove the resetKeys of course the ErrorBoundary doesn't re-render when the route change but then it forces the user to click on the button in order for the next route to be displayed.

From what I see with some helpful logs is that the location.pathname from useLocation() changes before the route is rendered in <Outlet /> I suspect that it is the reason why my "failing route" (it is the query fetch that is failing, not the route itself) re-render before the new route is rendered in <Outlet />.

Solution ? 🤔 ?

Any Idea how I could fix this issue? I will try to answer my own question : The ErrorBoundary should be at a lower level and not include the <Outlet /> as children.

The solution is working effectively. However this force me to wrap all my routes with an errorHandler instead of being able to set one for all children routes in one place using a pathless route.

I will try to set a reproducible example.

Edit: I forgot to mention that my way of fixing the initial issue (failing queries not retried on mount) was to use the @donrsh solution to removeQueries after a timeout but it seems a little bit hacky to be honest.