Allow passing platform context from adapters

[fezproof]

This is a draft PR that implements some of the ides in #2426 and #3426. I have only currently implemented passing in a new platform object from adapter/cloudflare-pages as an example, as it is what I am familiar with. Maintainers more familiar with the other adapters and environments should probably handle them.

The main change in this PR allows this in adapters:

return await app.render({ request: req, platform: { env } });

The developer can now access env from the new platform object, which is passed to hooks and endpoints.

Doing this PR has raised an important question though.
How should a pattern like this be handled in local development?

Perhaps each adapter could provide a default platform that can act as a sort of 'polyfill'. Something like KV and Durable Objects from Cloudflare, or user context from Netlify shouldn't be that hard to shim.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• Waiting on remove hn example from ignore option #3427 to allow changeset generation
• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

Closes #2426

[changeset-bot]

🦋 Changeset detected

Latest commit: 983ec4f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@sveltejs/adapter-cloudflare	Patch
@sveltejs/kit	Patch

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

[netlify]

✔️ Deploy Preview for kit-demo canceled.

🔨 Explore the source changes: 983ec4f

🔍 Inspect the deploy log: https://app.netlify.com/sites/kit-demo/deploys/61f20e51b04cd40008d605a8

[Rich-Harris]

Thank you! I'd personally lean towards creating a new property on RequestEnv (perhaps meta) and allowing that to be completely controlled by the adapter, rather than muddling locals. What do you think?

response = await app.render({ request, meta: env });

The local development question is a good one, and it's actually an even deeper can of worms — should the adapter try to simulate the environment completely, by e.g. adding web APIs supported by Cloudflare Workers and Deno? should it remove Node built-ins? (It's useful to be able to use fs during prerendering, in endpoints that will never run in a worker — does that affect the decision?) It's possible that the way to solve this is to somehow integrate with platforms' own development tools, though it's far from clear what that looks like.

All of which is to say that it's probably fine to kick that can down the road for now and address it in a follow-up PR!

[fezproof]

Thanks for the feedback, will update the PR and docs with this approach. I thought about this too but I was afraid adding yet another object passed into the handle hook event could become overbearing, but it also makes a lot of sense to keep them seperate, allowing the developer to make the choice of what to do with it.

Another question to raise is whether this should be passed all the way down to endpoints. Separation of concerns, and not forcing the dev to implement handle get up and running with an adapter to get all its features.

Again, probably a better question for a follow up PR :)

[fezproof]

RE the environment point about fs and such, I think the Cloudflare adapter kind of handles this. I am not exactly sure how the other adapters work (yet), but the es build that happens as part of cloudflares build system forces a 'browser' like environment. It might be ok to allow node specific things to be used in dev, if they are going to fail the build anyway before a deploy.

[Rich-Harris]

Another question to raise is whether this should be passed all the way down to endpoints

definitely, i think — it's easiest to understand if RequestEvent is used consistently throughout the app. not to mention simpler to implement!

[fezproof]

@Rich-Harris @benmccann PR is now ready for review. I am not sure where to put more documentation for the new object, so some guidance there would be appreciated :)

Testing with a local repo, I am thinking meta should be made Readonly? Probably want to encourage that only locals is the object that should be changed.

[benmccann]

I'm not in love with the name meta. Two alternatives would be to rename it to platform or to stick the items directly onto the request object. We should check with the other maintainers on the desired API

The other suggestion I would have is to stick the platform request on the "meta" object or whatever we end up calling it. This has been requested several times in adapter-node especially where some middlewares will stick stuff on the request and today there's no way to access it

[Conduitry]

Do we want to say in the changelog what it contains? If so, we're presumably want separate changesets for the Cloudflare and Node adapters.

[Rich-Harris]

After discussion, we decided it makes more sense to have app.render(request, { platform }) since request is the required argument and the options are optional. This also means that it's a non-breaking change. I've updated the PR accordingly.

Though now that I review it more closely, I find myself asking what problem this actually solves. In the Cloudflare Workers case, env is a global, so as far as I can tell there's nothing to be gained from passing it around like this. The example added to the adapter-node README demonstrates reading a cookie, which is trivial to do already.

Do we actually need this?

[Rich-Harris]

Ah, I see there's a difference between old and new-style workers, and it isn't a global if you're using the new kind (which we are)

[fezproof]

Yeah that’s right. For reference for others seeing this later.

Env is only global in the current common js cloudflare workers. I the newer ES Module workers, and cloudflare pages, it is only passed through as env in the fetch function call.

See
https://blog.cloudflare.com/workers-javascript-modules/
https://developers.cloudflare.com/pages/platform/functions#advanced-mode
From the durable object docs

In modules-syntax workers, bindings are delivered as a property of the environment object passed as the second parameter when an event handler or class constructor is invoked. This is new compared to pre-module workers, in which bindings show up as global variables.

[fezproof]

Perhaps the node README could have an example with something like passport? I haven’t used a node server for a few years so I don’t know what the cool kids are using these days.

[Rich-Harris]

My inclination is probably just to leave adapter-node unchanged until a use case presents itself. Otherwise we might find we're adding the wrong surface area. Updated the PR

[Conduitry]

@Rich-Harris Ben had said on Discord

The Node adapter is passing the request which is something we really needed because some middlewares stick stuff on the request and we have no way to access those at the moment

[Rich-Harris]

some middlewares stick stuff on the request and we have no way to access those at the moment

Would be curious to know what these middlewares are doing and whether there's any intrinsic reason it can't be done inside Kit instead. Presumably the middleware in question doesn't have access to anything that isn't represented on the Request object, it's just shoving stuff on req in the higgledy-piggledy way traditional of Express middlewares. If there's a valid use case then we could definitely do it in a follow-up PR, but the more we can encourage people to do things in an idiomatic way that can be used beyond just Node apps, the better