From 22decddb6f6ed90972c26e7b320263aefdc2e5f0 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Wed, 15 Apr 2026 21:39:12 +0200 Subject: [PATCH] refactor(cli): fix onerror ErrorEvent handling, DRY adapter types, simplify factory --- ui/cli/src/client/lifecycle.ts | 11 ++------ ui/cli/src/client/ws-adapter.ts | 37 +++++++++++-------------- ui/cli/tests/lifecycle.test.ts | 5 +--- ui/cli/tests/output.test.ts | 5 +--- ui/cli/tests/ws-adapter.test.ts | 34 +++++++++++++++-------- ui/common/tests/WebSocketClient.test.ts | 5 +--- 6 files changed, 45 insertions(+), 52 deletions(-) diff --git a/ui/cli/src/client/lifecycle.ts b/ui/cli/src/client/lifecycle.ts index 343e3238..bf1f69e7 100644 --- a/ui/cli/src/client/lifecycle.ts +++ b/ui/cli/src/client/lifecycle.ts @@ -8,7 +8,6 @@ import { type UIServerConfig, WebSocketClient, type WebSocketFactory, - type WebSocketLike, } from 'ui-common' import { WebSocket as WsWebSocket } from 'ws' @@ -17,11 +16,8 @@ import type { Formatter } from '../output/formatter.js' import { ConnectionError } from './errors.js' import { createWsAdapter } from './ws-adapter.js' -const createWsFactory = (): WebSocketFactory => { - return (url: string, protocols: string | string[]): WebSocketLike => { - return createWsAdapter(new WsWebSocket(url, protocols)) - } -} +const wsFactory: WebSocketFactory = (url, protocols) => + createWsAdapter(new WsWebSocket(url, protocols)) let activeClient: undefined | WebSocketClient let activeSpinner: null | ReturnType | undefined @@ -38,8 +34,7 @@ export interface ExecuteOptions { export const executeCommand = async (options: ExecuteOptions): Promise => { const { config, formatter, payload, procedureName, timeoutMs } = options - const factory = createWsFactory() - const client = new WebSocketClient(factory, config, timeoutMs) + const client = new WebSocketClient(wsFactory, config, timeoutMs) const { url } = client const isInteractive = process.stderr.isTTY diff --git a/ui/cli/src/client/ws-adapter.ts b/ui/cli/src/client/ws-adapter.ts index bdb7fe1c..0408153f 100644 --- a/ui/cli/src/client/ws-adapter.ts +++ b/ui/cli/src/client/ws-adapter.ts @@ -17,10 +17,10 @@ const toDataString = (data: WsWebSocket.Data): string => { } export const createWsAdapter = (ws: WsWebSocket): WebSocketLike => { - let onmessageCallback: ((event: { data: string }) => void) | null = null - let onerrorCallback: ((event: { error: unknown; message: string }) => void) | null = null - let oncloseCallback: ((event: { code: number; reason: string }) => void) | null = null - let onopenCallback: (() => void) | null = null + let onmessageCallback: WebSocketLike['onmessage'] = null + let onerrorCallback: WebSocketLike['onerror'] = null + let oncloseCallback: WebSocketLike['onclose'] = null + let onopenCallback: WebSocketLike['onopen'] = null ws.onmessage = event => { if (onmessageCallback != null) { @@ -31,15 +31,10 @@ export const createWsAdapter = (ws: WsWebSocket): WebSocketLike => { ws.onerror = event => { if (onerrorCallback != null) { - let error: Error - let message: string - if (event instanceof Error) { - error = event - message = event.message - } else { - message = typeof event === 'string' ? event : 'Unknown error' - error = new Error(message) - } + const raw = event as { error?: unknown; message?: string } + const error = + raw.error instanceof Error ? raw.error : new Error(raw.message ?? 'Unknown error') + const message = raw.message ?? error.message onerrorCallback({ error, message }) } } @@ -60,32 +55,32 @@ export const createWsAdapter = (ws: WsWebSocket): WebSocketLike => { close (code?: number, reason?: string): void { ws.close(code, reason) }, - get onclose (): ((event: { code: number; reason: string }) => void) | null { + get onclose () { return oncloseCallback }, - set onclose (callback: ((event: { code: number; reason: string }) => void) | null) { + set onclose (callback) { oncloseCallback = callback }, - get onerror (): ((event: { error: unknown; message: string }) => void) | null { + get onerror () { return onerrorCallback }, - set onerror (callback: ((event: { error: unknown; message: string }) => void) | null) { + set onerror (callback) { onerrorCallback = callback }, - get onmessage (): ((event: { data: string }) => void) | null { + get onmessage () { return onmessageCallback }, - set onmessage (callback: ((event: { data: string }) => void) | null) { + set onmessage (callback) { onmessageCallback = callback }, - get onopen (): (() => void) | null { + get onopen () { return onopenCallback }, - set onopen (callback: (() => void) | null) { + set onopen (callback) { onopenCallback = callback }, diff --git a/ui/cli/tests/lifecycle.test.ts b/ui/cli/tests/lifecycle.test.ts index 60c41f92..b8a1e740 100644 --- a/ui/cli/tests/lifecycle.test.ts +++ b/ui/cli/tests/lifecycle.test.ts @@ -1,7 +1,4 @@ -/** - * @file Unit tests for CLI lifecycle and error types - * @description Tests for connection lifecycle management and error handling - */ +/** @file Unit tests for CLI lifecycle and error types */ import assert from 'node:assert' import { describe, it } from 'node:test' diff --git a/ui/cli/tests/output.test.ts b/ui/cli/tests/output.test.ts index 2120004a..ef246092 100644 --- a/ui/cli/tests/output.test.ts +++ b/ui/cli/tests/output.test.ts @@ -1,7 +1,4 @@ -/** - * @file Unit tests for CLI output formatters (JSON and table) - * @description Tests for JSON and table output formatting functions - */ +/** @file Unit tests for CLI output formatters (JSON and table) */ import assert from 'node:assert' import { describe, it } from 'node:test' diff --git a/ui/cli/tests/ws-adapter.test.ts b/ui/cli/tests/ws-adapter.test.ts index 1e50e91f..2a38f635 100644 --- a/ui/cli/tests/ws-adapter.test.ts +++ b/ui/cli/tests/ws-adapter.test.ts @@ -1,7 +1,4 @@ -/** - * @file Unit tests for the WebSocket adapter (ws → WebSocketLike) - * @description Tests for converting ws library WebSocket to WebSocketLike interface - */ +/** @file Unit tests for the WebSocket adapter (ws → WebSocketLike) */ import type { WebSocket } from 'ws' @@ -136,7 +133,6 @@ await describe('WS Adapter', async () => { await it('should forward onerror event with error shape', () => { const mockWs = createMockWs() - const adapter = createWsAdapter(mockWs as unknown as WebSocket) let receivedError: unknown @@ -146,13 +142,14 @@ await describe('WS Adapter', async () => { receivedMessage = event.message } - const testError = new Error('connection failed') - mockWs.onerror?.(testError) + const cause = new Error('connection failed') + // Simulate ws ErrorEvent (has .error and .message properties) + mockWs.onerror?.({ error: cause, message: 'connection failed' }) assert.ok(receivedError instanceof Error) - // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion - const error = receivedError as Error - assert.strictEqual(error.message, 'connection failed') + if (receivedError instanceof Error) { + assert.strictEqual(receivedError.message, 'connection failed') + } assert.strictEqual(receivedMessage, 'connection failed') }) @@ -164,7 +161,7 @@ await describe('WS Adapter', async () => { receivedMessage = event.message } mockWs.onerror?.('connection refused') - assert.strictEqual(receivedMessage, 'connection refused') + assert.strictEqual(receivedMessage, 'Unknown error') }) await it('should forward onerror with fallback for unknown event type', () => { @@ -178,6 +175,21 @@ await describe('WS Adapter', async () => { assert.strictEqual(receivedMessage, 'Unknown error') }) + await it('should forward onerror when event has error but no message', () => { + const mockWs = createMockWs() + const adapter = createWsAdapter(mockWs as unknown as WebSocket) + let receivedMessage = '' + let receivedError: unknown + adapter.onerror = event => { + receivedError = event.error + receivedMessage = event.message + } + const cause = new Error('ECONNREFUSED') + mockWs.onerror?.({ error: cause }) + assert.ok(receivedError instanceof Error) + assert.strictEqual(receivedMessage, 'ECONNREFUSED') + }) + await it('should forward onclose event with code and reason', () => { const mockWs = createMockWs() mockWs.readyState = WebSocketReadyState.CLOSED diff --git a/ui/common/tests/WebSocketClient.test.ts b/ui/common/tests/WebSocketClient.test.ts index ec9422c1..24065011 100644 --- a/ui/common/tests/WebSocketClient.test.ts +++ b/ui/common/tests/WebSocketClient.test.ts @@ -1,7 +1,4 @@ -/** - * @file Unit tests for the SRPC WebSocket client - * @description Tests for WebSocketClient connection, request/response handling, and timeout validation - */ +/** @file Unit tests for the SRPC WebSocket client */ import assert from 'node:assert' import { describe, it } from 'node:test' -- 2.53.0