Skip to content

Conversation

@edmundhung
Copy link
Member

@edmundhung edmundhung commented Aug 6, 2025

Fixes DEVX-2124

This PR fully replaces Wrangler's dev registry setup with the Miniflare implementation, paving the way for dev registry support in multi-config mode in a later PR (#10354).

Notable changes:

  • Removes a large amount of logic from buildMiniflareBindingOptions, since Miniflare now handles this directly. As a result, Wrangler no longer updates the Miniflare config when the dev registry changes, because address resolution now happens in real time within Miniflare's proxy server.
  • Removes Wrangler's dev registry implementation. Instead, it imports getWorkerRegistry from Miniflare to populate the connection status when printing the bindings table, and sets up a callback in Miniflare to re-print the table when the dev registry updates.
  • Fixes tail handler support over the dev registry in wrangler dev when static assets are enabled.

  • Tests
    • Tests included
    • Tests not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: no user facing changes, internal refactor only
  • Wrangler V3 Backport
    • Wrangler PR:
    • Not necessary because: Miniflare dev registry is v4 only

@changeset-bot
Copy link

changeset-bot bot commented Aug 6, 2025

🦋 Changeset detected

Latest commit: 64b0ce4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
wrangler Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch

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 Aug 6, 2025

create-cloudflare

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

@cloudflare/kv-asset-handler

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

miniflare

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

@cloudflare/pages-shared

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

@cloudflare/unenv-preset

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

@cloudflare/vite-plugin

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

@cloudflare/vitest-pool-workers

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

@cloudflare/workers-editor-shared

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

wrangler

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

commit: 64b0ce4

@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch 2 times, most recently from 3bc942f to 106e326 Compare August 6, 2025 17:15
@edmundhung edmundhung changed the base branch from main to edmundhung/DEVX-2122 August 6, 2025 19:57
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2122 branch from 4418341 to bedb0c5 Compare August 7, 2025 16:00
Base automatically changed from edmundhung/DEVX-2122 to main August 7, 2025 19:02
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch from 88d833f to 3355a10 Compare August 8, 2025 11:10
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch 3 times, most recently from b85c6e3 to 1ab8411 Compare August 11, 2025 15:26
@edmundhung edmundhung changed the base branch from main to edmundhung/improve-dev-registry-tests August 11, 2025 15:27
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch 2 times, most recently from dc25544 to f00e3e2 Compare August 12, 2025 14:45
Base automatically changed from edmundhung/improve-dev-registry-tests to main August 13, 2025 10:18
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch from 3936d04 to c30afed Compare August 13, 2025 10:38
@edmundhung edmundhung changed the base branch from main to edmundhung/registry-update-callback August 13, 2025 10:39
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch from c30afed to db72fd8 Compare August 13, 2025 10:59
@edmundhung edmundhung changed the title refactor(wrangler): switch single worker dev to use miniflare dev registry implementation refactor: migrate wrangler dev to use Miniflare dev registry Aug 13, 2025
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch from db72fd8 to 9c876ab Compare August 13, 2025 11:55
Comment on lines +709 to +713
await waitFor(async () => {
let response = await fetch(url);
expect(response.status).toBe(503);
expect(await response.text()).toBe(
'Couldn\'t find a local dev session for the "ThingEntrypoint" entrypoint of service "bound" to proxy to'
Copy link
Member Author

Choose a reason for hiding this comment

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

The Miniflare dev registry is less precise here, since it cannot distinguish between a worker that is registered without the required entrypoint and one that is not registered at all.

I believe this logic mainly exists to support older Wrangler versions before JSRPC was introduced early last year, so it should be safe to drop for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree—I don't think we need to support Wrangler v3 to v4 dev registry connections

Comment on lines 823 to 872
test("should throw if binding to version of wrangler without entrypoints support over HTTPS", async ({
dev,
isolatedDevRegistryPath,
}) => {
// Start entry worker first, so the server starts with a stubbed service not
// found binding
const { url, session } = await dev({
"wrangler.toml": dedent`
name = "entry"
main = "index.ts"
[[services]]
binding = "SERVICE"
service = "bound"
`,
"index.ts": dedent`
export default {
async fetch(request, env, ctx) {
return env.SERVICE.fetch("http://placeholder/");
}
}
`,
});
let response = await fetch(url);
expect(await response.text()).toBe(
'[wrangler] Couldn\'t find `wrangler dev` session for service "bound" to proxy to'
);

await writeFile(
path.join(isolatedDevRegistryPath, "bound"),
JSON.stringify({
protocol: "https",
mode: "local",
port: 0,
host: "localhost",
durableObjects: [],
durableObjectsHost: "localhost",
durableObjectsPort: 0,
// Intentionally omitting `entrypointAddresses`
})
);

// Wait for error to be thrown
await waitFor(() => {
const output = session.getOutput();
expect(output).toMatch(
'Cannot proxy to `wrangler dev` session for service "bound" because it uses HTTPS. Please upgrade "bound"\'s `wrangler` version, or remove the `--local-protocol`/`dev.local_protocol` option.'
);
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is removed as the miniflare dev registry does support HTTPS (#10281)

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to be a test that passes with https configured? Why do we need to remove it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the test back in b12d326 and it now passes with https configured

type WorkerRegistry,
type WorkerDefinition,
getDefaultDevRegistryPath,
getWorkerRegistry,
Copy link
Member Author

Choose a reason for hiding this comment

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

We export this method for Wrangler to read the worker registry for printing the bindings table before miniflare startup.

const message = ${
isProxyEnabled
? `\`Cannot access "\${key}" as Durable Object RPC is not yet supported between multiple dev sessions.\``
? `\`Cannot access "\${className}#\${key}" as Durable Object RPC is not yet supported between multiple dev sessions.\``
Copy link
Member Author

@edmundhung edmundhung Aug 13, 2025

Choose a reason for hiding this comment

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

Minor improvement to include the class name in the error message.
e.g. Cannot access "MyDurableObject#ping" as Durable Object RPC is not yet supported between multiple dev sessions.

Comment on lines +25 to +32
tail(events: TraceItem[]) {
// Temporary workaround: the tail events is not serializable over capnproto yet
// But they are effectively JSON, so we are serializing them to JSON and parsing it back to make it transferable.
// @ts-expect-error FIXME when https://github.com/cloudflare/workerd/pull/4595 lands
return this.env.USER_WORKER.tail(
JSON.parse(JSON.stringify(events, tailEventsReplacer), tailEventsReviver)
);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This fixes tail handler support over the dev registry in wrangler dev when static assets are enabled.

@edmundhung edmundhung marked this pull request as ready for review August 13, 2025 14:18
@edmundhung edmundhung requested a review from a team as a code owner August 13, 2025 14:19
Base automatically changed from edmundhung/registry-update-callback to main August 13, 2025 20:03
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch from 9c876ab to 17f74f7 Compare August 13, 2025 20:15
@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 edmundhung/DEVX-2124 might be a good starting point.

Notes:

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

@edmundhung edmundhung added the skip-v3-pr Skip validation of presence of a v3 backport PR label Aug 13, 2025
name,
{
className,
scriptName,
Copy link
Member Author

Choose a reason for hiding this comment

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

Before this change, we skipped passing scriptName here likely because we consider those to be external DO which would be handled by the dev registry. But that means wrangler dev multi configs mode doesn't support scriptName. This fixes that.

Copy link
Contributor

@penalosa penalosa left a comment

Choose a reason for hiding this comment

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

Fantastic work! 🎉

Comment on lines +709 to +713
await waitFor(async () => {
let response = await fetch(url);
expect(response.status).toBe(503);
expect(await response.text()).toBe(
'Couldn\'t find a local dev session for the "ThingEntrypoint" entrypoint of service "bound" to proxy to'
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree—I don't think we need to support Wrangler v3 to v4 dev registry connections

Comment on lines 823 to 872
test("should throw if binding to version of wrangler without entrypoints support over HTTPS", async ({
dev,
isolatedDevRegistryPath,
}) => {
// Start entry worker first, so the server starts with a stubbed service not
// found binding
const { url, session } = await dev({
"wrangler.toml": dedent`
name = "entry"
main = "index.ts"
[[services]]
binding = "SERVICE"
service = "bound"
`,
"index.ts": dedent`
export default {
async fetch(request, env, ctx) {
return env.SERVICE.fetch("http://placeholder/");
}
}
`,
});
let response = await fetch(url);
expect(await response.text()).toBe(
'[wrangler] Couldn\'t find `wrangler dev` session for service "bound" to proxy to'
);

await writeFile(
path.join(isolatedDevRegistryPath, "bound"),
JSON.stringify({
protocol: "https",
mode: "local",
port: 0,
host: "localhost",
durableObjects: [],
durableObjectsHost: "localhost",
durableObjectsPort: 0,
// Intentionally omitting `entrypointAddresses`
})
);

// Wait for error to be thrown
await waitFor(() => {
const output = session.getOutput();
expect(output).toMatch(
'Cannot proxy to `wrangler dev` session for service "bound" because it uses HTTPS. Please upgrade "bound"\'s `wrangler` version, or remove the `--local-protocol`/`dev.local_protocol` option.'
);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to be a test that passes with https configured? Why do we need to remove it?

// absolute resolved path
persist: localPersistencePath,
registry: input.dev?.registry,
registryPath: input.dev?.registryPath,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to rename this? We have other fileds in this object that are paths but aren't named with the suffix Path (i.e. persist above)

Copy link
Member Author

Choose a reason for hiding this comment

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

removed the path suffix in 2835b62

bindings: CfWorkerInit["bindings"];
migrations: Config["migrations"] | undefined;
workerDefinitions: WorkerRegistry | undefined | null;
devRegistryPath: string | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

As above—can we just call this registry or devRegistry?

Copy link
Member Author

Choose a reason for hiding this comment

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

Named it devRegistry in 2835b62

@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Aug 14, 2025
@edmundhung edmundhung force-pushed the edmundhung/DEVX-2124 branch from f9b0355 to 02dfaf7 Compare August 18, 2025 16:24
@edmundhung edmundhung merged commit d304055 into main Aug 19, 2025
31 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Aug 19, 2025
@edmundhung edmundhung deleted the edmundhung/DEVX-2124 branch August 19, 2025 10:55
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

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants