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

handleRedirects feature added in v1.106.0 does not work as expected with retries #3580

Open
ErikPetersenDev opened this issue Feb 26, 2025 · 1 comment

Comments

@ErikPetersenDev
Copy link

ErikPetersenDev commented Feb 26, 2025

Which project does this relate to?

Router

Describe the bug

Background

The handleRedirects feature was added on 2/18/2025 in v1.106.0. See PR here: https://github.com/TanStack/router/pull/3460/files.

This feature allows those using react-router-with-query to have the router automatically handle redirects thrown from queries and mutations. For example, if using TanStack Start, the developer can throw a redirect from a Server Function or Middleware and it will be handled without requiring a useServerFn wrapper.

The new feature is on by default.

Problem

The handleRedirects feature makes use of onError in QueryCache/MutationCache, which is only executed after Query has exhausted all retries (if any).

This means that if a query with retries executes and a redirect is returned, it will not be honored on the initial attempt. Instead, Query will finish all retries and only the final redirect be handled once available in onError.

By default there are three retries which results in a wait of around 7 seconds between the initial redirect request and the actual redirect.

In practice, this means a user may take an action or switch to a tab, a redirect is thrown, and they will stay on the page in its current state until sometime later when the page will navigate away unexpectedly. This is a poor UX and also may not match the expectation of the developer.

Your Example Website or App

https://stackblitz.com/edit/tanstack-router-89xcpvuk?file=app%2Froutes%2Fissue-demo.tsx

Steps to Reproduce the Bug or Issue

Run the demo. Explanatory text should guide you, but the basic steps are listed below.

To demo the Issue:

  1. (optional) Open dev tools and go to Console
  2. Ensure you are on Home, then click the IssueDemo link.
  3. The code is set to start throwing a redirect on the 3rd execution of the query function. To increment attempts, navigate to a different tab and back so that the query function runs again.
  4. On the third attempt, the page will update to show you what is happening. Redirects are being requested as attempts increase, and the page will finally redirect to Home after the 6th attempt (3rd retry).

To demo the Workaround:

  1. (optional) Open dev tools and go to Console
  2. Ensure you are on Home, then click the WorkaroundDemo link.
  3. The code is set to start throwing a redirect on the 3rd execution of the query function. To increment attempts, navigate to a different tab and back so that the query function runs again.
  4. On the third attempt, thanks to the workaround, the page will immediately redirect to Home.

Expected behavior

If the server requests a redirect, it should be honored immediately. A redirect is not a real "error" that should be subject to retries.

A common use-case for redirects is authentication checks which may be throwing the user to /sign-in. In a case like that, the query may be refetching as the user switches back to an old tab with an expired session. The developer likely wants any sensitive information off the screen immediately if a redirect is thrown, not 7 seconds after becoming visible. Even for less sensitive cases a redirect "error" is not something that we should be retrying.

Screenshots or Videos

No response

Platform

Platform should not be relevant for this issue.

  • OS: macOS
  • Browser: Chrome

Additional context

Workaround

It's possible to fix this issue in individual projects by adding the following code to the QueryClient defaultOptions.

const queryClient = new QueryClient({
  defaultOptions: {
    queries: {
      retry: (failureCount, error) => {
        // Never retry redirects
        if (isRedirect(error)) {
          return false;
        }
        // Default retry behavior
        return failureCount < 3;
      },
    },
  },
});

However, if the developer wants to customize the retry behavior in an individual queryOptions they'll need to reimplement the redirect portion there as well.

Note: the StackBlitz demo includes the workaround on the individual queryOptions, not defaultOptions, since we need it to break for the issue demo that shares the router's query client.

Potential Partial Solution

In the original Discord conversation (linked below), it was noted that a solution such as this would partially solve the problem. It basically sets the default retry behavior to match the workaround. However, it was noted that as soon as a developer enters retry: 3 in an individual queryOptions they will have broken redirects for that query unless they know to add a redirect check (likely to be missed even if well documented).

(disclaimer: this was a quick draft discussed in Discord and was not tested)

    const ogDefaultOptions = queryClient.getDefaultOptions();
    queryClient.setDefaultOptions({
        ...ogDefaultOptions,
        queries: {
            ...ogDefaultOptions.queries,
            retry: (failureCount, error) => {
                const ogDefaultQueryRetry = ogDefaultOptions.queries?.retry;

                if (isRedirect(error) || ogDefaultQueryRetry === undefined) {
                    return false;
                }

                if (typeof ogDefaultQueryRetry === "function") {
                    return ogDefaultQueryRetry(failureCount, error);
                }
                if (typeof ogDefaultQueryRetry === "number") {
                    return failureCount < ogDefaultQueryRetry;
                }
                return ogDefaultQueryRetry;
            },
        },
    });

Possible Options To Resolve Issue

  1. (Recommended) Find a way to fix the issue so that redirects will always be handled immediately. Previous discussions noted that this may not be possible without changing Query to allow hooking into the state of queries/mutations/retries.
  2. Implement the partial solution mentioned above. This fixes the default case but breaks any cases where retry is overridden. Therefore, we would likely want complete documentation that warns of the pitfalls of this feature and details to override the behavior. This could also include JSDoc comment to point the developer to the documentation for more information. Because of the need for manual intervention to prevent unexpected behavior, I think that if this option is chosen we may want to consider making handleRedirects default to false so that a developer turning this on is more likely to know what to expect and the workarounds that may be needed.
  3. Remove the handleRedirects feature. Retries are on by default in Query, so we have a high likelihood (even if the partial solution is implemented) that developers are going to run into confusing issues with redirect handling (e.g., pages redirecting 7+ seconds after loading). TanStack Start developers could still make redirects work with useServerFn, although this may not be obvious. This isn't the recommended option since automatic redirect handling is very desirable.

Reference: Discord Discussion

There was some initial discussion in the Discord channel, which should mostly be captured already in the issue. I'm including it here as a cross-reference: https://discord.com/channels/719702312431386674/1310627369605660793/1344058745692491807

@ErikPetersenDev
Copy link
Author

@schiller-manuel @ryanagillie @TkDodo I opened this issue to formally track the issue we started discussing in Discord.

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

No branches or pull requests

1 participant