feat(server): default resolve to app fetch .crossws#200
Conversation
Make `resolve` optional for the `crossws/server` plugin. When omitted, hooks
are resolved by calling the srvx server's own `fetch` handler and reading the
`.crossws` property off the returned `Response`, removing the boilerplate every
srvx/h3 user had to hand-write:
serve(app, { plugins: [ws()] }); // no resolve
A shared `defaultResolve(server, wsOpts)` helper is used by all six runtime
plugins. Inline hooks (`ws({ message })`) opt out of the fetch-based default and
keep running via the adapter's global-hook path with zero per-event overhead.
Throws a clear error when the server has no `fetch` handler.
Also fixes node.ts spreading `options.deno` instead of `options.node`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a server-side ChangesDefault resolve and adapter wiring
Handshake and resolver coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
`AdapterHookable.callHook` called `options.resolve(request)` on every hook event — meaning the default `crossws/server` resolver ran the app's `fetch` handler on *every message*. Memoize the resolved hooks per connection in a `WeakMap` keyed by the stable `peer.request` identity that every runtime adapter shares, so `resolve` runs at most once per connection. Adds cross-provider tests (asserted on the shared `callHook` path plus an end-to-end node run) proving `resolve`/`fetch` is not invoked per message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks.ts`:
- Around line 31-45: The resolve() result cached in `#resolveCache` can remain as
a rejected promise and poison later hooks for the same request/peer. Update the
hook resolution path in hooks.ts around resolve and `#resolveCache` so rejected
resolveHooksPromise entries are removed from the WeakMap on failure, allowing
subsequent events for that request/Peer to call resolve again and recover from
transient errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c96b393f-1bf3-4772-b3e2-495636b75503
📒 Files selected for processing (3)
docs/1.guide/7.resolver.mdsrc/hooks.tstest/server-resolve.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/1.guide/7.resolver.md
Key the resolve cache by the connection's `context` object instead of the
request. `upgrade()` creates `context` (`request.context || {}`) and every
adapter re-exposes that exact object as `peer.context`, so a single identity
spans both the `upgrade` event and every later peer event — collapsing the
previous two resolves per connection (upgrade + peer, which use different
`request` objects) into one.
`upgrade()` seeds the cache by passing `context` to `callHook`; peer events
derive the same key from `peer.context`.
Tests assert exactly one resolve/fetch spans a whole connection, including a
peer whose `request` differs from the upgrade request.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/hooks.ts (1)
42-52: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEvict rejected
resolve()results from#resolveCache.If
resolve(request)rejects, the rejected promise is cached in#resolveCacheand reused for every subsequent hook call on the same connection — a single transient resolver failure permanently breaks that connection instead of allowing retry on the next event.🛠️ Proposed fix
if (this.#resolveCache.has(cacheKey)) { resolveHooksPromise = this.#resolveCache.get(cacheKey); } else { resolveHooksPromise = resolve(request); + if (resolveHooksPromise instanceof Promise) { + resolveHooksPromise.catch(() => this.#resolveCache.delete(cacheKey)); + } this.#resolveCache.set(cacheKey, resolveHooksPromise); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks.ts` around lines 42 - 52, Evict failed hook resolution results from the shared cache in hooks.ts: when `resolve(request)` rejects in the `#resolveCache` lookup path, remove that `cacheKey` entry so a transient failure doesn’t poison future events for the same connection. Update the logic around `resolveHooksPromise`, `#resolveCache.has/get/set`, and `resolve(request)` so only successful resolutions stay cached and rejected promises are not reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/hooks.ts`:
- Around line 42-52: Evict failed hook resolution results from the shared cache
in hooks.ts: when `resolve(request)` rejects in the `#resolveCache` lookup path,
remove that `cacheKey` entry so a transient failure doesn’t poison future events
for the same connection. Update the logic around `resolveHooksPromise`,
`#resolveCache.has/get/set`, and `resolve(request)` so only successful
resolutions stay cached and rejected promises are not reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 576053c4-062b-410b-bba6-a5940273ede7
📒 Files selected for processing (2)
src/hooks.tstest/server-resolve.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/server-resolve.test.ts
A rejected `resolve` promise was memoized permanently, poisoning every later hook event on the connection and never retrying — a single transient failure (e.g. the default resolver's `fetch` rejecting once) would kill the connection. Attach a guarded `catch` that removes the failed entry from the cache so the next event re-invokes `resolve` and recovers. The catch is a separate branch and does not swallow the rejection surfaced to the current event. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The default resolver now runs the app `fetch` on every WS upgrade, which surfaced a few sharp edges when that handler misbehaves: - Normalize a *synchronous* throw from `resolve` into a rejected promise in `callHook`, so it flows through the existing eviction/`.catch` path instead of escaping as an uncaught exception on fire-and-forget event call sites (message/close/…). - Catch a failed `upgrade()` in the node adapter's `handleUpgrade` (which the server plugin invokes fire-and-forget) and fail the handshake with a 500, instead of hanging the socket and raising an unhandled rejection. - Cancel the discarded response body in the default resolver so a streaming/proxied body on the upgrade path isn't leaked per connection. - Add a compile-time exhaustiveness guard for `HOOK_NAMES` so a hook added to `Hooks` but not listed can't silently break inline-hook detection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Explain that the default resolver calls the app `fetch` with the upgrade request once per connection: attach hooks via `.crossws`, keep `fetch` side-effect-safe, and how throwing vs. returning a non-crossws Response affects the handshake. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/server-resolve.test.ts (1)
314-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: consolidate near-duplicate handshake-failure tests.
Both tests differ only in how
fetchfails (sync throw vs. rejected promise). Could parameterize withtest.eachto reduce duplication.♻️ Example consolidation
-test("a synchronously throwing app fetch fails the handshake cleanly", async () => { - ... -}); - -test("a rejecting app fetch fails the handshake cleanly", async () => { - ... -}); +test.each([ + ["synchronously throwing", () => { throw new Error("boom"); }], + ["rejecting", () => Promise.reject(new Error("boom"))], +])("a %s app fetch fails the handshake cleanly", async (_label, fetchImpl) => { + const port = await getRandomPort("localhost"); + const server = serve({ + port, + hostname: "127.0.0.1", + fetch: fetchImpl, + websocket: {}, + }); + currentServer = server; + await server.ready(); + + const client = new WebSocket(`ws://127.0.0.1:${port}/`); + const [error] = await once(client, "error"); + expect(error).toBeInstanceOf(Error); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/server-resolve.test.ts` around lines 314 - 350, Consolidate the two near-identical handshake failure cases in server-resolve.test.ts into a single parameterized test using test.each, keeping both failure modes covered while reducing duplication. Update the existing WebSocket handshake assertions around serve(), currentServer, and the default websocket resolver so the only variable is the fetch failure behavior (sync throw vs Promise rejection).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/node.ts`:
- Around line 119-129: The handshake fallback in `handleUpgrade` is too brittle
because the `catch` block discards the rejection reason and the `sendResponse`
failure can still surface as another unhandled rejection. Update the
`hooks.upgrade` try/catch to capture the error for diagnostics, then make the
500-response path best-effort by guarding or swallowing failures from
`sendResponse(socket, new Response(...))` so a closed socket does not rethrow.
Reference the `handleUpgrade` flow and the `hooks.upgrade` / `sendResponse`
calls when applying the fix.
---
Nitpick comments:
In `@test/server-resolve.test.ts`:
- Around line 314-350: Consolidate the two near-identical handshake failure
cases in server-resolve.test.ts into a single parameterized test using
test.each, keeping both failure modes covered while reducing duplication. Update
the existing WebSocket handshake assertions around serve(), currentServer, and
the default websocket resolver so the only variable is the fetch failure
behavior (sync throw vs Promise rejection).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ff20486-e642-4547-a8ec-c2e718480dff
📒 Files selected for processing (4)
src/adapters/node.tssrc/hooks.tssrc/server/_resolve.tstest/server-resolve.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/_resolve.ts
- src/hooks.ts
The default resolver now accepts either a `Response` carrying `.crossws`
or a plain `{ crossws, headers }` object from the app `fetch` handler for
upgrade requests. Optional `headers` are applied to the WebSocket
handshake response (composed with any `upgrade` hook from `.crossws`). A
real Response's own HTTP headers are still not treated as handshake
headers.
The `fetch` return type in `ServerWithWSOptions` is widened accordingly
(new exported `WSUpgradeResult` type), with the srvx `serve` wrappers
casting back to `ServerOptions`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When the app `fetch` returns a `Response` without `.crossws` on the
upgrade path, a non-`2xx` response (error or redirect, e.g.
`new Response("Unauthorized", { status: 401 })`) is now sent back to the
client and the handshake is rejected, rather than silently opening a
handler-less socket. A plain `2xx` response without hooks still upgrades
with no hooks (unchanged).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`res.ok` is only true for 2xx, so a `101` (Switching Protocols) response — e.g. the one Cloudflare's `WebSocketPair` produces — would be rendered as an error by the non-ok render path. Exclude `101` so it proceeds with the upgrade (hook-less when no `.crossws`) instead of failing the handshake. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Makes
resolveoptional for thecrossws/serverplugin, and makes hook resolution run exactly once per connection.Every srvx/h3 user currently hand-writes the same
resolveboilerplate to wire crossws back into their own app:After this change the common case is just:
When
resolveis omitted, hooks are resolved by calling the srvx server's ownfetchhandler and reading the.crosswsproperty off the returnedResponse— the srvx-level convention for attaching WebSocket hooks. The plugin already hasserver.options.fetch, so it can do this itself.1. Default
resolve(src/server/)src/server/_resolve.ts— shareddefaultResolve(server, wsOpts)helper:resolveunchanged when provided.undefinedwhen inline hooks (ws({ message })) are present, so those keep running via the adapter's global-hook path with zero per-event overhead (inline and app-resolved modes are mutually exclusive — decision point 1 in the task)..crosswsoff the app's fetchResponse.fetchhandler:[crossws] server has no fetch handler to resolve WebSocket hooks from.bun.ts,deno.ts,cloudflare.ts,node.ts,bunny.ts— swapresolve: wsOpts.resolve→resolve: defaultResolve(server, wsOpts).default.ts(SSE) — refactored so the adapter is built inside the(server) => {}closure (needed to reachserver.options.fetch); SSEconsole.warnpreserved._types.ts— updatedWSOptions.resolveJSDoc to document the new default.node.tswas spreadingwsOpts.options?.denoinstead ofwsOpts.options?.node.2. Resolve exactly once per connection (
src/hooks.ts)AdapterHookable.callHookpreviously invokedoptions.resolve(request)on every hook event. Since every adapter routesmessage/open/close/drain/errorthroughcallHook, the default resolver'sfetch(req)was firing on every message.Now the resolved hooks are memoized in a
WeakMapkeyed by the connection'scontextobject — whichupgrade()creates (request.context || {}) and every adapter re-exposes verbatim aspeer.context. That single shared identity spans both theupgradeevent and every later peer event, so oneresolvecall serves the whole connection (the earlier approach keyed byrequestresolved twice, because adapters give the peer a different request object than the upgrade request). Keyed weakly, so entries are freed when the connection is GC'd.This also addresses the per-event cost noted as out-of-scope (decision point 2) in the task.
Tests (
test/server-resolve.test.ts)End-to-end via the node server +
wsclient, plus unit tests on the sharedAdapterHookable.callHookpath (the single choke point every provider funnels through, so the guarantee holds cross-provider):resolve→ hooks resolved from the fetch handler's.crossws(open/message/close fire).resolvestill takes precedence.resolveor fetch.crossws.server.options.fetchthrows the guard error.resolveruns once per connection, not per message (20 messages + open + close ⇒ one resolve; a second connection ⇒ two).upgrade+ peer events share a single resolve viacontext(peer'srequestdeliberately differs from the upgrade request).fetchserves a whole connection across 21 messages.Each per-connection test was confirmed to fail on the pre-fix code path and pass after — locking in the guarantee. Full suite: 230 passed / 6 skipped.
lint+typecheckclean.Docs
Added a "Server plugin" section to the resolver guide (
docs/1.guide/7.resolver.md) documentingws()with noresolve,resolveas optional/custom-only, and that resolution is cached per connection.Notes / follow-ups (out of scope)
crossws/serverdefault path) reconstruct context on wake, so a woken peer resolves again — the in-memoryWeakMapcan't survive hibernation regardless.feat/ez-ws) docs/examples andcrosswspeer-range bump once released.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
fetchresponse usingcrosswswhen no custom resolver is provided.Bug Fixes
Documentation
Tests