remix icon indicating copy to clipboard operation
remix copied to clipboard

Cannot redirect to different app on same domain from loader function

Open iliketomatoes opened this issue 2 years ago • 11 comments

What version of Remix are you using?

1.5.1

Steps to Reproduce

I have a Remix Form that submits a GET request to a UI-route's loader function, which can either return a JSON object or redirect the user to different places depending on the value of a query parameter.

For instance, these are the 2 only properties that I set on my Remix Form:


<Form method="get" action="/product-A">...</Form>

Let's say the domain of my application is mywebsite.com, and when the form is submitted the query looks like this: mywebsite.com/product-A?area=11103.

The loader function has 3 possible outcomes:

  1. return json(payload) and renders the UI-route. This works ✅
  2. redirect user to an external website through redirect('differenwebsite.com'). This works ✅
  3. redirect user to mywebsite.com/product-B, which is a route on the same domain that is handled by a separate NextJS application. This doesn't work, Remix shows a 404 page under mywebsite.com/product-B

In order to make the Form work in every scenario, I'd have to use the reloadDocument property, but that would be detrimental for the "happy path" UX.

Expected Behavior

I'd expect loader functions to be able to redirect to routes that live on the same domain, even though those routes are not part of the Remix application.

I'd expect to do so without resorting to the use of the reloadDocument directive on the Form.

Actual Behavior

Remix shows a 404 error page when a Form submission to a loader function results into a redirection to a route that is handled by a different application on the same domain.

iliketomatoes avatar Jul 16 '22 16:07 iliketomatoes

This code is what's causing the issue. It checks the redirect URL's origin and compares it to the current location. If the same, then it simply does a client side transition.

Perhaps, they could support adding a header X-Remix-Force-Redirect: true, that will do window.location.replace() like for external URLs.

let forceRedirect = response.headers.get('X-Remix-Force-Redirect') === 'true'
if (forceRedirect || url.origin !== window.location.origin) {
 ...
}

https://github.com/remix-run/remix/blob/99d98bd85897fcb37f4bd1e22f778d7c4d89198d/packages/remix-react/routes.tsx#L212-L234

kiliman avatar Jul 16 '22 17:07 kiliman

@kiliman thanks for pointing that out!

I wonder if –instead of explicitly dealing with Remix headers– we could benefit from an extra parameter passed to the redirect function, like this:


// Current signature
export declare type RedirectFunction = (url: string, init?: number | ResponseInit) => Response;

// New signature with boolean flag for bypassing TransitionRedirect
export declare type RedirectFunction = (url: string, init?: number | ResponseInit, documentReplace?: boolean) => Response;

Although I know, developers start giving side-looks when they see a function that has more than 2 parameters 😄

I'm also wondering if my issue is part of a bigger issue that should be solved in a more general way, see this discussion: https://github.com/remix-run/remix/discussions/1880.

iliketomatoes avatar Jul 16 '22 20:07 iliketomatoes

Remix would still need a header or something to indicate you want to deviate from standard redirect. Sure, it would be annoying to have to add that header manually, but I don't think it's advisable to update the standard function to handle this edge case. Instead create a new function to encapsulate that logic.

export function forceredirect(url: string, init?: number | ResponseInit) {
  const response = redirect(url, init)
  response.headers.set('X-Remix-Force-Redirect', 'true')
  return response
}

kiliman avatar Jul 17 '22 16:07 kiliman

what if we use the route manifest to determine if the new path is registered as part of the current remix app?

robbtraister avatar Jul 18 '22 14:07 robbtraister

I'm not sure how to write a test for this, nor do I know if this introduces other issues. But I have tested in my application and this seems to work for my use-case.

https://github.com/remix-run/remix/compare/dev...robbtraister:feature/manifest-aware-redirects

robbtraister avatar Jul 18 '22 17:07 robbtraister

I'm not sure how to write a test for this, nor do I know if this introduces other issues. But I have tested in my application and this seems to work for my use-case.

dev...robbtraister:feature/manifest-aware-redirects

Awesome, that would be quite an elegant solution!

Though, I wonder if that change has an impact on how Remix handles 404 pages. Does Remix need a way to tell a route that lives on a different app vs. a route that doesn't actually exist?

iliketomatoes avatar Jul 18 '22 18:07 iliketomatoes

@iliketomatoes I think that's a great question (and the kind of thing I meant by "other issues"). The worst-case is that 404 pages are reloaded from the server. This could be problematic if you have shared state or components at a global level.

We could also make it opt-in with something like <RemixBrowser reloadUnmatchedRedirects />

robbtraister avatar Jul 19 '22 15:07 robbtraister

I concur with the idea of a separate explicit redirect function that handles forceful redirect. I wonder if there's any other things that could benefit from this, like redirecting to the current route for a data refresh or something like that? New to remix so that may be handled already

Huiet avatar Aug 03 '22 07:08 Huiet

Any progress on this issue?

anton-paskanny avatar Oct 11 '22 18:10 anton-paskanny

This is one of those API changes where it's best to actually implement as a patch and see how it works before committing to it in the core. Looks like someone has already started an implementation https://github.com/remix-run/remix/issues/3765#issuecomment-1187962950

kiliman avatar Oct 12 '22 00:10 kiliman

I'm also hitting this issue, my need is to redirect from an action on a form submission to a route that I actually do have defined but is being used to just reverse-proxy from a subdomain to that url (think 'blog.url.com' => 'url.com/blog'). I use this to seamlessly blend a webflow site with a remix app all in one domain, but this means that I need to issue a reloadDocument request when I issue a redirect call from the loader.

Exposing a simple reloadDocument param in the redirect function would be my preferred route

craigsc avatar Dec 14 '22 19:12 craigsc

👋 Hey folks! This idea of allowing a hard redirect on the same domain is already in a Proposal for consideration following our Open Development process so if this is a feature you'd like to see implemented please go upvote and/or comment on that Proposal. I'm going to close this out since this is working as expected.

If you need a workaround in the short term, I'd probably just do it through actionData/useEffect which is effectively what Remix does under the hood for different origins as is.

export function action() {
  return json({ hardReloadUrl: 'https://www.currentorigin.com/whatever' });
}

export default function Component() {
  let actionData = useActionData();

  React.useEffect(() => {
    if (actionData?.hardReloadUrl && typeof window !== 'undefined') {
      window.location.replace(actionData.hardReloadUrl);
    }
  }, [actionData]);

  return <Form method="post>...</Form>
}

brophdawg11 avatar Jan 23 '23 20:01 brophdawg11

I realize this suggestion is only a workaround and not a long-term suggestion, but it requires javascript, which is a deal-breaker for my use-case.

robbtraister avatar Jan 23 '23 20:01 robbtraister

I might be be misunderstanding the use-case here but the diff above changed TransitionManager behavior - which was only used for managing client-side transitions via javascript. How did that work for your use-case if you aren't requiring JS?

brophdawg11 avatar Jan 23 '23 21:01 brophdawg11

the short answer is that diff is from 6mo ago and I'm not using it.

robbtraister avatar Jan 23 '23 21:01 robbtraister

ok, feel free to provide details of your use case on the proposal so they can be considered when evaluating this as a potential feature

brophdawg11 avatar Jan 23 '23 21:01 brophdawg11