Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds WebSocket handler support for AWS API Gateway WebSocket APIs. The implementation mirrors the existing HTTP handler architecture, providing a consistent plugin-based middleware system for WebSocket connections.
Changes:
- Added
websocketBase.tswith WebSocket-specific request/response types and context interfaces - Added
buildWebSocket.tsimplementing the WebSocket handler middleware with plugin support - Exported new WebSocket functionality through the middleware index
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/middleware/websocketBase.ts | Defines WebSocket handler types, request/response interfaces, and plugin compatibility layer for reusing HTTP plugins with WebSocket handlers |
| src/middleware/buildWebSocket.ts | Implements the WebSocket middleware builder with plugin lifecycle management (create, begin, end, error) matching the HTTP handler pattern |
| src/middleware/index.ts | Exports the new buildWebSocket function and websocketBase types to make them available to consumers |
| src/middleware/database/index.ts | Adds SQLClient to named exports (appears unrelated to WebSocket functionality) |
| package.json | Bumps version from 0.0.74 to 0.0.75 for this feature release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/middleware/database/index.ts
Outdated
| @@ -1,4 +1,4 @@ | |||
| export { ConnectionProxy } from './connectionProxy'; | |||
| export { expressionBuilder, sql } from './sqlClient'; | |||
| export { expressionBuilder, sql, SQLClient } from './sqlClient'; | |||
There was a problem hiding this comment.
The export of SQLClient appears to be unrelated to the WebSocket handler additions described in the PR description. This change makes SQLClient available as a named export from the database module, but there's no indication in the PR description or other files that this is needed for WebSocket functionality. If this is an intentional addition that should be included, please update the PR description to mention it. Otherwise, consider removing this change and creating a separate PR for it.
There was a problem hiding this comment.
잘못 인입된 거 같아서 되돌리겠습니다
There was a problem hiding this comment.
build.ts 파일과 거의 비슷합니다.
http 핸들러만 websocket 핸들러로 바꾸었습니다.
There was a problem hiding this comment.
base.ts 파일과 거의 비슷합니다.
웹소켓 request 타입인 APIGatewayProxyWebsocketEventV2 을 받는 형태일 뿐.
다만 웹소켓 통신의 특성상 http 통신과 달리 header가 존재하지 않습니다.
그래서 문제가 있는데 아래쪽에 써두겠습니다.
| /** | ||
| * For HTTP plugin compatibility (TracerPlugin uses this). | ||
| * WebSocket events may have headers in $connect route (HTTP handshake), | ||
| * but typically don't have headers in other routes. | ||
| */ | ||
| public header(key: string): string | undefined { | ||
| const event = this.event as any; | ||
| if (event.headers) { | ||
| return event.headers[key.toLowerCase()]; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
TracerPlugin 의 begin 함수를 보면
this.client.version = request.header('X-Version') || '0.0.0';
이렇게 request.header() 함수를 씁니다.
aws sqs에 이벤트 넣을 때 클라이언트 버전을 같이 표기하기 위해서인데...
아까 말했듯 웹소켓 메세지에는 (처음 connect 과정을 제외하고) header가 없습니다.
일단 connect를 제외하고는 trace.push 했을 때 모든 버전이 0.0.0으로 표기되는 문제가 있긴 하겠으나, 아직 웹소켓 trace에서 버전을 주의깊게 봐야할 필요성은 없으므로 간단히 구현합니다.
There was a problem hiding this comment.
제 생각에는 구현이 간단하고 예상되는 부작용도 없기 때문에 포함 안할 이유가 딱히 없어 보입니다.
connect 시 결국 헤더를 쓰기는 하니까요..
constructor(event: APIGatewayProxyWebsocketEventV2, context: Context) {
// ... 기존 코드
const ev = this.event as any;
if (ev.headers) {
const normalized: Record<string, string | undefined> = {};
for (const key of Object.keys(ev.headers)) {
normalized[key.toLowerCase()] = ev.headers[key];
}
ev.headers = normalized;
}
}이렇게 몇 줄 추가면 base.ts 와 동일 패턴이 되고 버전도 올바르게 보일 텐데, 추가하면 좋지 않을까요?
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
astron8t-voyagerx
left a comment
There was a problem hiding this comment.
클로드가 다 잘 짜여져 있대요.. (저도 이상한 부분 없이 잘 읽혔습니다..)
코멘트 하나만 확인해 주세요!
| /** | ||
| * For HTTP plugin compatibility (TracerPlugin uses this). | ||
| * WebSocket events may have headers in $connect route (HTTP handshake), | ||
| * but typically don't have headers in other routes. | ||
| */ | ||
| public header(key: string): string | undefined { | ||
| const event = this.event as any; | ||
| if (event.headers) { | ||
| return event.headers[key.toLowerCase()]; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
제 생각에는 구현이 간단하고 예상되는 부작용도 없기 때문에 포함 안할 이유가 딱히 없어 보입니다.
connect 시 결국 헤더를 쓰기는 하니까요..
constructor(event: APIGatewayProxyWebsocketEventV2, context: Context) {
// ... 기존 코드
const ev = this.event as any;
if (ev.headers) {
const normalized: Record<string, string | undefined> = {};
for (const key of Object.keys(ev.headers)) {
normalized[key.toLowerCase()] = ev.headers[key];
}
ev.headers = normalized;
}
}이렇게 몇 줄 추가면 base.ts 와 동일 패턴이 되고 버전도 올바르게 보일 텐데, 추가하면 좋지 않을까요?
API Gateway 웹소켓을 위한 웹소켓 핸들러를 추가합니다.
buildWebSocket.ts 파일은 기존 build.ts 파일을,
websocketBase.ts 파일은 base.ts 파일을 많이 참고하여 거의 비슷하게 작성했습니다.