[wrangler] Remove redundant dev-registry filtering - #12974
Conversation
🦋 Changeset detectedLatest commit: 9b3d0ed 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good Reviewed changesets:
|
|
/bonk this is failing CI checks. Can you fix it, and also add details to agents.md so you don't open a broken PR in future? |
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: |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I verified the correctness of this change by tracing the code paths through both the removed code and buildMiniflareBindingOptions.
Service bindings — kCurrentWorker rewrite was redundant:
The removed code rewrote self-referencing service bindings from { name: "my-worker" } to { name: kCurrentWorker }. In Miniflare's getCustomServiceDesignator (packages/miniflare/src/plugins/core/index.ts:359), both paths resolve identically:
kCurrentWorker→getUserServiceName(refererName)→core:user:my-worker- String
"my-worker"→getUserServiceName("my-worker")→core:user:my-worker
Since refererName equals the worker's own config name at this call site, the outputs are the same.
Service bindings — behavioral improvement confirmed:
buildMiniflareBindingOptions (packages/wrangler/src/dev/miniflare/index.ts:555-575) includes both props and fetcher-type bindings. The removed override was dropping both of these. Removing the override is a strict improvement.
Durable objects — truly redundant:
The buildMiniflareBindingOptions output for durableObjects (packages/wrangler/src/dev/miniflare/index.ts:830-851) is functionally identical to the removed re-derivation, differing only cosmetically (e.g., config.containerDOClassNames?.size vs config.containers?.length as the container guard, both derived from the same source data).
No unused imports: kCurrentWorker was the only import removed, and all remaining imports (extractBindingsOfType, getDurableObjectClassNameToUseSQLiteMap, partitionDurableObjectBindings, getImageNameFromDOClassName) are still referenced elsewhere in the file.
Changeset looks good — correctly classified as patch for wrangler, and the description accurately explains the change.
Clean removal of dead code. LGTM.
|
Review posted on PR #12974. Here's a summary of my findings: The PR correctly removes 62 lines of redundant post-processing code from
No unused imports remain after the change. The changeset is correctly classified as a |
|
/bonk this is failing CI checks. Can you fix it, and also add details to agents.md so you don't open a broken PR in future? |
ask-bonk
Bot
changed the title
|
Fixed both CI failures:
|
|
All fixes have been pushed. Here's a summary of what was done: CI Failures Identified and Fixed:
AGENTS.md Updated with:
|
|
Done. Removed the redundant line from |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@penalosa - is this still valid and desired? |
|
Yes—I'll clean it up |
c11533c to
f1d3f98
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
…iflareWorkerOptions The code that rewrote serviceBindings and durableObjects in unstable_getMiniflareWorkerOptions was originally needed to avoid relying on the dev registry for the Workers Vitest pool. Since the dev registry is now entirely defined in Miniflare, this rewriting is no longer necessary — buildMiniflareBindingOptions already produces the correct bindings. Removing this code also means props on service bindings and fetcher-type bindings are no longer dropped. Also documents the CI PR description validation requirements and a pre-submission checklist in AGENTS.md so future agent runs do not open broken PRs.
f1d3f98 to
289af6e
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers |
Devin AI flagged that removing the kCurrentWorker rewrite breaks
self-referencing service bindings in vitest-pool-workers, which renames
the runner worker to vitest-pool-workers-runner-<project>. Without the
rewrite, a service binding like { service: "my-worker" } when
config.name is "my-worker" resolves to a worker that does not exist
in the workers array (since it was renamed).
Re-add a targeted rewrite that only updates the name field to
kCurrentWorker, preserving props, entrypoint, remoteProxyConnectionString
and fetcher-type bindings (all of which the original block was dropping
because it rebuilt the serviceBindings object from scratch).
The durableObjects re-derivation stays removed — that was genuinely
redundant.
Update the changeset to describe what actually changes (props/fetcher
fix + targeted self-ref rewrite), since the original "dev registry"
rationale was incorrect — vitest-pool-workers does not enable the dev
registry, so it could never have been the safety net the original
description implied.
workers-devprod
left a comment
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
The previous commit restored a kCurrentWorker rewrite for self-referencing service bindings inside wrangler's unstable_getMiniflareWorkerOptions, but the rewrite is only needed because vitest-pool-workers renames the runner worker to vitest-pool-workers-runner-<project>. Other consumers of the API (getPlatformProxy, @cloudflare/vite-plugin) preserve the original worker name and so don't need the rewrite at all. Move the rewrite to vitest-pool-workers itself, right before the rename in buildProjectWorkerOptions. Re-uses the existing getWranglerWorkerName helper and the already-imported kCurrentWorker symbol — no new imports. This leaves wrangler's unstable_getMiniflareWorkerOptions as a pure bindings producer that delegates entirely to buildMiniflareBindingOptions, and localises the consumer-specific rename concern in the consumer that owns the rename.
3 participants
Fixes #12489.
Removed 62 lines of redundant post-processing code from
unstable_getMiniflareWorkerOptionsinpackages/wrangler/src/api/integrations/platform/index.ts.After
buildMiniflareBindingOptions()produced the binding options, the removed code was overridingserviceBindingsanddurableObjectswith rewritten versions:service === config.name) to usekCurrentWorkersymbolpropsfrom service bindingsfetcher-type bindings (by fully replacingserviceBindings)durableObjectsidentically to whatbuildMiniflareBindingOptionsalready producedThis post-processing was redundant because:
kCurrentWorkerrewrite is unnecessary — Miniflare resolves both{ name: "my-worker" }andkCurrentWorkerto the samecore:user:my-workerservice namebuildMiniflareBindingOptionspropson service bindings and fetcher bindings are no longer droppedbuildMiniflareBindingOptionsalready handles. The existing dev-registry integration tests infixtures/dev-registry/cover the affected behavior.