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

Add support to force reload on redirect with X-Remix-Reload-Document header #10705

Merged
merged 14 commits into from
Aug 2, 2023

Conversation

robbtraister
Copy link
Contributor

@robbtraister robbtraister commented Jul 14, 2023

Sometimes we want to force a document reload when redirecting on the same origin. This PR adds support for a custom header (X-Remix-Reload-Document), which, when present, tells the router to trigger a document reload when handling a redirect in the browser.

My use-case for this behavior is using remix as an OAuth login app, sharing the same origin with other apps, each served on a separate basename (behind a reverse proxy). On the final stage of the login flow, the response may either fail or succeed. In the case of failure, we return an error message without a page reload and remain within the remix app. In the case of success, we redirect to the target application. Since the result is dynamic, we cannot set reloadDocument in the request form. Since the target application is served externally from remix, we cannot use a history redirect. And we cannot rely on a success page to trigger a client-side redirect as that won't work without javascript.

The only solution that works in all cases (including without javascript) is a custom header included with the redirect response that is then used to trigger a document reload in the client router.

I have an example repo with this implementation patched here: https://github.com/robbtraister/remix-hard-redirect

I also added and exported a redirectWithReload utility function that appends the header in question so you don't need to remember or type it.

Link to the Open Development discussion

Robb Traister added 3 commits July 14, 2023 15:40
This header designates a redirect to trigger a full document reload.
X-Remix-Reload-Document header automatically
@changeset-bot
Copy link

changeset-bot bot commented Jul 14, 2023

🦋 Changeset detected

Latest commit: 05de4f6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
react-router Minor
react-router-dom Minor
react-router-dom-v5-compat Minor
react-router-native Minor
@remix-run/router Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 14, 2023

Hi @robbtraister,

Welcome, and thank you for contributing to React Router!

Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once.

You may review the CLA and sign it by adding your name to contributors.yml.

Once the CLA is signed, the CLA Signed label will be added to the pull request.

If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run.

Thanks!

- The Remix team

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jul 14, 2023

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

Copy link
Contributor

@brophdawg11 brophdawg11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR - just a few minor comments! We're working on getting Remix v2 out the door so I'll figure out if we want to include this in the 2.0.0 release or follow it up in a 2.1.0.

packages/router/utils.ts Outdated Show resolved Hide resolved
packages/router/__tests__/router-test.ts Outdated Show resolved Hide resolved
@brophdawg11
Copy link
Contributor

We just released Remix 1.19.0 and that should be our last 1.x release so I think we're good to go - I'll handle the small comments above and get this merged. Thanks for the PR!

@brophdawg11 brophdawg11 self-assigned this Jul 19, 2023
@brophdawg11
Copy link
Contributor

I think this is good to go but chatting with @mjackson we may want to bikeshed the redirectWithReload function name a bit, a few options off the cuff:

  • redirectAndReload
  • redirectReload
  • redirectDocument
  • hardRedirect

@gerbyzation
Copy link

gerbyzation commented Jul 20, 2023

Has it been considered to keep this redirect function itself with an optional argument/option object? Something along the lines of redirect("/id/login", { reloadDocument: true})

@robbtraister
Copy link
Contributor Author

robbtraister commented Jul 20, 2023

I think this is good to go but chatting with @mjackson we may want to bikeshed the redirectWithReload function name a bit, a few options off the cuff:

  • redirectAndReload
  • redirectReload
  • redirectDocument
  • hardRedirect

You can see by then name of my example repo that I originally had called it hard-redirect, so I'm fine with that. I changed it after this comment, which seemed reasonable to me. I have no preference.

@robbtraister
Copy link
Contributor Author

Has it been considered to keep this redirect function itself with an optional argument/option object? Something along the lines of redirect("/id/login", { reloadDocument: true})

The biggest reason against doing this is that the current options object is the standard ResponseInit (as defined in the standard DOM lib and used by Response). This same options object is used for json and defer responses, as well.

It is relatively easy to extend the options specifically for the redirect function. @brophdawg11 thoughts?

@robbtraister
Copy link
Contributor Author

robbtraister commented Jul 20, 2023

Here's the change to using an overloaded options object, for comparison: robbtraister/react-router@feature/redirect-with-reload...feature/redirect-with-reload-option#diff-f8259c3bc9c7dd9df41ae71334beb10f64f09a7d9045f9d1abd504310590bf9d

We would need to add a similar type change to the remix PR, as well.

@gerbyzation
Copy link

Yes it was just for illustration, if done this way it might have to be a third parameter. An override would prevent you from passing headers at the same time.

@brophdawg11
Copy link
Contributor

I don't like extending ResponseInit, since then you would need to strip it off to avoid passing it to new Response.

I like the separate function for explicitness, and better than extending ResponseInit or adding a third param - it's awkward to have multiple optional params in a function signature.

@robbtraister
Copy link
Contributor Author

While I appreciate the idea of preserving the standard API, there is nothing preventing overloading. You do not need to strip the extraneous property from the options object; the Response constructor will just ignore it. And if you're concerned about passing extra values, you could use destructuring or the delete keyword.

Point being, I think debating the API is worthwhile, but implementation details, not so much.

@brophdawg11
Copy link
Contributor

the Response constructor will just ignore it

For now 😉 . I don't like relying on shapes of 3rd party APIs not changing when they're out of my control. Is the Response constructor likely to start accepting a reloadDocument option in the future? No. Could it? Yes 😄.

Plus it makes reasoning/explaining/docs tougher - since the second param is no longer a commonly understood/standard type - it's a commonly understood/standard type and then this one other little tiny thing that's specific to React Router/Remix.

@robbtraister
Copy link
Contributor Author

standard type and then this one other little tiny thing

this was my original point (and I agree).

What do you think of exporting a const with the value of the header name? Either along with, or in place of, the custom redirectWithReload (or whatever it's called) function.

it would look something like this:

import { redirect, RELOAD_DOCUMENT } from "@remix-run/server-runtime";

export const loader = () => redirect("/external", { headers: { RELOAD_DOCUMENT: "true" } });

@brophdawg11
Copy link
Contributor

Thanks for this @robbtraister! We landed on redirectDocument as the preferred name so updated that and added some docs as well. This will be available in the next React Router release, and Remix v2

@brophdawg11
Copy link
Contributor

We just published version 6.15.0-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

@robbtraister
Copy link
Contributor Author

I pinned react-router@6.15.0-pre.0 (and react-router-dom) and removed the patch in my example app here: robbtraister/remix-hard-redirect@b2defd3

Works exactly as expected. Huzzah!

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

Successfully merging this pull request may close these issues.

4 participants