-
Notifications
You must be signed in to change notification settings - Fork 121
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 cloudflare env #514
Fix cloudflare env #514
Changes from all commits
d97633d
08fa891
4a47d5a
69c71d3
7c2277a
88a8675
b8197ce
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,8 @@ | ||
--- | ||
"@opennextjs/aws": patch | ||
--- | ||
|
||
Fix cloudflare env | ||
Fix an issue with cookies and the node wrapper | ||
Fix some issue with cookies being not properly set when set both in the routing layer and the route itself | ||
Added option for headers priority |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,9 @@ const wrapper: WrapperHandler = async (handler, converter) => { | |
const internalEvent = await converter.convertFrom(req); | ||
const _res: StreamCreator = { | ||
writeHeaders: (prelude) => { | ||
res.setHeader("Set-Cookie", prelude.cookies); | ||
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. I don't think 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. Actually it does ( I thought the same thing as well at first ). 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. That's really weird, according to the spec: https://httpwg.org/specs/rfc6265.html#overview it shouldn't. Since the comma character may also be part of the cookie value, eg I wonder what the Edit: I tried doing comma separated Set-Cookies in my server and it doesn't work. Only the first cookie is set. But doing multiple Set-Cookie for each cookie works as expected. 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.
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. It will work with an array: https://nodejs.org/api/http.html#http_response_setheader_name_value 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. Ah ok, I missed that the node response setHeader is doing the heavy lifting. 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. Actually there was an issue here ( i forgot to remove the custom wrapper on my test 🙄 ). |
||
res.writeHead(prelude.statusCode, prelude.headers); | ||
res.flushHeaders(); | ||
res.uncork(); | ||
return res; | ||
}, | ||
|
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.
Where is the
process
variable coming from? Is that specific to cloudflare?Oh nvm, it's the
global.process
... got confused w/ theglobalThis
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 added this as a js banner in the esbuild that i did for the cloudflare worker.
Now that i think about it we should probably do this esbuild part inside of OpenNext as well. We'd get a single file out of it which is easier for cloudflare worker.
I'll publish a sample repo later with SST v3 with different way to deploy OpenNext on different target.
Right now i have EC2, ECS and lambda with either cloudfront or cloudflare in front of it ( and with cloudflare the middleware runs in the worker )
Maybe that's something we could put in the new org as well