-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Fix native fetch immutable headers issue #9693
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@remix-run/server-runtime": patch | ||
--- | ||
|
||
Fix issue with setting additional headers on raw native fetch responses with immutable headers |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -360,12 +360,12 @@ async function handleDataRequest( | |
|
||
// Mark all successful responses with a header so we can identify in-flight | ||
// network errors that are missing this header | ||
response.headers.set("X-Remix-Response", "yes"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this response is a direct |
||
response = await safelySetHeader(response, "X-Remix-Response", "yes"); | ||
return response; | ||
} catch (error: unknown) { | ||
if (isResponse(error)) { | ||
error.headers.set("X-Remix-Catch", "yes"); | ||
return error; | ||
let response = await safelySetHeader(error, "X-Remix-Catch", "yes"); | ||
return response; | ||
} | ||
|
||
if (isRouteErrorResponse(error)) { | ||
|
@@ -705,8 +705,8 @@ async function handleResourceRequest( | |
if (isResponse(error)) { | ||
// Note: Not functionally required but ensures that our response headers | ||
// match identically to what Remix returns | ||
error.headers.set("X-Remix-Catch", "yes"); | ||
return error; | ||
let response = await safelySetHeader(error, "X-Remix-Catch", "yes"); | ||
return response; | ||
} | ||
|
||
if (isResponseStub(error)) { | ||
|
@@ -799,3 +799,48 @@ function createRemixRedirectResponse( | |
headers, | ||
}); | ||
} | ||
|
||
async function safelySetHeader( | ||
response: Response, | ||
name: string, | ||
value: string | ||
): Promise<Response> { | ||
try { | ||
response.headers.set(name, value); | ||
} catch (error: unknown) { | ||
// Check if this was a directly-returned native `fetch` response with | ||
// immutable headers preventing us from setting additional headers | ||
let isImmutableHeadersError = | ||
isError(error) && | ||
error.name === "TypeError" && | ||
error.message === "immutable"; | ||
|
||
// Re-throw any other type of error | ||
if (!isImmutableHeadersError) { | ||
throw error; | ||
} | ||
|
||
// Clone the response so we can set our headers | ||
let newHeaders = new Headers(); | ||
for (let [key, value] of response.headers.entries()) { | ||
newHeaders.append(key, value); | ||
} | ||
newHeaders.set(name, value); | ||
return new Response(await response.blob(), { | ||
status: response.status, | ||
statusText: response.statusText, | ||
headers: newHeaders, | ||
}); | ||
} | ||
return response; | ||
} | ||
|
||
function isError(e: unknown): e is Error { | ||
return ( | ||
typeof e === "object" && | ||
e != null && | ||
"name" in e && | ||
"message" in e && | ||
"stack" in e | ||
); | ||
} |
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.
I don't really want to keep this here but the only way to replicate this issue was to actually get a legit
Response
from afetch
call which is why we didn't run into this before. Thoughts on how to best prevent a regression in this area without spamming our prod server?