alkdev/pubsub
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs: complete review-websocket-adapters checkpoint

Browse Source
This commit is contained in:
glm-5.1
2026-05-08 07:42:18 +00:00
parent 5e49412364
commit f7d8aecb3a
1 changed files with 105 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

116
tasks/010-review-websocket-adapters.md
View File

@@ -1,7 +1,7 @@
--- ---
id: review-websocket-adapters id: review-websocket-adapters
name: Review WebSocket client and server adapters name: Review WebSocket client and server adapters
status: pending status: completed
depends_on: [websocket-client-tests, websocket-server-tests] depends_on: [websocket-client-tests, websocket-server-tests]
scope: narrow scope: narrow
risk: low risk: low
@@ -22,13 +22,13 @@ Verify:
## Acceptance Criteria ## Acceptance Criteria
- [ ] `npm run build` passes cleanly - [x] `npm run build` passes cleanly
- [ ] `npm run lint` passes - [x] `npm run lint` passes
- [ ] `npm test` passes with all core + Redis + WebSocket tests - [x] `npm test` passes with all core + Redis + WebSocket tests (181/181)
- [ ] WebSocket client subscription forwarding matches ADR-003 - [x] WebSocket client subscription forwarding matches ADR-003
- [ ] WebSocket server fan-out and subscription tracking matches architecture spec - [x] WebSocket server fan-out and subscription tracking matches architecture spec
- [ ] No unnecessary comments in source (project convention) - [x] No unnecessary comments in source (project convention)
- [ ] License headers present where needed - [x] License headers present where needed
## References ## References
@@ -36,10 +36,104 @@ Verify:
- docs/architecture/event-targets/websocket-server.md - docs/architecture/event-targets/websocket-server.md
- docs/architecture/decisions/003-subscription-control-protocol.md - docs/architecture/decisions/003-subscription-control-protocol.md
## Notes ## Review Report
> To be filled by implementation agent ### 1. Build / Lint / Tests
| Check | Result |
|-------|--------|
| `npm run build` | Pass — tsup produces ESM + CJS + declarations for all 4 entry points |
| `npm run lint` (`tsc --noEmit`) | Pass — no type errors |
| `npm test` | Pass — 181/181 tests (46 ws-server, 36 ws-client, 25 redis, 10 redis-integration, 11 create_pubsub, 53 operators) |
### 2. Client Adapter (`event-target-websocket-client.ts`) — Spec Alignment
| Spec Requirement | Implementation | Status |
|-----------------|----------------|--------|
| Takes a single connected `WebSocket` | `createWebSocketClientEventTarget<TEvent>(ws: WebSocket)` | Pass |
| `dispatchEvent` sends `JSON.stringify(event.detail)` and returns `true` | Line 89: `ws.send(JSON.stringify(event.detail)); return true;` | Pass |
| `addEventListener` registers local listener + sends `__subscribe` on first listener per topic | Lines 38-56: `addCallback` tracks per-topic `Set<EventListener>`, sends `__subscribe` when `isFirst` | Pass |
| `removeEventListener` removes local listener + sends `__unsubscribe` when last listener removed | Lines 58-78: `removeCallback` deletes from Set, sends `__unsubscribe` only when `callbacks.size === 0` | Pass |
| `null` callback is a no-op (no listener registered, no `__subscribe` sent) | Lines 82-83, 92-96: `if (callbackOrOptions != null)` guard | Pass |
| `EventListenerObject.handleEvent` unwrapping | Lines 84, 95: `"handleEvent" in callbackOrOptions ? callbackOrOptions.handleEvent : callbackOrOptions` | Pass |
| Malformed JSON → silently ignored, warning logged | Lines 10-17: `try/catch` around `JSON.parse` with `console.warn` | Pass |
| Control events (`__subscribe`, `__unsubscribe`) from server → silently ignored | Line 19: `envelope.type.startsWith("__")` causes early return | Pass |
| Subscription reference counting (dedup) | Multiple `addEventListener` for same topic only sends one `__subscribe` (tested) | Pass |
| `ws.send()` failure → error propagates to caller | Line 89: `dispatchEvent` calls `ws.send()` directly, no try/catch — error propagates | Pass |
| Reconnection is caller-managed (new instance per connection) | Confirmed — no reconnection logic in adapter; tests verify new ws → new target → fresh `__subscribe` | Pass |
### 3. Server Adapter (`event-target-websocket-server.ts`) — Spec Alignment
| Spec Requirement | Implementation | Status |
|-----------------|----------------|--------|
| `WebSocketLike` interface (not raw `WebSocket`) | Lines 3-9: `WebSocketLike` with `send`, `close`, `bufferedAmount`, `onmessage`, `onclose` | Pass |
| `SpokeEventTarget<TEvent>` exposes `ws` property | Lines 11-13: `SpokeEventTarget` extends `TypedEventTarget<TEvent>` with `readonly ws` | Pass |
| `addConnection(ws)` / `removeConnection(ws)` | Lines 130-217, 98-128: Full lifecycle management | Pass |
| `addConnection` sets up `onmessage`/`onclose`, stores originals, calls `onConnection` | Lines 130-217 | Pass |
| `removeConnection` cleans up subs/maps, restores original handlers, does NOT close ws | Lines 98-128: No `ws.close()` call | Pass |
| `__subscribe` control event → adds ws to topic's `Set<WebSocketLike>` | Lines 153-167 | Pass |
| `__unsubscribe` control event → removes ws from topic set | Lines 170-186 | Pass |
| Control events not dispatched to local listeners | Code returns early for `__subscribe`/`__unsubscribe` before local listener dispatch | Pass |
| Invalid topic in `__subscribe` (empty or non-string) → silently ignored | Lines 154-155: `typeof topic === "string" && topic.length > 0` | Pass |
| Duplicate `__subscribe` is idempotent (Set handles dedup) | `Set<WebSocketLike>.add()` is idempotent | Pass |
| Malformed JSON → caught, warned, message ignored | Lines 143-149 | Pass |
| `dispatchEvent` → fan-out to subscribed connections + local listeners, always returns `true` | Lines 262-278 | Pass |
| Per-connection spoke target: `addEventListener`/`removeEventListener` for events from that spoke only | Lines 47-96: `createSpokeTarget` with per-connection listener map | Pass |
| Per-connection spoke `dispatchEvent` sends to that specific ws | Lines 77-91: `spoke.dispatchEvent` checks backpressure then sends | Pass |
| Backpressure: check `bufferedAmount > maxBufferedAmount` before send, close with 1013 | Lines 219-226 (`sendToConnection`) and 79-83 (spoke target) | Pass |
| Default `maxBufferedAmount` = 1,048,576 (1 MB) | Line 30 | Pass |
| `onBackpressure` called before disconnect, cannot prevent it | Lines 221-222, 81-82 | Pass |
| `ws.send()` failure → catches error, removes connection, fires `onDisconnection` | Lines 87-88 (`spoke.dispatchEvent`), 227-228 (`sendToConnection`) | Pass |
| Duplicate `addConnection` for same ws is a no-op | Line 131: `if (spokeTargets.has(ws)) return;` | Pass |
| `onclose` handler calls `removeConnection` and chains to original handler | Lines 209-214 | Pass |
### 4. ADR-003 Subscription Control Protocol Compliance
| ADR-003 Requirement | Implementation | Status |
|---------------------|----------------|--------|
| Control events use `EventEnvelope` format | Both adapters send/receive `{ type: "__subscribe"|"__unsubscribe", id: "", payload: { topic: "..." } }` | Pass |
| `id` field is `""` for control events | Lines 51, 72 (client) | Pass |
| Control events use `payload.topic` for routing, not `type:id` scoping | Client sends `payload: { topic }`, server reads `envelope.payload.topic` | Pass |
| `__`-prefixed types are reserved, not dispatched to user-facing listeners | Client: line 19 `startsWith("__")` → return; Server: lines 153-186 handle `__subscribe`/`__unsubscribe` with early return | Pass |
| Fire-and-forget semantics (no ack) | No acknowledgment mechanism in either adapter | Pass |
| Cleanup on disconnection | Server removes all subscriptions on `onclose` → `removeConnection`; Client per-connection so new instance needed | Pass |
| Reconnection requires new instance + re-subscribe | Client: each connection gets new instance, `addEventListener` triggers fresh `__subscribe` | Pass |
### 5. Test Coverage
**Client adapter (36 tests):** Covers send path, receive path, topic scoping, subscription forwarding (including dedup), unsubscribe forwarding, malformed JSON, control event ignoring, envelope round-trip, `null` callback, `EventListenerObject`, reconnection, connection close, and subscription reference counting.
**Server adapter (46 tests):** Covers `addConnection`/`removeConnection`, automatic cleanup on close, subscription tracking (subscribe/unsubscribe/idempotent/invalid topic), topic-based fan-out, dispatched events to local listeners, per-connection spoke targets, incoming message aggregation, backpressure (threshold, callback, disconnect, below threshold, default 1MB), send failure handling, local `addEventListener`/`removeEventListener`, `EventListenerObject`, and `onDisconnection` callback.
All test items from the architecture spec test plans are covered.
### 6. Code Conventions
- No unnecessary comments in either adapter source file — Pass
- No stray debug code — Pass
- `types.ts` retains the graphql-yoga MIT license header — Pass
### 7. Exports
| File | Status |
|------|--------|
| `src/index.ts` — barrel re-exports `createWebSocketClientEventTarget` and `createWebSocketServerEventTarget` + types | Pass |
| `tsup.config.ts` — includes both `event-target-websocket-client.ts` and `event-target-websocket-server.ts` entries | Pass |
| `package.json` exports — includes both `./event-target-websocket-client` and `./event-target-websocket-server` sub-path exports with ESM/CJS/d.ts | Pass |
| No peer deps for WebSocket adapters (correct — WebSocket is web standard) | Pass |
### 8. Minor Observations (non-blocking)
1. The client adapter uses the raw `WebSocket` type (browser global) rather than a `WebSocketLike` interface like the server. This is consistent with the spec ("No native deps — works in browsers and Node") but makes the client adapter harder to unit-test with alternative WebSocket implementations. The current tests use `as any` casts for mock WebSockets, which works but is less type-safe than the server's `WebSocketLike` pattern. This is a design choice, not a bug — the client is simpler (single connection) and the spec explicitly says "takes an already-connected WebSocket."
2. The server adapter has a subtle difference between `sendToConnection` (used for server `dispatchEvent` fan-out) and `spoke.dispatchEvent` (used for per-connection spoke sending). Both check backpressure and handle send failures, but the spoke target's `dispatchEvent` has its own inline backpressure check rather than calling `sendToConnection`. Duplicated logic, but functionally equivalent — no bug.
3. The server's `onmessage` handler checks `typeof envelope.type !== "string"` (line 151) but does NOT check for `__`-prefixed types beyond `__subscribe` and `__unsubscribe`. If a future control type like `__ping` were added, it would fall through to the regular event dispatch and be delivered to local listeners. This is fine as long as ADR-003's convention is followed — only `__subscribe` and `__unsubscribe` are defined currently. Future control types should be added to the handler.
### Verdict
**PASS** — All acceptance criteria met. Both adapters correctly implement the architecture spec and ADR-003. Tests cover all specified behaviors. Build, lint, and 181 tests pass cleanly.
## Summary ## Summary
> To be filled on completion Review completed successfully. Both WebSocket client and server adapters fully align with their architecture specs and ADR-003. Subscription control protocol (`__subscribe`/`__unsubscribe`) is correctly implemented in both directions. Topic-based fan-out, backpressure protection, error handling, and connection lifecycle management all match spec. All 181 tests pass. Two minor non-blocking observations noted (client uses raw `WebSocket` type, duplicated backpressure logic in spoke target). No blockers found.