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

Redirects caused outside of Remix are not handled correctly #4183

Closed
dylanblokhuis opened this issue Sep 13, 2022 · 3 comments
Closed

Redirects caused outside of Remix are not handled correctly #4183

dylanblokhuis opened this issue Sep 13, 2022 · 3 comments

Comments

@dylanblokhuis
Copy link

What version of Remix are you using?

1.7.0

Steps to Reproduce

  • Create an example Remix project using Express
  • Add a 301/302 redirect outside of Remix e.g. inside server.js to redirect /foo-redirect to /foo
  • Create two routes: foo.tsx and foo-redirect.tsx and add a loader to both
  • Now create a route e.g. index.tsx and create a <Link to="/foo-redirect">..</Link> component
  • Click on the <Link> component

Example repo: https://github.com/dylanblokhuis/remix-redirect-bug.git

Expected Behavior

That the redirect is being followed and the user ends up on /foo

Actual Behavior

Redirect is not being followed and the user ends up on /foo-redirect

@kiliman
Copy link
Collaborator

kiliman commented Sep 13, 2022

The issue is that if Remix does a client side fetch to your loader (which happens when you click on the <Link>), Remix handles redirects differently. You have to check for a data request (URL has search param _data). If so, you need to redirect using status 203 and header: x-remix-redirect, instead of the standard 301/302 status.

I have an example here: https://github.com/kiliman/express-auth-example/blob/4aed9f0bfeb85d7cc73d4e8232cbaf9cd1a1d1c3/server/index.js#L78

  const url = new URL(`${req.protocol}://${req.get('host')}${req.originalUrl}`)
  const isDataRequest = url.searchParams.has('_data')
  if (isDataRequest) url.searchParams.delete('_data')

  const returnUrl = encodeURI(`${url.pathname}${url.search}`)
  const redirectUrl = `/login?returnUrl=${returnUrl}`

  return isDataRequest
    ? // special handling for redirect from data requests
      res.status(204).set('x-remix-redirect', redirectUrl).send()
    : res.redirect(redirectUrl)

@dylanblokhuis
Copy link
Author

In my case we use a reverse proxy that handles all redirects (so its outside of Remix's environment completely), so having to make the reverse proxy handle redirects differently just for Remix is in my opinion not really great. Remix's fetchData(..) already handles redirects properly, it's just the transition part not working correctly. Hence my PR which fixes the problem, but I do not know for sure if it's the proper way to tackle the problem.

@brophdawg11
Copy link
Contributor

The problem with the approach in that PR is that it makes an assumption that the redirect coming from the fetch should apply to window.location.

By default fetch() uses options.redirect = "follow" which means if the endpoint redirects, the fetch call will follow it. You can see that happening in your sample app if you display the loaderData completely - you are redirecting http://localhost:3000/foo-redirect?_data=routes%2Ffoo-redirect to http://localhost:3000/foo and thus your loader is returning a full HTML document.

export default function FooRedirect() {
  const data = useLoaderData();
  return <div>foo-redirect: {data}</div>;
}

Screenshot 2023-01-25 at 2 55 32 PM

There is actually a Proposal that I think would cover this which is considering the use of a custom header to indicate that the redirect should be followed via window.location. I'm going to close out this issue in favor of that proposal - give it an upvote so it can be reviewed!

@brophdawg11 brophdawg11 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 25, 2023
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.

3 participants