-
Notifications
You must be signed in to change notification settings - Fork 514
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-pages): pass CfRequestContext to nitro localCall #997
fix(cloudflare-pages): pass CfRequestContext to nitro localCall #997
Conversation
@@ -44,8 +44,7 @@ export async function onRequest(ctx: CFRequestContext) { | |||
host: url.hostname, | |||
protocol: url.protocol, | |||
body, | |||
// TODO: Allow passing custom context | |||
// cf: ctx, | |||
context: ctx, |
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.
Note: this needs to be called context
and not cf
(as suggested in the comment) because unenv takes this specific context field and puts it in the req.__unenv__
: https://github.com/unjs/unenv/blob/4ed234cf8fd88264852739d14b40bebf7ce31afe/src/runtime/fetch/call.ts#L51
(which is where then users can have access to it)
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.
Sure but it can be nested like here: https://github.com/unjs/nitro/blob/7bcfc362a77764aed566b9f266ae702ccd0dceed/src/runtime/entries/cloudflare.ts#L36
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.
ah yeah good point
in the current way it makes getting the value from nuxt quite clean just via event.context.env
.
if I nested it with something like cfContext
it would be: event.context.cfContext.env
which is not as clean, but not terrible either (note, I can add the cf
later that is a different unrelated object)
would you prefer that I nested the value?
(PS: it could actually help if/when we add cf
as well π€ )
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.
It is intentionally verbose because we are accessing a platform-api. cf
is already added for normal worker target. we need to add it it for pages too (i suppose interfaces are same)
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.
ah by the way, regarding the cf
we I don't think we need to manually pass it since it will accessible from the ctx.request
anyways
so it looks like context: ctx,
should actually be enough, what do you think?
or alternatively we can and choose what fields from the CfRequestContext pass along, in that case we could construct the context
ourselves, but I am not sure if that's needed/beneficial
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.
event.context.cf
can have all the keys and context passed by cloudflare by using context: { cf: ctx }
that simple :)
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.
mh.... ok, but if we do that then if someone uses the worker adaptor then the event.context.cf
will contain only the cf object whilst with the pages adaptor event.context.cf
will contain the CfRequestContext
, those are two very different objects, so this might lead to confusion?
if you are ok with this misalignment of the two I am as well, I'd also want to update the worker adaptor anyways in a followup PR to make it use the ESM syntax (which is needed to pass things cleanly there as well, and also in order to have access to some services like DOs), so I could re-align them there (but I wanted to get this pages change out of the way asap so that people can start using Nuxt on Cloudflare pages, the worker adaptor refactoring is not that pressing)
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.
@pi0 I've updated the code as you suggested, please have a look π
@pi0 sorry to bother you, could you help me understand why the deployment's failing? |
Failed docs deployment is okay :) |
7bcfc36
to
db33453
Compare
in the cloudflare-pages adapter add the CfRequestContext that the onRequest function receives from Cloudflare to the nitroApp localCall so that users can access such context in their nitro application resolves nitrojs#773
db33453
to
4fde52c
Compare
No longer needed after #1004 |
π Linked issue
#773
β Type of change
π Description
in the cloudflare-pages adapter I've added the CfRequestContext that the onRequest function receives from Cloudflare to the nitroApp localCall so that users can access such context in their nitro application
π Checklist