Part 4️⃣ of removing CfWorkerInit["bindings"]#12461
Conversation
🦋 Changeset detectedLatest commit: 2d34899 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 |
342f512 to
69881a0
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: |
94dc9e4 to
1a8b638
Compare
69881a0 to
84c79cb
Compare
1a8b638 to
5b8c78c
Compare
84c79cb to
73cd210
Compare
- Modify createWorkerUploadForm to accept StartDevWorkerInput["bindings"] - Add "inherit" binding type to support versions/secrets without full binding details - Fix pre-existing bug in print-bindings.ts (array destructuring on string)
73cd210 to
8dcaff8
Compare
ascorbic
left a comment
There was a problem hiding this comment.
Not blocking this, but it would be good to add unit tests here
| export function createWorkerUploadForm( | ||
| worker: CfWorkerInit, | ||
| options?: { dryRun: true } | ||
| ): FormData { | ||
| const bindings = convertCfWorkerInitBindingsToBindings(worker.bindings); | ||
| return createFlatWorkerUploadForm(worker, bindings, { | ||
| dryRun: options?.dryRun, | ||
| unsafe: worker.bindings.unsafe, | ||
| }); | ||
| } | ||
|
|
||
| export function createFlatWorkerUploadForm( | ||
| worker: Omit<CfWorkerInit, "bindings" | "rawBindings">, | ||
| bindings: StartDevWorkerInput["bindings"], | ||
| options?: { | ||
| dryRun?: true; | ||
| unsafe?: { metadata?: Record<string, unknown>; capnp?: CfCapnp }; | ||
| } |
There was a problem hiding this comment.
These seem like functions that would benefit from unit tests.
There was a problem hiding this comment.
They probably would, yeah. We have extensive integration tests in deploy.test.ts that exercise these functions, but no unit tests. I'll add it as a followup item.
There was a problem hiding this comment.
Sorry it's unrelated to this PR, but could you remove the export here? I don't think that it's needed?
| | ({ type: `unsafe_${string}` } & Omit<CfUnsafeBinding, "name" | "type">) | ||
| | { type: "assets" }; | ||
| | { type: "assets" } | ||
| | { type: "inherit" }; |
There was a problem hiding this comment.
Binding is a public facing API, so wouldn't this change require a changeset?
|
Claude finished @penalosa's task —— View job Changeset ReviewTodo List:
The changeset has one significant issue: Version Type Incorrect: The changeset is marked as
However, since the changeset explicitly notes this is internal-only and should not be used by external users, this creates a contradiction:
The current
Recommendation: The Actually, on reflection: ✅ All changesets look good The |
Part 4 of landing #12151 in stages. This part:
createWorkerUploadFormto acceptStartDevWorkerInput["bindings"](flat format)inheritbinding type to supportversions/secretswithout requiring full binding details