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

adapter-cloudflare: IncomingRequestCfProperties are not part of the types on RequestEvent.request #5447

Closed
JReinhold opened this issue Jul 9, 2022 · 3 comments

Comments

@JReinhold
Copy link

JReinhold commented Jul 9, 2022

Describe the bug

When deploying to Cloudflare Pages (or Workers) via @sveltejs/adapter-cloudflare, Cloudflare adds a custom cf property to all requests that contains a lot of useful information about the request.
The property is called IncomingRequestCfProperties and is documented here:
https://developers.cloudflare.com/workers/runtime-apis/request/#incomingrequestcfproperties

The property is actually available on the request object in SvelteKit (when deployed, not locally of course), but the typings for the event doesn't include it RequestEvent.request, and the docs doesn't describe how to fix the types.

Reproduction

Minimal repo here: https://github.com/JReinhold/sveltekit-missing-cf
With emphasis on this line: https://github.com/JReinhold/sveltekit-missing-cf/blob/main/src/routes/index.ts#L6
Deployed to Cloudflare Pages here: https://sveltekit-missing-cf.pages.dev
(remember, this doesn't do anything locally, which is why I've deployed it as well)

Real-life usage can be seen at JReinhold/reinhold.is#39 and in action at https://reinhold.is

Logs

No response

System Info

System:
    OS: macOS 12.4
    CPU: (4) x64 Intel(R) Core(TM) i7-5557U CPU @ 3.10GHz
    Memory: 102.38 MB / 16.00 GB
    Shell: 5.8.1 - /usr/local/bin/zsh
  Binaries:
    Node: 18.3.0 - /usr/local/bin/node
    Yarn: 1.22.18 - /usr/local/bin/yarn
    npm: 8.11.0 - /usr/local/bin/npm
  Browsers:
    Brave Browser: 101.1.38.111
    Chrome: 103.0.5060.114
    Firefox: 101.0.1
    Safari: 15.5
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.55
    @sveltejs/adapter-cloudflare: 1.0.0-next.26 => 1.0.0-next.26
    @sveltejs/kit: next => 1.0.0-next.367
    svelte: ^3.44.0 => 3.49.0
    vite: ^2.9.13 => 2.9.14

Severity

annoyance

Additional Information

I see two possible solutions to this:

  1. As part of the effort in adapter types #5386, let @sveltejs/adapter-cloudflare modify the types on the request to include IncomingRequestCfProperties.
  2. Add the cf property to platform, and update the types accordingly.

Option 2 seems more in line with SvelteKit's general MO of adding platform-specific things to platform and it is more explicit, but doesn't match Cloudflare's documentation on the matter. Option 1 is the opposite.

I'm open to open a PR for either of the solutions but would like a maintainer to decide which is best for SvelteKit.

Relevant work: #5386, #5081

@Rich-Harris
Copy link
Member

I think option 2 is preferable. Not all requests come from Cloudflare — it a page requests some data via fetch in load, for example, SvelteKit will make a request directly against itself, rather than going via HTTP:

response = await respond(
new Request(new URL(requested, event.url).href, { ...opts }),

In either case, cf will be missing. I think a missing platform.cf is probably more understandable than a missing request.cf (though neither is ideal — I think the non-standard property is a regrettable design choice!), so that seems preferable to me. I'm also not sure how we could implement option 1 without some fairly invasive changes to the types.

@Rich-Harris Rich-Harris added this to the whenever milestone Jul 19, 2022
@CanRau
Copy link

CanRau commented Oct 14, 2022

Just stumbled upon the missing .cf types.

Personally I'd say option 1 is more "natural" as that's where you'd expect it to be coming from Workers, as I just did and also that's where the property is already anyway, though if it's too hard to add the types 😬

2 coupled with docs would be better than no types.

Also I found the readme already confusing as I was expecting to destructure platform in my hooks.server.ts though it's actually part of event. Might be just my SvelteKit ignorance as I'm still only couple of ~days old 😅

Anyway, I was pretty excited how quickly and smoothly I just deployed a Kit skeleton to Workers 🥳

@JReinhold
Copy link
Author

Closed by #9978 🚀

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

No branches or pull requests

4 participants