- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1k
refactor: migrate wrangler dev to use Miniflare dev registry #10245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
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?
There was a problem hiding this comment.
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
| 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.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, | 
There was a problem hiding this comment.
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.\`` | 
There was a problem hiding this comment.
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.
| 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.
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.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! 🎉
| 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.
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
| 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.
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, | 
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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; | 
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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.