feat(vite-plugin): Add containers support in vite-plugin #9819
Conversation
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
CarmenPopoviciu
force-pushed
the
carmen/containers-in-vite
branch
from
6de3f9b to
1f860e2
Compare
CarmenPopoviciu
force-pushed
the
carmen/containers-in-vite
branch
from
1f860e2 to
33ee2d2
Compare
| @@ -467,6 +516,13 @@ export function cloudflare(pluginConfig: PluginConfig = {}): vite.Plugin[] { | |||
| }) | |||
| ); | |||
| }, | |||
| async closeBundle() { | |||
There was a problem hiding this comment.
I would really appreciate some sanity check here. According to vite docs this is the right hook where we'd want to cleanup process resources,
but I'm new to vite so I might be overlooking smth
| * with image tag set to well-known dev format, or undefined if | ||
| * containers are not enabled or not configured. | ||
| */ | ||
| async function getContainerOptions( |
There was a problem hiding this comment.
this aaalmost duplicates the wrangler counterpart, with the exception that containerBuildId needs to be passed into this function, as the vite-plugin does not have access to it via the config, as wrangler does
There was a problem hiding this comment.
lgtm on an initial pass 🧡
@petebacondarwin can you have a look at this from the vite side?
| // Assemble container options and build if necessary | ||
| const containerOptions = await getContainerOptions( | ||
| entryWorkerConfig, | ||
| containerBuildId as string |
There was a problem hiding this comment.
maybe instead of casting it to a string, assert this is defined?
| entryWorkerConfig, | ||
| containerBuildId as string | ||
| ); | ||
| const dockerPath = unstable_getDockerPath(); |
There was a problem hiding this comment.
instead of exporting this function in wrangler, i think we can just get it off process.env. According to pete all the env var factories exist because we wanted to consolidate and warn on certain env vars. but we don't need that for this env var, so we can just get it off process.env to reduce complexity.
Fixes #9793 & DEVX-1944