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

Vite Environment API - Cloudflare environment support #12637

Draft
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

jamesopstad
Copy link

@jamesopstad jamesopstad commented Sep 2, 2024

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

  • A new kit.environments option has been added to the Svelte config where users can pass in the cloudflare environment provider factory function.
  • If the above option is provided, the factory function is used to create the plugin in packages/kit/src/exports/vite/index.js.
  • A new virtual module is created in packages/kit/src/exports/vite/index.js that exports the values (manifest etc.) required by the Server instance. This avoids using any Node.js specific APIs.
  • A new entrypoint file has been added at packages/kit/src/exports/vite/dev/cloudflare_entrypoint.js. This imports the above virtual module, creates the Server instance and calls server.respond to handle the Request and return a Response. It also adds the Cloudflare platform properties. As the code is executed in a workerd environment, these can be accessed directly.
  • In packages/kit/src/exports/vite/dev/index.js, if the environment exists, devEnv.api.getHandler is called with the above entrypoint file to create the request handler. This is used in place of the existing request handler.

Not implemented

  • The 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.
  • Inlining styles during SSR has not yet been implemented. The existing approach of collecting dependencies using vite.moduleGraph.getModuleByUrl would require communicating with the main process. This should be possible (e.g. using import.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 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.

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:

  • 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

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

James Opstad added 30 commits August 6, 2024 09:32
Copy link

changeset-bot bot commented Sep 2, 2024

⚠️ No Changeset found

Latest commit: bbe7b42

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jamesopstad jamesopstad changed the title Vite cloudflare environment Vite Environment API - Cloudflare environment support Sep 2, 2024
@@ -26,6 +26,7 @@
"kleur": "^4.1.5",
"magic-string": "^0.30.5",
"mrmime": "^2.0.0",
"pathe": "^1.1.2",
Copy link
Member

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

Copy link
Author

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.

@eltigerchino eltigerchino added this to the 3.0 milestone Sep 2, 2024
@dominikg
Copy link
Member

dominikg commented Sep 2, 2024

Our current abstraction for different environments is through adapters. My expectation would be that this was kept and no provider/environment specific code like packages/kit/src/exports/vite/dev/cloudflare_entrypoint.js was added to kit itself.

Instead the adapter initializer should return all things needed and kit wires it together with its own vite plugins.
It would not scale if we had to add extra code to kit itself for each new environment.

Things to keep in mind:

  1. apps can use more than two environments, not just "client" and "server".
  2. environments are not just for dev time, but also create different outputs
  3. for platform specific apis, there is locals.platform, which might need namespacing unless we don't want to support one adapter that emits multiple environments
  4. it must be environment agnostic so that netlify, vercel, deno, sst, tauri, electron, .... and any custom environment can be integrated just as easy

@jamesopstad
Copy link
Author

Our current abstraction for different environments is through adapters. My expectation would be that this was kept and no provider/environment specific code like packages/kit/src/exports/vite/dev/cloudflare_entrypoint.js was added to kit itself.

Instead the adapter initializer should return all things needed and kit wires it together with its own vite plugins. It would not scale if we had to add extra code to kit itself for each new environment.

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 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.

@jamesopstad
Copy link
Author

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 getHandler property to adapters, which returns a request handler. This would create the handler by calling the getHandler function on the provided environment with the entrypoint file. That way any platform specific code would be kept out of kit. If this getHandler property exists on the adapter then the handler will be used in place of the default request handler in development. It would therefore have precedence over the emulate property.

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 createEnvironmentPlugin property like this:

/**
* @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
Copy link
Member

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
Copy link
Member

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];
Copy link
Member

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...

Suggested change
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));
Copy link
Member

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')
Copy link
Member

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) => {
Copy link
Member

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

Copy link
Author

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;
Copy link
Member

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

Copy link
Author

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 = {
Copy link
Member

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?

Copy link
Author

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()
Copy link
Member

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

Copy link
Author

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.

@benmccann
Copy link
Member

One option would be to add an additional getHandler property to adapters, which returns a request handler.

I'd probably call it getDevHandler

In order to support multiple environments per adapter, the properties could be objects rather than functions

Hmm. So I guess the environments here could be things like 'node', 'edge', etc. (e.g. as used in adapter-vercel). The existing runtime is set on a per-route level. The environments are currently used closer to the server entry point though before we know which route we're processing, so I'm not quite sure how you would choose the appropriate environment. If you have just a single ssr environment things are fine. But once you're choosing an environment based on the route it would appear that things need to be restructured a bit from the current implementation in this PR if possible

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

Successfully merging this pull request may close these issues.

5 participants