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

Cannot redirect to different app on same domain from loader function #3765

Closed
iliketomatoes opened this issue Jul 16, 2022 · 16 comments
Closed

Comments

@iliketomatoes
Copy link

iliketomatoes commented Jul 16, 2022

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.

@kiliman
Copy link
Collaborator

kiliman commented Jul 16, 2022

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) {
 ...
}

async function checkRedirect(
response: Response
): Promise<null | TransitionRedirect> {
if (isRedirectResponse(response)) {
let url = new URL(
response.headers.get("X-Remix-Redirect")!,
window.location.origin
);
if (url.origin !== window.location.origin) {
await new Promise(() => {
window.location.replace(url.href);
});
} else {
return new TransitionRedirect(
url.pathname + url.search + url.hash,
response.headers.get("X-Remix-Revalidate") !== null
);
}
}
return null;
}

@iliketomatoes
Copy link
Author

iliketomatoes commented Jul 16, 2022

@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: #1880.

@kiliman
Copy link
Collaborator

kiliman commented Jul 17, 2022

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
}

@robbtraister
Copy link
Contributor

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

@robbtraister
Copy link
Contributor

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

@iliketomatoes
Copy link
Author

iliketomatoes commented Jul 18, 2022

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?

@robbtraister
Copy link
Contributor

robbtraister commented Jul 19, 2022

@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 />

@Huiet
Copy link

Huiet commented Aug 3, 2022

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

@anton-paskanny
Copy link

Any progress on this issue?

@kiliman
Copy link
Collaborator

kiliman commented Oct 12, 2022

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 #3765 (comment)

@craigsc
Copy link

craigsc commented Dec 14, 2022

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

@brophdawg11
Copy link
Contributor

👋 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 brophdawg11 removed their assignment Jan 23, 2023
@robbtraister
Copy link
Contributor

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.

@brophdawg11
Copy link
Contributor

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?

@robbtraister
Copy link
Contributor

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

@brophdawg11
Copy link
Contributor

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

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

No branches or pull requests

7 participants