Part 2️⃣ of removing CfWorkerInit["bindings"]#12455
Conversation
|
| "version_metadata", | ||
| bindings | ||
| ); | ||
| const fetchers = extractBindingsOfType("fetcher", bindings); |
There was a problem hiding this comment.
This is messy for now, but a followup will eventually allow Miniflare's config to take a similar structure, removing a lot of this conversion logic
| // In remote `dev`, we make folks use a separate kv namespace called | ||
| // `preview_id` instead of `id` so that they don't | ||
| // break production data. So here we check that a `preview_id` | ||
| // has actually been configured. | ||
| // This whole block of code will be obsoleted in the future | ||
| // when we have copy-on-write for previews on edge workers. | ||
| if (!preview_id && !local) { | ||
| // TODO: This error has to be a _lot_ better, ideally just asking | ||
| // to create a preview namespace for the user automatically | ||
| throw new UserError( | ||
| `In development, you should use a separate kv namespace than the one you'd use in production. Please create a new kv namespace with "wrangler kv namespace create <name> --preview" and add its id as preview_id to the kv_namespace "${binding}" in your ${configFileName(configParam.configPath)} file`, | ||
| { | ||
| telemetryMessage: | ||
| "no preview kv namespace configured in remote dev", | ||
| } | ||
| ); // Ugh, I really don't like this message very much | ||
| } |
There was a problem hiding this comment.
This error (and the below duplicates of it) has been removed. We no longer recommend remote mode dev, and will be removing preview IDs entirely eventually.
There was a problem hiding this comment.
Is this removal an essential part of this restructure?
It feels like this is a significant change to behaviour in what should be a no-op refactor?
There was a problem hiding this comment.
It's not essential, but given I was touching this area of the codebase anyway (and it's a simple enough change) it seemed worth doing. This logic throws an error if the user hasn't specified a preview ID when using KV or R2 bindings and wrangler dev --remote. Crucially, it doesn't display when using remote bindings, which is what we expect most people to use going forward. This error made sense in a world where wrangler dev connected to production resources by default, but since you now have to opt-in with remote = true or --remote it's much clearer to the user that that is happening, and if they want to they can use alternatives like environments.
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: |
| export function getFlatBindings( | ||
| configParam: Config, | ||
| env: string | undefined, | ||
| envFiles: string[] | undefined, | ||
| local: boolean, | ||
| args: AdditionalDevProps | ||
| ): StartDevWorkerInput["bindings"] { | ||
| const bindings = getBindings(configParam, env, envFiles, local, args); | ||
| return convertCfWorkerInitBindingsToBindings(bindings); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is messy and will be removed in part 3️⃣ or 4️⃣
95db292 to
a72b4a0
Compare
909f63e to
9c06fb4
Compare
Co-authored-by: devin-ai-integration[bot] <158243242+devin-ai-integration[bot]@users.noreply.github.com>
petebacondarwin
left a comment
There was a problem hiding this comment.
Just a few comments to resolve before I approve.
| const workflowBindings = extractBindingsOfType("workflow", bindings); | ||
| for (const workflow of workflowBindings) { | ||
| delete bindings?.[workflow.binding]; | ||
| } |
There was a problem hiding this comment.
Alternatively, we could avoid more work by passing an ignoreBindingTypes param to the getFlatBindings() and so down to convertCfWorkerInitBindingsToBindings() which would mean it would not even compute these in the first place?
There was a problem hiding this comment.
Potentially in a followup, yes? convertCfWorkerInitBindingsToBindings() will be entirely removed from the codebase in part 6, which means we won't really be doing any duplicate work over the current state of the codebase.
| options?.remoteProxyConnectionString | ||
| ); | ||
|
|
||
| // This function is currently only exported for the Workers Vitest pool. |
There was a problem hiding this comment.
Outside of this PR but this comment is no longer valid. The vite-plugin calls this function too. Does that also want to avoid using the dev-registry?
There was a problem hiding this comment.
I don't think this code actually does anything anymore, since the dev registry is entirely defined in Miniflare. I'll remove it in a followup PR
| // In remote `dev`, we make folks use a separate kv namespace called | ||
| // `preview_id` instead of `id` so that they don't | ||
| // break production data. So here we check that a `preview_id` | ||
| // has actually been configured. | ||
| // This whole block of code will be obsoleted in the future | ||
| // when we have copy-on-write for previews on edge workers. | ||
| if (!preview_id && !local) { | ||
| // TODO: This error has to be a _lot_ better, ideally just asking | ||
| // to create a preview namespace for the user automatically | ||
| throw new UserError( | ||
| `In development, you should use a separate kv namespace than the one you'd use in production. Please create a new kv namespace with "wrangler kv namespace create <name> --preview" and add its id as preview_id to the kv_namespace "${binding}" in your ${configFileName(configParam.configPath)} file`, | ||
| { | ||
| telemetryMessage: | ||
| "no preview kv namespace configured in remote dev", | ||
| } | ||
| ); // Ugh, I really don't like this message very much | ||
| } |
There was a problem hiding this comment.
Is this removal an essential part of this restructure?
It feels like this is a significant change to behaviour in what should be a no-op refactor?
Part 2 of landing #12151 in stages. This part:
LocalRuntimeControllerso that it no longer convertsStartDevWorkerOptions["bindings"]->CfWorkerInit["bindings"]->Miniflare, it just goes straight fromStartDevWorkerOptions["bindings"]to Miniflare's config.A picture of a cute animal (not mandatory, but encouraged)