refactor: migrate wrangler dev to use Miniflare dev registry#10245
refactor: migrate wrangler dev to use Miniflare dev registry#10245edmundhung merged 8 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 64b0ce4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
3bc942f to
106e326
Compare
4418341 to
bedb0c5
Compare
88d833f to
3355a10
Compare
b85c6e3 to
1ab8411
Compare
dc25544 to
f00e3e2
Compare
3936d04 to
c30afed
Compare
c30afed to
db72fd8
Compare
db72fd8 to
9c876ab
Compare
| 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' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I agree—I don't think we need to support Wrangler v3 to v4 dev registry connections
| 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.' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This test is removed as the miniflare dev registry does support HTTPS (#10281)
There was a problem hiding this comment.
Can we change this to be a test that passes with https configured? Why do we need to remove it?
There was a problem hiding this comment.
Added the test back in b12d326 and it now passes with https configured
| type WorkerRegistry, | ||
| type WorkerDefinition, | ||
| getDefaultDevRegistryPath, | ||
| getWorkerRegistry, |
There was a problem hiding this comment.
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.\`` |
There was a problem hiding this comment.
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.
| 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) | ||
| ); | ||
| } |
There was a problem hiding this comment.
This fixes tail handler support over the dev registry in wrangler dev when static assets are enabled.
9c876ab to
17f74f7
Compare
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
| name, | ||
| { | ||
| className, | ||
| scriptName, |
There was a problem hiding this comment.
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.
| 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' |
There was a problem hiding this comment.
I agree—I don't think we need to support Wrangler v3 to v4 dev registry connections
| 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.' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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)
| bindings: CfWorkerInit["bindings"]; | ||
| migrations: Config["migrations"] | undefined; | ||
| workerDefinitions: WorkerRegistry | undefined | null; | ||
| devRegistryPath: string | undefined; |
There was a problem hiding this comment.
As above—can we just call this registry or devRegistry?
f9b0355 to
02dfaf7
Compare
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:
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.getWorkerRegistryfrom 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.wrangler devwhen static assets are enabled.