Skip to content

feat(client): add DevframeRpcClient.close() - #175

Merged
antfu merged 1 commit into
devframes:mainfrom
dvcolomban:feat/client-close
Aug 7, 2026
Merged

feat(client): add DevframeRpcClient.close()#175
antfu merged 1 commit into
devframes:mainfrom
dvcolomban:feat/client-close

Conversation

@dvcolomban

@dvcolomban dvcolomban commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

What

DevframeRpcClient has no close()/dispose() of any kind — a connection lives until the process ends, with no way for a caller to tear it down. The server transport already has the symmetric piece: WsRpcTransport.close() (attachWsRpcTransport) detaches upgrade routing, force-closes every connected peer, and closes any server it created itself. The client side has never had an equivalent.

This bites any caller that races a connection attempt against its own deadline (Promise.race([connectDevframe(...), timeout])) — the deadline can't cancel the loser, so a slow-to-resolve attempt becomes a fully connected, unreferenced client with nothing able to close it. That's the concrete case that surfaced this: @contentsquare/devkit carries a local patch adding this exact method so its connection-deadline and disconnect() paths can close what they no longer want, rather than leaking it.

createWsRpcChannel returns close(), closing the underlying WebSocket — widening its return type to ChannelOptions & { close: () => void }, since birpc's own ChannelOptions has no teardown of its own. createWsRpcClientMode hoists its channel to a local so it can reach it, and exposes close() on DevframeRpcClientMode. DevframeRpcClient.close() delegates to the mode. Purely additive: no behavior change for anyone not calling it.

Why a no-op close() on the static backend, not an optional one

createStaticRpcClientMode has no live socket — every call is a local fetch — so its close() does nothing. Making it optional instead (close?: () => void) would push the "does this mode support closing" question onto every caller of DevframeRpcClientMode/DevframeRpcClient, for no real benefit: the two modes already union-compatible on every other method this way (requestTrustWithCode, ensureTrusted, …), and a no-op is exactly as safe to call as a real one.

Why not just expose the WebSocket

Handing back the raw socket would let a caller close it out from under the channel without the channel's own bookkeeping (pendingRequestMethods, the queued-send cleanup in post) ever running — the same class of bug #165 declined to reintroduce by not exposing httpServer on the server side. A narrow close() matches that precedent and the shape WsRpcTransport.close() already established server-side.

Why close() doesn't also reject pending calls

It doesn't need to. Closing the socket fires the channel's own close listener, which createWsRpcClientMode already wires to onDisconnectedrejectAllPending(...) — the exact path a real disconnect takes. Adding a second, separate rejection here would just race the first.

Note for maintainers

Opening as draft since I'm an external consumer proposing this from downstream need, not a maintainer. Two things I'd rather leave to you than decide unilaterally:

  • Naming. close() mirrors the server transport's own WsRpcTransport.close(), but dispose()/Symbol.dispose are also reasonable given DevframeRpcClient is a resource with a lifetime. Happy to rename either way.
  • Async or sync. WsRpcTransport.close() is async (it awaits ownedServer.close()); this one is sync, since WebSocket.close() itself doesn't return anything to await and there's nothing else here to wait on. Flagging the asymmetry in case you'd rather keep the two close()s the same shape regardless.

Tests

  • ws-client.test.ts (new): close() closes the underlying WebSocket.
  • rpc-ws-status.test.ts: createWsRpcClientMode's close() closes its socket.
  • rpc.test.ts: getDevframeRpcClient's close() closes the socket on a websocket backend, and is a no-op (not a throw) on a static one.
  • rpc-auth-gate.test.ts: updated its hand-typed DevframeRpcClientMode mock for the new required field.

pnpm --filter devframe exec tsc --noEmit clean. pnpm exec vitest run --project devframe — 51 files, 463 tests (was 459), all green. Also ran --project @devframes/hub --project @devframes/hub-ui (129 tests) and tsc --noEmit across @devframes/hub, @devframes/hub-ui, @devframes/nuxt — the packages consuming DevframeRpcClient — clean. eslint clean on every touched file.

@netlify

netlify Bot commented Aug 6, 2026

Copy link
Copy Markdown

Deploy Preview for devfra ready!

Name Link
🔨 Latest commit 5c36bbb
🔍 Latest deploy log https://app.netlify.com/projects/devfra/deploys/6a74cfa7ada06400085fc50f
😎 Deploy Preview https://deploy-preview-175--devfra.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@dvcolomban
dvcolomban marked this pull request as ready for review August 6, 2026 16:15
Copilot AI lite review requested due to automatic review settings August 6, 2026 16:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds an explicit close() API to the Devframe RPC client so consumers can deterministically tear down websocket connections (mirroring server transport close behavior).

Changes:

  • Extend the public DevframeRpcClient / DevframeRpcClientMode types with close().
  • Add close() support to the websocket channel/transport and wire it through websocket/static modes.
  • Add unit tests ensuring close() closes the underlying websocket and is a no-op for static mode.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/snapshots/tsnapi/devframe/client.snapshot.d.ts Updates snapshot typings to include close() in the public client/mode interfaces.
packages/devframe/src/rpc/transports/ws-client.ts Extends ws channel factory to return a closable channel and implements close().
packages/devframe/src/rpc/transports/ws-client.test.ts Adds a focused unit test verifying channel close() calls WebSocket.close().
packages/devframe/src/client/rpc.ts Exposes rpc.close() and threads close() through to the selected mode.
packages/devframe/src/client/rpc.test.ts Adds integration tests for websocket close + static backend no-op close.
packages/devframe/src/client/rpc-ws.ts Hoists channel creation so mode can expose close() that closes the socket.
packages/devframe/src/client/rpc-ws-status.test.ts Adds a test verifying mode close() closes the underlying socket.
packages/devframe/src/client/rpc-static.ts Implements close() as a no-op for static mode.
packages/devframe/src/client/rpc-auth-gate.test.ts Updates the mocked mode to satisfy the new required close() property.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread packages/devframe/src/client/rpc.ts
Comment thread packages/devframe/src/client/rpc.ts
@dvcolomban
dvcolomban marked this pull request as draft August 6, 2026 16:35
@dvcolomban
dvcolomban marked this pull request as ready for review August 6, 2026 16:47
Copilot AI review requested due to automatic review settings August 6, 2026 16:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

packages/devframe/src/client/rpc.ts:210

  • The JSDoc rationale for why DevframeRpcClient.close is optional points at DevframeRpcClientMode implementations predating the method, but that rationale actually applies to DevframeRpcClientMode.close (and/or older DevframeRpcClient mocks), not specifically to the DevframeRpcClient property itself. Rewording this section would make the public API docs less confusing, especially since getDevframeRpcClient() always attaches a close function and treats a missing mode.close as a no-op.
   * Optional so a `DevframeRpcClientMode` implemented before this method existed — a custom
   * transport, a hand-typed mock — still satisfies the interface; an absent `close` is treated
   * as nothing to close.
   */

`DevframeRpcClient` has no `close()`/`dispose()` of any kind — a connection
lives until the process ends, with no way for a caller to tear it down. The
server transport already has the symmetric piece: `WsRpcTransport.close()`
(attachWsRpcTransport) detaches upgrade routing, force-closes every connected
peer, and closes any server it created itself. The client side has never had
an equivalent.

This bites any caller that races a connection attempt against its own
deadline (`Promise.race([connectDevframe(...), timeout])`) — the deadline
can't cancel the loser, so a slow-to-resolve attempt becomes a fully
connected, unreferenced client with nothing able to close it.

## Changes

- `createWsRpcChannel` (rpc/transports/ws-client.ts) returns `close()`,
  closing the underlying `WebSocket`. Widens its return type to
  `ChannelOptions & { close: () => void }`, since birpc's own `ChannelOptions`
  has no teardown of its own.
- `createWsRpcClientMode` hoists its channel to a local so it can close it,
  and exposes `close()` on `DevframeRpcClientMode`.
- `createStaticRpcClientMode` gets a no-op `close()` — a static backend has
  no live socket, every call is a local fetch — so the two modes stay
  union-compatible.
- `DevframeRpcClient.close()` delegates to the mode. No behavior change for
  anyone not calling it.

## Tests

- `ws-client.test.ts` (new): `close()` closes the underlying `WebSocket`.
- `rpc-ws-status.test.ts`: `createWsRpcClientMode`'s `close()` closes its
  socket.
- `rpc.test.ts`: `getDevframeRpcClient`'s `close()` closes the socket on a
  `websocket` backend, and is a no-op (not a throw) on a `static` one.
- `rpc-auth-gate.test.ts`: updated its hand-typed `DevframeRpcClientMode` mock
  for the new required field.

`pnpm --filter devframe exec tsc --noEmit` clean. `pnpm exec vitest run
--project devframe` — 51 files, 463 tests (was 459), all green. Also ran
`--project @devframes/hub --project @devframes/hub-ui` (129 tests) and
`tsc --noEmit` across `@devframes/hub`, `@devframes/hub-ui`, `@devframes/nuxt`
— the packages consuming `DevframeRpcClient` — clean. `eslint` clean on every
touched file.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

test: update tsnapi snapshot for DevframeRpcClient.close()

CI's exports.test.ts snapshots each package's public .d.ts surface; adding
close() to DevframeRpcClient/DevframeRpcClientMode is a real, intentional
API change, so devframe/client.snapshot.d.ts needs to reflect it.

Verified with a full `pnpm run build` at the repo root first, so tsnapi reads
every package's real dist output rather than erroring on a missing one — this
is the only snapshot that changed; close() doesn't ripple into any plugin's
or @devframes/hub's public surface.

`pnpm exec vitest run` — 101 files, 1087 tests, all green (was 1 failing).
`pnpm run lint` and `pnpm run typecheck` (25 packages via turbo) both clean.

fix(client): make DevframeRpcClient.close() optional

Copilot's review on this PR flagged both close() additions
(DevframeRpcClient and DevframeRpcClientMode) as a breaking change:
adding a required property to an exported, externally-implementable
interface breaks any existing consumer that hand-types a mock/adapter
against it without close() — as this repo's own rpc-auth-gate.test.ts
mock did until now.

Made close?: () => void on both interfaces instead. Every factory this
repo owns (createWsRpcClientMode, createStaticRpcClientMode) still
provides it unconditionally, so nothing here loses close(); only the
type requirement is relaxed. getDevframeRpcClient's close now calls
mode.close?.() to match.

Updated the three call sites that invoked close() directly against the
now-optional type (rpc.test.ts, rpc-ws-status.test.ts) to close?.(), and
removed the auth-gate mock's close() entirely rather than keep it,
since a mode without one is now exactly the scenario this is meant to
keep working — a new test asserts rpc.close() against it doesn't throw.

Verified with a full `pnpm run build` + `pnpm exec vitest run --project
tests -u`: only the client tsnapi snapshot changed (close: () => void
-> close?: () => void), nothing else. Full suite: 101 files, 1088 tests
(was 1087), all green. `pnpm run typecheck` and `pnpm run lint` clean
across all 22 packages.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@antfu
antfu merged commit 02967d8 into devframes:main Aug 7, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants