-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Vite Environment API - Cloudflare environment support #12637
base: main
Are you sure you want to change the base?
Vite Environment API - Cloudflare environment support #12637
Conversation
…ext virtual module
…t node environment as fallback
|
@@ -26,6 +26,7 @@ | |||
"kleur": "^4.1.5", | |||
"magic-string": "^0.30.5", | |||
"mrmime": "^2.0.0", | |||
"pathe": "^1.1.2", |
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.
We try really hard not to add new dependencies. Let's find a way to do this without adding one
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 is to use in place of node:path
for compatibility with other runtimes. We're only using the resolve
function though so we could probably create our own with the same behaviour.
Our current abstraction for different environments is through adapters. My expectation would be that this was kept and no provider/environment specific code like Instead the adapter initializer should return all things needed and kit wires it together with its own vite plugins. Things to keep in mind:
|
Sure, that makes sense. I've added a sentence to clarify that this isn't a suggestion for the final implementation. I think it's worth first considering the changes that are needed to SvelteKit internals (primarily exposing the manifest etc. as a virtual module) and then thinking about the best way to integrate this and the options that are needed.
|
I've been thinking about how this could be integrated with the existing adapter API, as @dominikg mentions. One option would be to add an additional It could look something like this: /**
* @returns {Promise<(request: Request) => Promise<Response>>}
*/
async getHandler(environment) {
return await environment.api.getHandler({
entrypoint: path.join(__dirname, 'cloudflare_entrypoint.js')
});
} We would also need a way to provide the factory function that creates the environment plugin. This could be a /**
* @returns {import('vite').Plugin}
*/
createEnvironmentPlugin(environment_name, options) {
return cloudflare(environment_name, options);
} In order to support multiple environments per adapter, the properties could be objects rather than functions: {
// ...
createEnvironmentPlugins: {
ssr: (environment_name, options) => {
return cloudflare(environment_name, options);
}
},
getHandlers: {
ssr: async (environment) => {
return await environment.api.getHandler({
entrypoint: path.join(__dirname, 'cloudflare_entrypoint.js')
});
}
}
}; What are your thoughts? It would be great to come up with something that the other adapters will also be able to utilise. |
* @returns {(environment_name: string) => import('vite').Plugin[]} | ||
*/ | ||
export function node() { | ||
// @ts-ignore |
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.
can we remove the ts-ignore
or add a comment saying why the ts-ignore
is required?
/** | ||
* This fetch handler is the entrypoint for the environment. | ||
* @param {Request & { cf: any }} request | ||
* @param {any} env |
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'd be nice to avoid the any
* The environment that was provided to `kit.environments.ssr` in the Svelte config. | ||
* @type { ((import('vite').DevEnvironment & { api?: { getHandler: (opts: { entrypoint: string }) => Promise<(req: Request) => Promise<Response>> }})) | undefined } | ||
*/ | ||
const devEnv = vite.environments[SSR_ENVIRONMENT_NAME]; |
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 know our naming scheme is unusual. sorry...
const devEnv = vite.environments[SSR_ENVIRONMENT_NAME]; | |
const dev_env = vite.environments[SSR_ENVIRONMENT_NAME]; |
*/ | ||
const devEnv = vite.environments[SSR_ENVIRONMENT_NAME]; | ||
|
||
const __dirname = fileURLToPath(new URL('.', import.meta.url)); |
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 live at the top of the file
if (devEnv) { | ||
if (devEnv.api) { | ||
handler = await devEnv.api.getHandler({ | ||
entrypoint: path.join(__dirname, 'cloudflare_entrypoint.js') |
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.
let's move anything cloudflare-specific to the cloudflare adapter
* @param {any} env | ||
* @param {any} context | ||
*/ | ||
fetch: async (request, env, context) => { |
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 don't know if fetch
is the name I would use for this. It makes me think it's a per-environment implementation of the fetch
API, but it's not really since the request signature differs. Maybe something like handleRequest
would be better
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 is the standard format for a Cloudflare Worker entrypoint (https://developers.cloudflare.com/workers/get-started/guide/#3-write-code).
} else { | ||
const module_runner = createServerModuleRunner(vite.environments[SSR_ENVIRONMENT_NAME]); | ||
const entrypoint = await module_runner.import(path.join(__dirname, 'node_entrypoint.js')); | ||
handler = entrypoint.default.fetch; |
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.
perhaps this path should also be called if !devEnv
? I think it would simplify things to always have a handler
defined and the default should probably be to use the Node handler
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.
Yes, that's definitely the best way to go. That's what I meant by this comment:
This could eventually be used as the default environment when kit.ssrEnvironment is not provided. This would mean that a single approach could be used, based on the entrypoint and virtual module convention, rather than maintaining two separate code paths.
It requires going all in on this approach though so I need to fill in any missing functionality first.
|
||
const resolve = create_resolve(${s(cwd)}); | ||
|
||
export let manifest = { |
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.
should the manifest code be deleted from vite/dev/index.js
? i.e. does this replace that one?
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.
Yes, same as above. This is the best approach but means that there can't be any holes in the implementation. It's quite useful to be able to compare with the existing code path for the time being but I'll remove the redundant code when possible.
kit: { | ||
adapter: adapter(), | ||
environments: { | ||
ssr: cloudflare() |
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 this should be specified by using the cloudflare adapter instead and I would remove this config option
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.
Agreed. I'll work on integrating with the existing adapter API instead.
I'd probably call it
Hmm. So I guess the environments here could be things like 'node', 'edge', etc. (e.g. as used in |
Vite 6 introduces the Environment API, which enables code to be run in custom environments. At Cloudflare, we are working with framework authors to integrate the Environment API and we are developing a plugin that creates a Cloudflare (workerd) environment. In combination, this will enable running server side code in the workerd runtime during local development, ensuring that development closely replicates production. This would replace the existing usage of
getPlatformProxy
that is currently used to simulate the Cloudflare platform in Node.js. In the future, a similar approach could be used to integrate other runtimes e.g. Deno, Bun etc.This PR is a proof of concept of the approach and is intended for discussion and gathering feedback. It follows some initial discussion with @benmccann. A working example can be found at
playgrounds/vite-cloudflare-environment
.I've tried to avoid changing the existing code as much as possible for now so that the main concepts are clear. The details of how it should fit into the SvelteKit codebase and integrate with the existing Adapter API are for discussion.
I've added comments to the new code and include an implementation summary below.
Implementation summary
kit.environments
option has been added to the Svelte config where users can pass in thecloudflare
environment provider factory function.packages/kit/src/exports/vite/index.js
.packages/kit/src/exports/vite/index.js
that exports the values (manifest etc.) required by theServer
instance. This avoids using any Node.js specific APIs.entrypoint
file has been added atpackages/kit/src/exports/vite/dev/cloudflare_entrypoint.js
. This imports the above virtual module, creates theServer
instance and callsserver.respond
to handle theRequest
and return aResponse
. It also adds the Cloudflareplatform
properties. As the code is executed in a workerd environment, these can be accessed directly.packages/kit/src/exports/vite/dev/index.js
, if the environment exists,devEnv.api.getHandler
is called with the aboveentrypoint
file to create the request handler. This is used in place of the existing request handler.Not implemented
read
function from$app/server
can only be used in environments that support file system access so support for this has not been implemented. A possible scenario to consider is that users may deploy to Cloudflare but have pre-rendered routes that access the file system. These would need to be supported in development.vite.moduleGraph.getModuleByUrl
would require communicating with the main process. This should be possible (e.g. usingimport.meta.hot
) or there might be an alternative approach.In addition to the Cloudflare environment described above, a Node.js environment has also been added. An example using this can be found in
playgrounds/vite-node-environment
. This could eventually be used as the default environment whenkit.ssrEnvironment
is not provided. This would mean that a single approach could be used, based on theentrypoint
and virtual module convention, rather than maintaining two separate code paths.I'm sure there will be loads of things I haven't considered so please take a look and let me know if you have any questions.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits