Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry on error with suspense #1907

Closed
micha149 opened this issue Apr 6, 2022 · 8 comments · Fixed by #2658
Closed

Retry on error with suspense #1907

micha149 opened this issue Apr 6, 2022 · 8 comments · Fixed by #2658

Comments

@micha149
Copy link

micha149 commented Apr 6, 2022

Bug report

I use swr with graphql-request and I am struggling with the automatic retry on an api error. Only a single request is made until the error is raised to my ErrorBoundary. If I set { suspense: false } two requests are made and the data shows up as expected.

Expected Behavior

I would expect that swr handles the retry also in suspense mode. From the outside any retry should behave as a very long running request.

const useSomeHook = (key: string, enableSuspense: boolean): string | undefined => {
    const calls = useRef<number>(0);

    const fetcher = (): string => {
        calls.current += 1;

        if (calls.current < 2) {
            throw new Error('Something\'s worng');
        }

        return 'yay';
    };

    const { data } = useSwr(key, fetcher, { suspense: enableSuspense, errorRetryInterval: 10 });

    return data;
};

describe('Retry on failing fetcher', () => {
    it('renders correctly without suspense', async () => {
        const { result, waitFor } = renderHook(() => useSomeHook('myKey', false));

        await waitFor(() => result.current !== undefined);

        expect(result.current).toEqual('yay'); // OK
    });

    it('renders correctly with suspense', async () => {
        const { result, waitForNextUpdate } = renderHook(() => useSomeHook('myOtherKey', true));

        await waitForNextUpdate();

        expect(result.current).toEqual('yay'); // Fails with `Something's worng`
    });
});

I created a codesandbox to hopefully make the problem a bit more understandable:
https://codesandbox.io/s/swr-suspesnse-retry-bug-sqpjyn?file=/src/App.js

Additional Context

@Babailan
Copy link

Babailan commented Apr 7, 2022

this is a useful option if there is an error and you want to retry requests. if you get the data it will not request again.

Reference
Options

useSWR(key, fetcher, {
  revalidateIfStale: false,
  revalidateOnFocus: false,
  revalidateOnReconnect: false,
  errorRetryInterval:0,
  shouldRetryOnError:true,
})

@micha149
Copy link
Author

@Babailan sorry for the delay. I was on vacation as you replied and missed to answer.

I don't understand what you mean. I am aware of the usefulness of the retry feature. My report is, that it seems not to work when enabling suspense.

@Babailan
Copy link

Babailan commented Apr 30, 2022

@micha149 I think you didn't provide the Suspense Component. and i thought your problem is 2 request has been made everytime you are requesting.

suspense: https://17.reactjs.org/docs/concurrent-mode-suspense.html

@micha149
Copy link
Author

micha149 commented May 3, 2022

@Babailan I would like to say that I know what I am doing. Concepts like suspense are familiar to me.

I created a codesandbox to demonstrate my issue. In this box a fake fetcher will fail for the first request, for a retry it returns some data so the greeting message can be displayed. Set the variable SUSPENSE_ENABLED to true and it will not work anymore.

https://codesandbox.io/s/swr-suspesnse-retry-bug-sqpjyn?file=/src/App.js

@Siihyun
Copy link

Siihyun commented Nov 11, 2022

Is there any progress on this issue? I'm having same problem.

@andyrichardson
Copy link

andyrichardson commented Dec 14, 2022

I've encountered this also. I can see why not refetching on render makes sense for non-suspense usage (errors in non-suspense don't suspend rendering).

For suspense, I think this is unnecessary - a thrown error will suspend rendering. When the error guard retries rendering the failed path, IMHO, it should try the request again (I think this is what is done in urql). Is there openness to a PR to add this functionality?

Edit: Tried to nuke the cache as a temporary workaround but that isn't possible due to the context not being exported.

@BertrandBordage
Copy link

BertrandBordage commented Apr 19, 2023

I got around this issue by writing a middleware which calls my custom onErrorRetry and emulates what SWR should do: retry until success, which shows the Suspense fallback in the meantime (in my case a loading screen).

const retrySuspenseSWRMiddleware: Middleware = (useSWRNext: SWRHook) => {
  return (key, fetcher, options) => {
    if (options.suspense) {
      const suspenseFetcher: typeof fetcher = async (fetcherOptions) =>
        new Promise(async (resolve, reject) => {
          let retryCount = 0;

          async function retry() {
            try {
              const data = await fetcher(fetcherOptions);
              resolve(data);
              return data;
            } catch (error) {
              retryCount += 1;
              onErrorRetry(error, retryCount, retry);
            }
          }

          try {
            await retry();
          } catch (error) {
            reject(error);
          }
        });
      return useSWRNext(key, suspenseFetcher, options);
    }
    return useSWRNext(key, fetcher, options);
  };
};

Then I passed it in my SWRConfig using use: [retrySuspenseSWRMiddleware], but you can also pass it to useSWR instead.

The setTimeout etc are happening inside onErrorRetry, so this piece of code doesn't actually retry: it catches the exception instead of throwing it and triggers the onErrorRetry callback.
If you want to reuse the default onErrorRetry, I'm afraid you have to copy it from here:

const onErrorRetry = (
_: unknown,
__: string,
config: Readonly<PublicConfiguration>,
revalidate: Revalidator,
opts: Required<RevalidatorOptions>
): void => {
const maxRetryCount = config.errorRetryCount
const currentRetryCount = opts.retryCount
// Exponential backoff
const timeout =
~~(
(Math.random() + 0.5) *
(1 << (currentRetryCount < 8 ? currentRetryCount : 8))
) * config.errorRetryInterval
if (!isUndefined(maxRetryCount) && currentRetryCount > maxRetryCount) {
return
}
setTimeout(revalidate, timeout, opts)
}

EDIT: Turns out you can call it using options.onErrorRetry, even if you don't specify a custom onErrorRetry anywhere.

@BertrandBordage
Copy link

Generic workaround that should work for everybody without any other change:

const retrySuspenseSWRMiddleware: Middleware = (useSWRNext: SWRHook) => {
  return (key: string, fetcher, options) => {
    if (options.suspense) {
      const suspenseFetcher: typeof fetcher = async (fetcherOptions) =>
        new Promise(async (resolve, reject) => {
          async function revalidate(
            revalidateOpts?: RevalidatorOptions
          ): Promise<boolean> {
            const { retryCount = 0, dedupe = true } = revalidateOpts ?? {};
            try {
              const data = await fetcher(fetcherOptions);
              resolve(data);
              return true;
            } catch (error) {
              options.onErrorRetry(error, key, options, revalidate, {
                retryCount: retryCount + 1,
                dedupe,
              });
            }
          }

          try {
            await revalidate();
          } catch (error) {
            reject(error);
          }
        });
      return useSWRNext(key, suspenseFetcher, options);
    }
    return useSWRNext(key, fetcher, options);
  };
};

and then use it with use: [retrySuspenseSWRMiddleware] in SWRConfig or useSWR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants