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
@cloudflare/workers-utils
wrangler
commit: |
e212f90 to
32810d2
Compare
d2f5c95 to
62ce85b
Compare
| * Union of all binding type discriminant strings. | ||
| * This mirrors the Binding["type"] from wrangler but is defined here to avoid circular dependencies. |
There was a problem hiding this comment.
This is messy, but I don't want to further bloat the diff size of this PR by moving the Binding type. I'll move it in a future PR
| expect(metadata.bindings).toEqual( | ||
| expect.arrayContaining(expectedBindings as unknown[]) | ||
| ); | ||
| expect(metadata.bindings?.length).toEqual( | ||
| (expectedBindings as unknown[])?.length | ||
| ); |
There was a problem hiding this comment.
I spent a lot of time trying to get this to match the order, but ultimately it made everything more complicated. The order of the metadata bindings array shouldn't have any impact.
| @@ -777,7 +777,7 @@ export default { | |||
| "------formdata-undici-0.test | |||
| Content-Disposition: form-data; name=\\"metadata\\" | |||
|
|
|||
| {\\"main_module\\":\\"functionsWorker-0.test.js\\",\\"bindings\\":[{\\"name\\":\\"TEST_JSON_PREVIEW\\",\\"type\\":\\"plain_text\\",\\"text\\":\\"{\\\\njson: \\\\\\"value\\\\\\"\\\\n}\\"},{\\"name\\":\\"TEST_PLAINTEXT_PREVIEW\\",\\"type\\":\\"plain_text\\",\\"text\\":\\"PLAINTEXT\\"},{\\"name\\":\\"KV_PREVIEW\\",\\"type\\":\\"kv_namespace\\",\\"namespace_id\\":\\"kv-id\\"},{\\"name\\":\\"KV_PREVIEW2\\",\\"type\\":\\"kv_namespace\\",\\"namespace_id\\":\\"kv-id\\"},{\\"name\\":\\"DO_PREVIEW\\",\\"type\\":\\"durable_object_namespace\\",\\"class_name\\":\\"some-class-do-id\\",\\"script_name\\":\\"some-script-do-id\\",\\"environment\\":\\"some-environment-do-id\\"},{\\"name\\":\\"DO_PREVIEW2\\",\\"type\\":\\"durable_object_namespace\\",\\"class_name\\":\\"some-class-do-id\\",\\"script_name\\":\\"some-script-do-id\\",\\"environment\\":\\"some-environment-do-id\\"},{\\"name\\":\\"DO_PREVIEW3\\",\\"type\\":\\"durable_object_namespace\\",\\"class_name\\":\\"do-class\\",\\"script_name\\":\\"do-s\\",\\"environment\\":\\"do-e\\"},{\\"type\\":\\"queue\\",\\"name\\":\\"QUEUE_PREVIEW\\",\\"queue_name\\":\\"q-id\\"},{\\"type\\":\\"queue\\",\\"name\\":\\"QUEUE_PREVIEW2\\",\\"queue_name\\":\\"q-id\\"},{\\"name\\":\\"R2_PREVIEW\\",\\"type\\":\\"r2_bucket\\",\\"bucket_name\\":\\"r2-name\\"},{\\"name\\":\\"R2_PREVIEW2\\",\\"type\\":\\"r2_bucket\\",\\"bucket_name\\":\\"r2-name\\"},{\\"name\\":\\"D1_PREVIEW\\",\\"type\\":\\"d1\\",\\"id\\":\\"d1-id\\"},{\\"name\\":\\"D1_PREVIEW2\\",\\"type\\":\\"d1\\",\\"id\\":\\"d1-id\\"},{\\"name\\":\\"SERVICE_PREVIEW\\",\\"type\\":\\"service\\",\\"service\\":\\"service\\",\\"environment\\":\\"production\\"},{\\"name\\":\\"SERVICE_PREVIEW2\\",\\"type\\":\\"service\\",\\"service\\":\\"service\\",\\"environment\\":\\"production\\"},{\\"name\\":\\"AE_PREVIEW\\",\\"type\\":\\"analytics_engine\\",\\"dataset\\":\\"data\\"},{\\"name\\":\\"AE_PREVIEW2\\",\\"type\\":\\"analytics_engine\\",\\"dataset\\":\\"data\\"},{\\"name\\":\\"AI_PREVIEW\\",\\"type\\":\\"ai\\"}],\\"compatibility_date\\":\\"2023-02-14\\",\\"compatibility_flags\\":[],\\"placement\\":{\\"mode\\":\\"smart\\"},\\"limits\\":{\\"cpu_ms\\":50}} | |||
| {\\"main_module\\":\\"functionsWorker-0.test.js\\",\\"bindings\\":[{\\"name\\":\\"TEST_JSON_PREVIEW\\",\\"type\\":\\"plain_text\\",\\"text\\":\\"{\\\\njson: \\\\\\"value\\\\\\"\\\\n}\\"},{\\"name\\":\\"TEST_PLAINTEXT_PREVIEW\\",\\"type\\":\\"plain_text\\",\\"text\\":\\"PLAINTEXT\\"},{\\"name\\":\\"DO_PREVIEW\\",\\"type\\":\\"durable_object_namespace\\",\\"class_name\\":\\"some-class-do-id\\",\\"script_name\\":\\"some-script-do-id\\",\\"environment\\":\\"some-environment-do-id\\"},{\\"name\\":\\"DO_PREVIEW2\\",\\"type\\":\\"durable_object_namespace\\",\\"class_name\\":\\"some-class-do-id\\",\\"script_name\\":\\"some-script-do-id\\",\\"environment\\":\\"some-environment-do-id\\"},{\\"name\\":\\"DO_PREVIEW3\\",\\"type\\":\\"durable_object_namespace\\",\\"class_name\\":\\"do-class\\",\\"script_name\\":\\"do-s\\",\\"environment\\":\\"do-e\\"},{\\"name\\":\\"KV_PREVIEW\\",\\"type\\":\\"kv_namespace\\",\\"namespace_id\\":\\"kv-id\\"},{\\"name\\":\\"KV_PREVIEW2\\",\\"type\\":\\"kv_namespace\\",\\"namespace_id\\":\\"kv-id\\"},{\\"type\\":\\"queue\\",\\"name\\":\\"QUEUE_PREVIEW\\",\\"queue_name\\":\\"q-id\\"},{\\"type\\":\\"queue\\",\\"name\\":\\"QUEUE_PREVIEW2\\",\\"queue_name\\":\\"q-id\\"},{\\"name\\":\\"R2_PREVIEW\\",\\"type\\":\\"r2_bucket\\",\\"bucket_name\\":\\"r2-name\\"},{\\"name\\":\\"R2_PREVIEW2\\",\\"type\\":\\"r2_bucket\\",\\"bucket_name\\":\\"r2-name\\"},{\\"name\\":\\"D1_PREVIEW\\",\\"type\\":\\"d1\\",\\"id\\":\\"d1-id\\"},{\\"name\\":\\"D1_PREVIEW2\\",\\"type\\":\\"d1\\",\\"id\\":\\"d1-id\\"},{\\"name\\":\\"SERVICE_PREVIEW\\",\\"type\\":\\"service\\",\\"service\\":\\"service\\",\\"environment\\":\\"production\\"},{\\"name\\":\\"SERVICE_PREVIEW2\\",\\"type\\":\\"service\\",\\"service\\":\\"service\\",\\"environment\\":\\"production\\"},{\\"name\\":\\"AE_PREVIEW\\",\\"type\\":\\"analytics_engine\\",\\"dataset\\":\\"data\\"},{\\"name\\":\\"AE_PREVIEW2\\",\\"type\\":\\"analytics_engine\\",\\"dataset\\":\\"data\\"},{\\"name\\":\\"AI_PREVIEW\\",\\"type\\":\\"ai\\"}],\\"compatibility_date\\":\\"2023-02-14\\",\\"compatibility_flags\\":[],\\"placement\\":{\\"mode\\":\\"smart\\"},\\"limits\\":{\\"cpu_ms\\":50}} | |||
There was a problem hiding this comment.
This is just array ordering in the bindings key
| The following unsafe metadata will be attached to your Worker: | ||
| { | ||
| \\"extra_data\\": \\"interesting value\\", | ||
| \\"more_data\\": \\"dubious value\\" | ||
| } |
There was a problem hiding this comment.
This is fixing an unrelated bug where unsafe metadata would show up as env.key, when they aren't actually accessible on the env
|
|
||
| // mask anything that was overridden in cli args | ||
| // so that we don't log potential secrets into the terminal | ||
| const maskedVars = { ...withoutStaticAssets.vars }; |
There was a problem hiding this comment.
We no longer need to mask vars, since .env vars and CLI flags are now properly marked as secret_text
| * Throws errors for missing KV/R2 preview configs in remote mode. | ||
| * Logs warnings for missing D1 preview configs. | ||
| */ | ||
| function validatePreviewBindings(config: Config, local: boolean): void { |
There was a problem hiding this comment.
I've maintained the existing preview ID validation, but in a future PR I think we should remove it. Preview IDs are not very useful in practice, and we intend to remove them in the next Wrangler major anyway.
fb0fdc7 to
c1133ed
Compare
| let bindings = getBindings(config); | ||
|
|
||
| for (const [key, value] of Object.entries(props.vars ?? [])) { | ||
| bindings[key] = { | ||
| type: "secret_text", | ||
| value: value, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔴 CLI --var overrides are uploaded as secret_text bindings (changes runtime semantics)
Wrangler deploy/version upload now converts user-provided vars overrides into secret_text bindings. That changes the binding type sent to the Workers API from a normal variable (plain_text) to a secret (secret_text).
Actual vs expected:
- Expected:
--var FOO=barshould behave like a regular variable binding and be uploaded asplain_text(like configvars). - Actual: it becomes a secret binding, which is treated differently by the platform (and can affect
keep_bindings, secret management, and potentially value visibility/constraints).
Root Cause
In deploy.ts, overrides in props.vars are injected into the flat bindings map as secret_text:
packages/wrangler/src/deploy/deploy.ts:801-808
for (const [key, value] of Object.entries(props.vars ?? [])) {
bindings[key] = { type: "secret_text", value };
}In versions/upload.ts, the code similarly rewrites plain_text into secret_text for keys present in props.vars, but then uses that mutated bindings when creating the upload form, so it is not “display-only”:
packages/wrangler/src/versions/upload.ts:585-599
if (binding && binding.type === "plain_text") {
bindings[key] = { type: "secret_text", value: binding.value };
}createWorkerUploadForm() serializes secret_text bindings into Worker metadata:
packages/wrangler/src/deployment-bundle/create-worker-upload-form.ts:120-126
case "secret_text": {
metadataBindings.push({ name: binding, type: "secret_text", text: config.value });
}Impact: Deploys (and versions uploads) that use CLI var overrides will silently change the binding type in the uploaded worker, potentially breaking expectations and changing how bindings are persisted/inherited.
Prompt for agents
Fix CLI var overrides so they remain normal vars (plain_text/json) when uploaded.
1) In packages/wrangler/src/deploy/deploy.ts around lines 801-808, stop creating { type: "secret_text" } bindings for props.vars. Instead, set { type: "plain_text", value } (and keep the masking behavior in printBindings separately if needed).
2) In packages/wrangler/src/versions/upload.ts around lines 585-599, do not mutate the bindings map used for createWorkerUploadForm. If you need to hide values in logs, pass a separate copy of bindings to printBindings where values are masked/converted for display only, but keep the upload bindings as plain_text.
3) Add/adjust tests that verify using CLI vars does not produce secret_text bindings in the uploaded metadata (assert on metadata.bindings types in mock-upload-worker helpers).
Was this helpful? React with 👍 or 👎 to provide feedback.
| }); | ||
| let bindings = getBindings(config); | ||
|
|
||
| for (const [key, value] of Object.entries(props.vars ?? [])) { |
There was a problem hiding this comment.
🟡 Object.entries(props.vars ?? []) drops/incorrectly iterates deploy var overrides when props.vars is undefined
The deploy flow iterates CLI var overrides using Object.entries(props.vars ?? []). When props.vars is undefined, it falls back to an array instead of an empty object.
Actual vs expected:
- Expected: If no CLI vars are provided, the loop should be a no-op.
- Actual: It relies on
[]as a fallback. While this happens to no-op at runtime, it is the wrong shape and is easy to accidentally break (and it obscures intent).
Root Cause
In deploy.ts:
packages/wrangler/src/deploy/deploy.ts:803
for (const [key, value] of Object.entries(props.vars ?? [])) {This should fall back to {} rather than [].
Impact: Today it likely no-ops, but it’s a real logic bug (wrong default type) and can lead to subtle issues if the code inside the loop changes (e.g. assumes string keys or uses object-specific behavior).
| for (const [key, value] of Object.entries(props.vars ?? [])) { | |
| for (const [key, value] of Object.entries(props.vars ?? {})) { |
Was this helpful? React with 👍 or 👎 to provide feedback.
Part 1 of my crusade to simplify data passing in Wrangler by removing the
CfWorkerInittype.This PR simplifies Wrangler's internals by using the
StartDevWorkerInput["bindings"]format for bindings across both dev and deploy flows, building on the StartDevWorker work (#4413).What changed:
printBindings()output is mentioned in the review comments)convertBindingsToCfWorkerInitBindings🎉 since we no longer need to convert between these binding types.This work is important because it aligns the internals of Wrangler with the API structure, and means that we no longer have two internal representations for bindings. It also unlocks some new super-secret stuff that's coming soon 🙈
A picture of a cute animal (not mandatory, but encouraged)