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

Remix should not rely on Response.headers.raw #4354

Closed
benmerckx opened this issue Oct 11, 2022 · 5 comments · Fixed by remix-run/web-std-io#39 or web-std/io#89
Closed

Remix should not rely on Response.headers.raw #4354

benmerckx opened this issue Oct 11, 2022 · 5 comments · Fixed by remix-run/web-std-io#39 or web-std/io#89
Assignees
Labels
bug:unverified feat:fetch Issues related to @remix-run/web-fetch

Comments

@benmerckx
Copy link

benmerckx commented Oct 11, 2022

What version of Remix are you using?

1.7.2

Steps to Reproduce

Any Response object that is created through other means than @remix-run/node (non node-fetch polyfills/undici/node builtins) fails here because Remix relies on the non standard Reponse.headers.raw:

for (let [key, values] of Object.entries(nodeResponse.headers.raw())) {
for (let value of values) {
res.append(key, value);
}
}

Expected Behavior

Remix should use the standard Response.headers interface to maximize compatibility, eg:

  for (let [key, value] of nodeResponse.headers.entries()) {
    res.append(key, value);
  }

Actual Behavior

Handing off a Response to Remix results in an error:

TypeError: nodeResponse.headers.raw is not a function
    at sendRemixResponse (C:\projects\alinea\examples\framework-remix\node_modules\@remix-run\express\dist\server.js:89:65)
    at C:\projects\alinea\examples\framework-remix\node_modules\@remix-run\express\dist\server.js:40:13
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
@kiliman
Copy link
Collaborator

kiliman commented Oct 11, 2022

I think they did it at one point due to an error in how duplicate headers were being handled in node-fetch. Headers like set-cookie were combined into a single header with comma-delimited values which was incorrect.

Now that Remix uses its own fetch implementation, I think we should go back to response.headers.entries()

@MoSattler
Copy link

This is causing problems when doing certain things with resource routes - e.g. handing GQL requests

@MichaelDeBoey
Copy link
Member

Closed by #7150

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version v0.0.0-nightly-1d3d86e-20230817 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!

Thanks!

@github-actions
Copy link
Contributor

🤖 Hello there,

We just published version 2.0.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!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:unverified feat:fetch Issues related to @remix-run/web-fetch
Projects
No open projects
Status: Merged
6 participants