-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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 a new dataRedirect utility for external redirects on .data requests #13364
base: dev
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: e15116a The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
res.statusMessage = response.statusText; | ||
res.status(response.status); | ||
for (let [key, value] of response.headers.entries()) { | ||
res.append(key, value); | ||
} |
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.
We don't export our internal logic for converting a fetch Response
to an express res
so for now it's assumed that the application has to do that adapter logic on their own. I think this is ok since status/headers are all that matter? There isn't a body on these responses
} else { | ||
return { status: SINGLE_FETCH_REDIRECT_STATUS, data }; | ||
} | ||
} |
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.
If we get back a 204 redirect we convert it to a single fetch redirect here for downstream processing
} | ||
if (result.replace) { | ||
headers["X-Remix-Replace"] = "yes"; | ||
headers["X-Remix-Replace"] = "true"; |
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.
Align these with values used elsewhere - value doesn't really matter since we use .has(header)
Background: In order to trigger a redirect navigation from a client side
fetch
, we have to use something other than a 3xx response because a 3xx will redirect thefetch
call. We need to return some form of "data" that we can look at and decide to perform a soft application redirect.Prior to single fetch, we used to return 204 responses with
X-Remix-Redirect
headers. These worked well and used web standards so while the 204 code and custom header were "implementation details", folks could use them if/when needed to perform redirects on_data
requests from outside of Remix - i.e., in an express middleware.With single fetch, we switched to encoding the redirect into the body of the response so we could encode them in prerendered
.data
files as well. This means that a redirect on a.data
request relies on the implementation detila of turbo-stream's encoding format.This made redirects on
.data
requests from outside of React Router much tricker, since they now need to encode aturbo-stream
response body and use the custom symbol we use internally (#12693).Options: We have 2 options if we want to re-enable this functionality:
turbo-stream
This PR adds back support for those 204 responses, and it also introduces a
dataRedirect
utility (bikeshedding welcome) for constructing them so we don't have to leak our custom header implementation details.Alternatively, another option now that the
redirect
util has forked a few times (forreplace
andreloadDocument
navigations) is to extendResponseInit
and collapse those back into a single utility:We originally chose not to extend onto
ResponseInit
like that but there is some precedent for it: https://developers.cloudflare.com/workers/runtime-apis/response/#parameters