Skip to content

Conversation

@penalosa
Copy link
Contributor

@penalosa penalosa commented Sep 1, 2025

Fixes DEVX-1737, DEVX-1738, DEVX-2171, and DEVX-1740

Improves the Wrangler auto-provisioning feature by:

  • writing back changes to the user's config file (not necessary, but can make it resilient to binding name changes)
  • fixing --dry-run
  • fixing bindings view for specific versions to not display TOML
  • some improvements to R2

  • Tests
    • Tests included
    • Tests not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: coming later, tracked in DEVX-2165
  • Wrangler V3 Backport
    • Wrangler PR:
    • Not necessary because: beta feature in v3

@penalosa penalosa requested a review from a team as a code owner September 1, 2025 16:05
@changeset-bot
Copy link

changeset-bot bot commented Sep 1, 2025

🦋 Changeset detected

Latest commit: 040c1e9

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

@pkg-pr-new
Copy link

pkg-pr-new bot commented Sep 1, 2025

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@10521

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@10521

miniflare

npm i https://pkg.pr.new/miniflare@10521

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@10521

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@10521

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@10521

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@10521

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@10521

wrangler

npm i https://pkg.pr.new/wrangler@10521

commit: 040c1e9

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a really nice command, i had no idea it existed until now

(adding it to the docs here: cloudflare/cloudflare-docs#24833)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, me neither until writing this PR!

@penalosa penalosa force-pushed the penalosa/resource-provisioning-next branch from d880601 to 3845580 Compare September 2, 2025 11:56
@penalosa penalosa requested a review from a team as a code owner September 2, 2025 16:02
@penalosa penalosa marked this pull request as draft September 9, 2025 15:17
@penalosa penalosa force-pushed the penalosa/resource-provisioning-next branch from 8ab18a0 to c4f361d Compare September 29, 2025 16:11
@penalosa penalosa marked this pull request as ready for review September 30, 2025 12:47
@github-actions
Copy link
Contributor

Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the v3-maintenance branch with your changes. Thank you for helping us keep Wrangler v3 supported!

Depending on your changes, running git rebase --onto v3-maintenance main penalosa/resource-provisioning-next might be a good starting point.

Notes:

  • your PR branch should be named v3-backport-10521
  • add the skip-v3-pr label to the current PR to stop this workflow from failing

@penalosa penalosa added the skip-v3-pr Skip validation of presence of a v3 backport PR label Sep 30, 2025
Comment on lines 5 to 8
Improves the Wrangler auto-provisioning feature by:
- Writing back changes to the user's config file (not necessary, but can make it resilient to binding name changes)
- Fixing --dry-run
- Fixing bindings view for specific versions to not display TOML
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Should we mention --x-provision just so people know this is behind an experimental flag and the way to test it out?

Also, it would be nice if you can add some details on what the fix for --dry-run is about.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description also mentions "some improvements to R2", would be nice to details what they are

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bindings.send_email.push({ ...binding, name: name });
} else if (binding.type === "wasm_module") {
if (!("source" in binding)) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this not be an error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least a warning to the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is a bit lossy when used with WorkerMetadataBinding (there's a warning in the comment above, but maybe that should be louder?). Essentially we can't really reconstruct legacy bindings like WASM/Text/Data from just the metadata because they also require access to the Worker's modules

}

/**
* Convert either StartDevWorkerOptions["bindings"] or WorkerMetadataBinding[] to CfWorkerInit["bindings"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I like when comments start with a one sentence high level summary of what the function does

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that not kinda what this line is?

if (binding.type === "plain_text") {
bindings.vars ??= {};
bindings.vars[name] = binding.value;
bindings.vars[name] = "value" in binding ? binding.value : binding.text;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would binding.value ?? binding.text work here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because the types don't overlap

} else if (previewIsDefined) {
if (getFlag("RESOURCES_PROVISION")) {
assert(binding);
return getIdFromSettings(config, binding, isLocal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return getIdFromSettings(config, binding, isLocal);
return await getIdFromSettings(config, binding, isLocal);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably have a lint check for that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a problem? It's an async function so returning a promise is perfectly valid

!namespace.preview_id
) {
assert(binding);
return getIdFromSettings(config, binding, isLocal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return getIdFromSettings(config, binding, isLocal);
return await getIdFromSettings(config, binding, isLocal);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

}

export function getKVNamespaceId(
async function getIdFromSettings(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add JSDoc

bindings.send_email.push({ ...binding, name: name });
} else if (binding.type === "wasm_module") {
if (!("source" in binding)) {
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least a warning to the user?

Comment on lines 173 to 177
override inherit(): void {
if (!this.binding.bucket_name) {
this.binding.bucket_name = INHERIT_SYMBOL;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am struggling to get my head around the logic here and in canInherit(). Could you add some comments about how we decide whether we can inherit an R2 binding?
Also, it might help to move this function next to canInherit() since the logic appears to be tightly coupled between the two.

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Oct 3, 2025
@penalosa penalosa requested a review from vicb October 3, 2025 10:40

/**
* Inheriting an R2 binding replaces the id property (bucket_name for R2) with the inheritance symbol.
* This works when deploying (and is appropriate for all other binding types), but it means that the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! This is the motivation I was failing to understand.
Thank you!

}

function getSettings(
export function getSettings(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this function be moved to a shared location (e.g. a file in the utils dir)? 🤔 (possibly as a followup)

@penalosa penalosa force-pushed the penalosa/resource-provisioning-next branch 2 times, most recently from 02a064a to 7ed9ee1 Compare October 13, 2025 11:13
@penalosa penalosa force-pushed the penalosa/resource-provisioning-next branch from 7ed9ee1 to 4a4fad4 Compare October 14, 2025 08:23
@jamesopstad jamesopstad merged commit 88b5b7f into main Oct 14, 2025
36 of 37 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Oct 14, 2025
@jamesopstad jamesopstad deleted the penalosa/resource-provisioning-next branch October 14, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-v3-pr Skip validation of presence of a v3 backport PR

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants