feat(miniflare): proxying service binding RPC calls through capnweb#14447
feat(miniflare): proxying service binding RPC calls through capnweb#14447edmundhung wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 247181f The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
edmundhung
added
ci:e2e
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This adds an unsafeCapnwebRpcProxy option to Miniflare that routes Worker service-binding JS RPC calls through a capnweb WebSocket session, while keeping fetch()/legacy bindings on the existing proxy.
Ranked issues:
- (medium)
#getProxyno longer surfaces the "No binding named X found" error for capnweb bindings, becausegetEnvBinding()always returns a stub (neverundefined). - (low) The
gettrap drops thereceiverargument on the rpcProxy path, inconsistent with thefetchProxypath.
Details in inline comments.
| ? await ( | ||
| await this.#getCapnwebProxyClient() | ||
| ).getEnvBinding(proxyBindingName) | ||
| : proxyClient.env[proxyBindingName]; |
There was a problem hiding this comment.
When useCapnwebProxy is true, getEnvBinding(proxyBindingName) returns a capnweb stub that is never undefined, even for a binding/worker name that doesn't exist. That means the if (proxy === undefined) guard below (and its helpful No ... binding named ... TypeError) is silently bypassed for capnweb bindings, so an invalid binding name now returns a broken stub instead of throwing.
getBindings and getWorker avoid this by gating on the resolved fetchProxy first (fetchProxy !== undefined && ...). Consider doing the same here — resolve the legacy proxy first and only swap in the capnweb binding when the fetch proxy exists:
| : proxyClient.env[proxyBindingName]; | |
| const fetchProxy = proxyClient.env[proxyBindingName]; | |
| const useCapnwebProxy = | |
| fetchProxy !== undefined && | |
| pluginName === CORE_PLUGIN_NAME && | |
| this.#isCapnwebProxyBinding( | |
| pluginName, | |
| bindingName, | |
| this.#workerOpts[this.#findAndAssertWorkerIndex(workerName)] | |
| ); | |
| const proxy = useCapnwebProxy | |
| ? await ( | |
| await this.#getCapnwebProxyClient() | |
| ).getEnvBinding(proxyBindingName) | |
| : fetchProxy; |
| if (prop === "fetch" || prop === util.inspect.custom) { | ||
| return Reflect.get(Object(fetchProxy), prop, receiver); | ||
| } | ||
| return Reflect.get(Object(rpcProxy), prop); |
There was a problem hiding this comment.
The fetchProxy branch above forwards receiver, but this branch calls Reflect.get(Object(rpcProxy), prop) without it. For consistency (and so getters resolve this against the proxy), pass receiver here too:
| return Reflect.get(Object(rpcProxy), prop); | |
| return Reflect.get(Object(rpcProxy), prop, receiver); |
|
Review posted successfully to PR #14447 with two inline suggestion comments. Summary of feedback:
|
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
2 participants
Fixes #[insert GH or internal issue link(s)].
Describe your change...
A picture of a cute animal (not mandatory, but encouraged)