-
Notifications
You must be signed in to change notification settings - Fork 1k
Support ctx.exports in the Vite plugin
#11238
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 097065b The changes in this PR will be included in the next version bump. 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 |
69919d4 to
d4e9916
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
59a54d3 to
ec6fc66
Compare
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
ctx.exportsctx.exports in the Vite plugin
5af2908 to
c44ccc4
Compare
| let exportType | ||
| if (typeof value === "function") { |
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 it would be good to warn the user if they have other export types in their entry module. That could be done in a follow-up PR.
… move warnings playground into e2e test. This is because the dev server will now error on startup.
c44ccc4 to
097065b
Compare
|
Could you update the description to give some background for the change – why it's needed, how it's used etc? |
Updated. |
edmundhung
left a comment
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 am still going through the second half of the commits. Just a few nits so far
| workers[workerEnvironmentName] = workerConfig; | ||
| environmentNameToWorkerMap.set( | ||
| workerEnvironmentName, | ||
| setWorker(workerConfig) |
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.
Nit: It might just be me but the term "set" seems confusing. How about resolveWorker? We can also change setWorker to accept the environmentNameToWorkerMap and update the map within it.
| const exportTypes = await ( | ||
| viteDevServer.environments[ | ||
| environmentName | ||
| ] as CloudflareDevEnvironment |
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.
Would it be worth introducing a helper like getCloudflareDevEnvironment that throws if the environment is not an instance of CloudflareDevEnvironment?
| ({ miniflareOptions, containerTagToOptionsMap } = | ||
| await getDevMiniflareOptions(ctx, viteDevServer)); |
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.
Nit: It look me a while to realize we are updating the variables here instead of defining an inline function 😅
Should we make it more explicit?
const updatedOptions = await getDevMiniflareOptions(ctx, viteDevServer);
miniflareOptions = updatedOptions.miniflareOptions;
containerTagToOptionsMap = updatedOptions.containerTagToOptionsMap;| function getWorkerNameToWorkflowEntrypointExportsMap( | ||
| workers: Worker[] | ||
| ): Map<string, Set<string>> { |
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.
Nit: Would it be better to have this exported from wrangler or @cloudflare/workers-utils directly instead of a random getDurableObjectClassNameToUseSQLiteMap helper?
Happy to keep this as is for now.
| const exportTypes: ExportTypes = Object.fromEntries([ | ||
| ...[...workerEntrypointExports].map( | ||
| (exportName) => [exportName, "WorkerEntrypoint"] as const | ||
| ), | ||
| ...[...durableObjectExports].map( | ||
| (exportName) => [exportName, "DurableObject"] as const | ||
| ), | ||
| ...[...workflowEntrypointExports].map( | ||
| (exportName) => [exportName, "WorkflowEntrypoint"] as const | ||
| ), | ||
| ]); |
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.
| const exportTypes: ExportTypes = Object.fromEntries([ | |
| ...[...workerEntrypointExports].map( | |
| (exportName) => [exportName, "WorkerEntrypoint"] as const | |
| ), | |
| ...[...durableObjectExports].map( | |
| (exportName) => [exportName, "DurableObject"] as const | |
| ), | |
| ...[...workflowEntrypointExports].map( | |
| (exportName) => [exportName, "WorkflowEntrypoint"] as const | |
| ), | |
| ]); | |
| const exportTypes: ExportTypes = {}; | |
| for (const exportName of workerEntrypointExports) { | |
| exportTypes[exportName] ??= "WorkerEntrypoint"; | |
| } | |
| for (const exportName of durableObjectExports) { | |
| exportTypes[exportName] ??= "DurableObject"; | |
| } | |
| for (const exportName of workflowEntrypointExports) { | |
| exportTypes[exportName] ??= "WorkflowEntrypoint"; | |
| } |
edmundhung
left a comment
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.
Looks good. Just a few minor nits.
| // Wait long enough to ensure that the server would have restarted if it was restarting | ||
| await new Promise((resolve) => setTimeout(resolve, 2000)); |
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.
Instead of waiting for 2 second. How about updating the entry file to return a different text response without changing the exports?
Fixes #000.
ctx.exportsis now supported in the runtime (see https://developers.cloudflare.com/workers/runtime-apis/context/#exports). Invite dev, the user's Worker entry module is not the top-level module that workerd sees. Instead the top-level module exports proxy classes that interface with Vite to fetch the user's code. Because of this,ctx.exportswould not automatically work as the exports to include in this module can not be determined from the user's config. We now run the code in the user's Worker entry module when starting the dev server to determine the exports that should be included.Summary
Initial server start
Note that restarting the Vite dev server itself is not possible during the initial start as the server has not yet been fully initialised. Restarting Miniflare is therefore the better option during this phase and is more optimal.
Updates to Worker entry modules
Note that restarting Miniflare without also restarting the dev server is problematic at this stage. This is because the dev server restart includes teardown and reinitialisation of environments that would otherwise have to be replicated somehow. Restarting the dev server is the best option in this phase and makes the behaviour of updating export types equivalent to updating the Worker config file.