sveltejs / kit

web development, streamlined
https://svelte.dev/docs/kit
MIT License
18.66k stars 1.93k forks source link

Allow Google Cloud Run Deployment by adding adapter-node to adapter-auto #11951

Closed LukeSchlangen closed 7 months ago

LukeSchlangen commented 7 months ago

Describe the problem

I would like SvelteKit applications to deploy successfully to Google Cloud Run with zero configuration using adapter-auto. Currently, it requires switching to the adapter-node package.

Describe the proposed solution

I would like to include@sveltejs/adapter-node in the list of adapters in order to support deployment to Google Cloud Run and other Node environments. I proposing to add Node last (code example here) since it might be present in other environments.

// Add Node last since it might be present in other environments
{
  name: 'Node',
  test: () => process.env.NODE_ENV,
  module: '@sveltejs/adapter-node',
  version: '5'
}

Alternatives considered

  1. We could create our own custom adapter for Google Cloud Run, but the only official default environment variables could catch other K Native environments that are not Cloud Run. This would also present additional maintenance when adapter-node already works out of the box.
    {
      name: 'Google Cloud Run',
      test: () => !!process.env.K_SERVICE,
      module: 'svelte-adapter-cloud-run',
      version: '1'
    }

Importance

would make my life easier

Additional Information

I'm happy to make a Pull Request (I have this one drafted), but since I am new to the world of SvelteKit adapters and there may already be a good reason for not including this, I wanted to create the issue for discussion first. I'm sure I am missing something. Let me know if this could work or if this could be modified in some way that would be acceptable.

geoffrich commented 7 months ago

Thanks for reaching out! I'm not really familiar with Google Cloud Run, but from the blog I found it seems it also requires the user to create a Dockerfile to deploy SvelteKit? If that's the case, the auto adapter isn't a great fit for it since the auto adapter should only be for environments that are zero-config (see also: https://github.com/sveltejs/kit/pull/8984).

If the deployment can be zero-config, then maybe the auto adapter could work. However, we'd want an environment variable that is unique to Cloud Run, since NODE_ENV could be present in other environments. We wouldn't want the auto adapter to start trying to use the node adapter unexpectedly in environments that aren't Cloud Run.

LukeSchlangen commented 7 months ago

Thanks for responding @geoffrich!

I helped Karl write the Dockerfile for that blog. So I can also confirm that it wouldn't be needed 😄

Right now, node build which is the last part of that Dockerfile (and starts the SvelteKit application), isn't included in the list of start commands that Buildpacks attempts, but I think the Buildpacks team would be willing to add something specific for SvelteKit (like it did with node .output/server/index.mjs for Nuxt.js).

I think the bigger issue is likely finding an environment variable specific to Cloud Run. The current list of environment variables are all related to K Native, but that might include non-Cloud Run environments as well.

Let me check with the team to see if we can come up with an environment variable that would work.

Thanks again for taking a look.

LukeSchlangen commented 7 months ago

@geoffrich - In the discussion with the Buildpacks team, they brought up the idea that rather than targeting a "Cloud Run specific" environment variable, instead they could add a specific environment variable for SvelteKit's node adapter?

What are your thoughts on something like this?

{
  name: 'Node',
  test: () => process.env.USE_SVELTEKIT_NODE_ADAPTER,
  module: '@sveltejs/adapter-node',
  version: '5'
}

Or it could be used to provide a general-purpose environment variable for any adapter?

{
  name: 'Node',
  test: () => process.env.SVELTEKIT_ADAPTER === '@sveltejs/adapter-node',
  module: '@sveltejs/adapter-node',
  version: '5'
},
{
  name: 'Static',
  test: () => process.env.SVELTEKIT_ADAPTER === '@sveltejs/adapter-static',
  module: '@sveltejs/adapter-static',
  version: '3'
}

This way, it wouldn't be necessary to add a specific configuration object for every host that supports node or static hosting?

geoffrich commented 7 months ago

Hmm, those suggested environment variables feel too generic to me. They seem like they could encourage other platforms to set them as well, and then you couldn't easily bump the specified adapter version since we wouldn't know what deployment platforms would be affected.

At the very least, I think the environment variable should include something to indicate it's coming from cloud run, e.g. CLOUD_RUN_USE_SVELTEKIT_NODE_ADAPTER.

I'll raise this issue internally to see if any other maintainers have an opinion.

LukeSchlangen commented 7 months ago

That makes sense. It should definitely be a conversation between maintainers. My thought process was that "since adapter-node isn't specific to Cloud Run, there's no reason other providers couldn't use it?" But that makes sense from the standpoint of knowing whether or not it's safe to update.

It could also be possible for the environment variable to explicitly define the version? Something like:

process.env.SVELTE_ADAPTER === '@sveltejs/adapter-node@5'

But that could bring in more complexity.

geoffrich commented 7 months ago

I raised this issue in the maintainers channel and we're still leaning towards an environment variable name that indicates it's Cloud-Run-specific to keep things organized and minimize unforeseen consequences.

The proposed process.env.SVELTEKIT_ADAPTER pattern is interesting, but I think that would need a lot more investigation and thought to make sure it's a pattern we feel good about, since we couldn't easily iterate on it once the environment variables are being used. So I think it's best to go with the established platform-specific env var pattern for now.

LukeSchlangen commented 7 months ago

Thank you so much for following up. That makes sense. I'll work with the Google Cloud Buildpacks team to determine a reliable environment variable and make a pull request.

LukeSchlangen commented 7 months ago

@geoffrich - I worked with the Buildpacks team who made these changes to their repository, which should pave the way for zero-config SvelteKit deployments. I created this Pull Request, which should be the last piece in the puzzle. I think it meets the "Cloud Run specific" goals that the SvelteKit team was looking to accomplish. Let me know if there's anything else I should do before this pull request is ready to merge.

Thanks again for all your help!