-
Notifications
You must be signed in to change notification settings - Fork 135
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
feat: add support for next15 geolocation #617
Conversation
Next15 moves the geolocation information from a geo property on the request to `x-vercel-ip` headers. `@vercel/functions` has a `geolocation` helper function to access those.
🦋 Changeset detectedLatest commit: f267083 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
Coverage Report
File Coverage
|
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 misunderstood what we were discussing on discord, i added a few comment to explain
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 think i misunderstood what you were saying on discord, we don't want to modify this but the cloudfront function in sst.
This one is only used when using lambda@edge (either with external middleware or as a full server fuction).
If we'd do that we'll also have to modify the other lambda converter
If we'd want to provide these headers for every function, we could keep the x-open-next
one in the cloudfront functions and we apply the x-vercel
in here :
export default async function routingHandler( |
We could add a function there that would add the x-vercel
headers only if the x-open-next
headers are present
@@ -6,12 +6,20 @@ import type { | |||
|
|||
import type { MiddlewareOutputEvent } from "../../core/routingHandler"; | |||
|
|||
const cfPropNameToHeaderName = { | |||
city: "x-vercel-ip-city", |
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.
If we decide to change the headers in routingHandler
then it would make sense to rename those as x-open-next
af4ceaf
to
6e66659
Compare
@conico974 PTAL - I think it's better... after 25 force pushes 🤣 |
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.
Just a little note, other than that LGTM !!
@@ -149,7 +143,7 @@ export function addNextConfigHeaders( | |||
source, | |||
locale, | |||
} of configHeaders) { | |||
const path = locale === false ? rawPath : localizedRawPath; | |||
const path = locale === false ? event.rawPath : localizePath(event); |
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.
The localizePath
was done outside in order to avoid recomputing the same value for every value.
Not that big of a deal but worth mentioning
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.
OOps, I missed that, I'll revert
No description provided.