-
Notifications
You must be signed in to change notification settings - Fork 176
refactor: internalEvent.url is a full URL #752
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
Changes from 2 commits
918e651
ea73fc6
3f36c06
c2d5d99
0e20584
e01ef8d
9ffa41a
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,9 @@ | ||
| --- | ||
| "@opennextjs/aws": patch | ||
| --- | ||
|
|
||
| `InternalEvent#url` is now a full URL | ||
|
|
||
| BREAKING CHANGE: `InternalEvent#url` was only composed of the path and search query before. | ||
|
|
||
| Custom converters should be updated to populate the full URL instead. |
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -57,13 +57,14 @@ async function convertFromAPIGatewayProxyEvent( | |||||||
| event: APIGatewayProxyEvent, | ||||||||
| ): Promise<InternalEvent> { | ||||||||
| const { path, body, httpMethod, requestContext, isBase64Encoded } = event; | ||||||||
| const headers = normalizeAPIGatewayProxyEventHeaders(event); | ||||||||
| return { | ||||||||
| type: "core", | ||||||||
| method: httpMethod, | ||||||||
| rawPath: path, | ||||||||
| url: path + normalizeAPIGatewayProxyEventQueryParams(event), | ||||||||
| url: `https://${headers.host ?? "on"}${path}${normalizeAPIGatewayProxyEventQueryParams(event)}`, | ||||||||
|
||||||||
| if (initialHeaders["x-forwarded-host"]) { | |
| initialHeaders.host = initialHeaders["x-forwarded-host"]; | |
| } |
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.
Ok I'll apply the change in aws converters.
Could the code in request handler (your link) be deleted then?
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.
Honestly i'm not sure how to handle this one properly. We need the headers to be updated, but only if there is a x-forwarded-host
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.
This could be done in the converter as it is aws specific, no?
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.
Not entirely, this is kind of a standard https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host.
And this will likely cause trouble when used with the node converter and wrapper as well.
Maybe a better solution would be to modify the url in the request handler as well. If we have a x-forwarded-host we change both the host header and the url to use this header.
We may have to move this to the routing handler though (in case of an external middleware)
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.
Here it fails in e2e because
event.urlis incorrect.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.
Do you know what test fails?
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.
opennextjs-aws/packages/tests-e2e/tests/appRouter/config.redirect.test.ts
Line 17 in 674dce6
opennextjs-aws/packages/tests-e2e/tests/appRouter/config.redirect.test.ts
Line 40 in 674dce6
opennextjs-aws/packages/tests-e2e/tests/appRouter/config.redirect.test.ts
Line 51 in 674dce6
And these 2
opennextjs-aws/packages/tests-e2e/tests/pagesRouter/redirect.test.ts
Lines 11 to 25 in 674dce6