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

Fix getting cookies from headers. #41

Merged

Conversation

gotgenes
Copy link

@gotgenes gotgenes commented Jun 25, 2024

Fixes an issue in the API Gateway v2 adapter where the call to headers.raw() throws the following error:

TypeError: nodeResponse.headers.raw is not a function

As described in remix-run/remix#4354, the headers.raw() method is not available in the Headers class in Node.js. This change replaces the headers.raw() method with headers.entries() from the Fetch API to fix the issue.

Edit: The original approach used headers.getSetCookie(), but @wingleung informed me this is not available in Node 18. The original PR also included dependency updates, which I since removed.

@iradcn
Copy link

iradcn commented Jul 12, 2024

I'm experiencing this issue and would love to see this being merged!

@gotgenes
Copy link
Author

gotgenes commented Oct 8, 2024

@wingleung Is there anything I can do to help you review this PR?

@wingleung
Copy link
Owner

wingleung commented Oct 10, 2024

hey @gotgenes sorry for the late reply, thank you for the PR 🙏
the headers.getSetCookie() is only supported from node.js 19.7.0 and up, while remix still supports node 18 so I think we need to tweak it a little.

I see other solutions that use headers.entries() instead which offers support from node.js 18 and up
👉 https://github.com/remix-run/remix/pull/7150/files#diff-e37d9e295b178aa51a4880acae4b8bc7f82759e6868583513a5fa52b16846408

related to that, bumping the remix version in the peerDependencies might give unexpected behavior for some people who are on older versions of remix

@gotgenes gotgenes force-pushed the fix-getting-cookies-from-headers branch from b397ffb to 3a5e651 Compare October 12, 2024 14:28
@gotgenes
Copy link
Author

@wingleung I updated the PR with your recommended changes. AFAICT this is working on the Node 20 Lambda runtime.

@wingleung
Copy link
Owner

wingleung commented Oct 13, 2024

@gotgenes just a question, how are you setting your headers?

When I test it locally with following loader function I get a set-cookie header for each letter of the value

export const loader: LoaderFunction = () => {
  const headers = new Headers();
  headers.append("Set-Cookie", "cookie1=cookie1Value");
  headers.append("Set-Cookie", "cookie2=cookie2Value");

  return json({}, {
    headers
  })
}
Screenshot 2024-10-13 at 23 02 54

I believe we can remove the nested for loop here 👉 https://github.com/wingleung/remix-aws/pull/41/files#diff-6f39d6ba51b70c669d17f629bce999ac9f413e477a17f01ac8effd3f2d589c10L62
but I would like to know if there was another way of setting cookies that I'm missing here.

@gotgenes gotgenes force-pushed the fix-getting-cookies-from-headers branch from 3a5e651 to 29acb94 Compare October 14, 2024 13:21
@gotgenes
Copy link
Author

@wingleung I'm sorry, you're correct. I've updated the PR to remove that inner loop.

@wingleung wingleung self-assigned this Oct 15, 2024
@wingleung wingleung changed the base branch from main to develop October 15, 2024 10:37
@wingleung wingleung merged commit 3f60f56 into wingleung:develop Oct 15, 2024
@wingleung
Copy link
Owner

@gotgenes thank you! 🙏 merged to develop and published a beta version 👉 1.3.0-beta.1

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

Successfully merging this pull request may close these issues.

3 participants