-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Add support to force reload on redirect with X-Remix-Reload-Document
header
#10705
Conversation
This header designates a redirect to trigger a full document reload.
X-Remix-Reload-Document header automatically
🦋 Changeset detectedLatest commit: 05de4f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
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 |
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 If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected]. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
There was a problem hiding this 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
.
We just released Remix |
I think this is good to go but chatting with @mjackson we may want to bikeshed the
|
Has it been considered to keep this redirect function itself with an optional argument/option object? Something along the lines of |
You can see by then name of my example repo that I originally had called it |
The biggest reason against doing this is that the current options object is the standard It is relatively easy to extend the options specifically for the redirect function. @brophdawg11 thoughts? |
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. |
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. |
I don't like extending 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. |
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 Point being, I think debating the API is worthwhile, but implementation details, not so much. |
For now 😉 . I don't like relying on shapes of 3rd party APIs not changing when they're out of my control. Is the 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. |
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 it would look something like this: import { redirect, RELOAD_DOCUMENT } from "@remix-run/server-runtime";
export const loader = () => redirect("/external", { headers: { RELOAD_DOCUMENT: "true" } }); |
Thanks for this @robbtraister! We landed on |
We just published version |
I pinned Works exactly as expected. Huzzah! |
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