Skip to content

Commit 95ad80c

Browse files
bartlomiejubartlomieju
committed
fix: address WebSocket PR review feedback
- Add doc comment on isWebSocketHandlers explaining duck-typing dispatch and when it would need updating - Remove sanitizeOps/sanitizeResources: false from integration tests (both sanitizers pass cleanly with defaults) - Add test for bare-mode ctx.upgrade() with protocol option - Clarify bare-mode options example in docs and note that app.ws() is managed-mode only
1 parent a3785d3 commit 95ad80c

3 files changed

Lines changed: 180 additions & 147 deletions

File tree

‎docs/latest/examples/websockets.md‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -102,25 +102,31 @@ export const handlers = define.handlers({
102102
Both modes accept an options object to configure the underlying WebSocket:
103103

104104
```ts
105-
// Managed mode
105+
// Managed mode — pass handlers first, then options
106106
ctx.upgrade(handlers, {
107107
idleTimeout: 60, // close if no ping received within 60s (default: 120)
108108
protocol: "graphql-ws", // sub-protocol to negotiate
109109
});
110110

111-
// Bare mode
112-
const { socket, response } = ctx.upgrade({
113-
idleTimeout: 60,
114-
protocol: "graphql-ws",
115-
});
111+
// Bare mode — pass options without handlers to get the raw socket back
112+
const { socket, response } = ctx.upgrade({ idleTimeout: 60 });
116113
```
117114

115+
> **How does Fresh tell the two calls apart?** The first argument is treated as
116+
> managed-mode handlers when it contains at least one function-valued handler
117+
> key (`open`, `message`, `close`, or `error`). A plain options object only has
118+
> non-function fields (`idleTimeout`, `protocol`), so it always enters bare
119+
> mode.
120+
118121
The same options can be passed to `app.ws()`:
119122

120123
```ts
121124
app.ws("/ws", handlers, { idleTimeout: 60 });
122125
```
123126

127+
> `app.ws()` always uses managed mode. For bare-mode access to the raw socket,
128+
> use `app.get()` with `ctx.upgrade()` instead.
129+
124130
## Error handling
125131

126132
If a non-WebSocket request hits a WebSocket route, `ctx.upgrade()` throws an

‎packages/fresh/src/context.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,17 @@ export interface WebSocketUpgradeOptions {
5858
protocol?: string;
5959
}
6060

61+
/**
62+
* Duck-type check: treats the argument as managed-mode handlers when at least
63+
* one of the handler keys (`open`, `message`, `close`, `error`) is a
64+
* function. This works because {@link WebSocketUpgradeOptions} only has
65+
* non-function fields (`idleTimeout`, `protocol`), so a plain options object
66+
* will never match.
67+
*
68+
* If `WebSocketUpgradeOptions` ever gains a function-valued field whose name
69+
* collides with a handler key, this guard must be updated (or replaced with a
70+
* branded/sentinel approach).
71+
*/
6172
function isWebSocketHandlers(
6273
value: unknown,
6374
): value is WebSocketHandlers {

‎packages/fresh/src/websocket_test.ts‎

Lines changed: 157 additions & 141 deletions
Original file line numberDiff line numberDiff line change
@@ -20,161 +20,177 @@ Deno.test("app.ws() - throws 400 on non-WebSocket request", async () => {
2020
expect(res.status).toEqual(400);
2121
});
2222

23-
Deno.test({
24-
name: "ctx.upgrade() - managed echo",
25-
sanitizeOps: false,
26-
sanitizeResources: false,
27-
async fn() {
28-
const app = new App()
29-
.get("/ws", (ctx) =>
30-
ctx.upgrade({
31-
message(socket, event) {
32-
socket.send(`echo: ${event.data}`);
33-
},
34-
}));
35-
36-
const ac = new AbortController();
37-
const server = Deno.serve({
38-
hostname: "127.0.0.1",
39-
port: 0,
40-
signal: ac.signal,
41-
onListen: () => {},
42-
}, app.handler());
43-
44-
const port = server.addr.port;
45-
46-
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`);
47-
const received = new Promise<string>((resolve, reject) => {
48-
ws.onmessage = (e) => resolve(e.data);
49-
ws.onerror = (e) => reject(e);
23+
Deno.test("ctx.upgrade() - bare mode with options", async () => {
24+
const app = new App()
25+
.get("/ws", (ctx) => {
26+
// Options-only call must return { socket, response } (bare mode),
27+
// NOT a plain Response (managed mode).
28+
const { socket, response } = ctx.upgrade({ protocol: "graphql-ws" });
29+
socket.onmessage = (e) => socket.send(`bare-opts: ${e.data}`);
30+
return response;
5031
});
51-
ws.onopen = () => ws.send("hello");
5232

53-
const msg = await received;
54-
expect(msg).toEqual("echo: hello");
33+
const ac = new AbortController();
34+
const server = Deno.serve({
35+
hostname: "127.0.0.1",
36+
port: 0,
37+
signal: ac.signal,
38+
onListen: () => {},
39+
}, app.handler());
40+
41+
const port = server.addr.port;
42+
43+
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`, "graphql-ws");
44+
const received = new Promise<string>((resolve, reject) => {
45+
ws.onmessage = (e) => resolve(e.data);
46+
ws.onerror = (e) => reject(e);
47+
});
48+
ws.onopen = () => ws.send("hello");
49+
50+
const msg = await received;
51+
expect(msg).toEqual("bare-opts: hello");
52+
expect(ws.protocol).toEqual("graphql-ws");
53+
54+
ws.close();
55+
ac.abort();
56+
await server.finished;
57+
});
5558

56-
ws.close();
57-
ac.abort();
58-
await server.finished;
59-
},
59+
Deno.test("ctx.upgrade() - managed echo", async () => {
60+
const app = new App()
61+
.get("/ws", (ctx) =>
62+
ctx.upgrade({
63+
message(socket, event) {
64+
socket.send(`echo: ${event.data}`);
65+
},
66+
}));
67+
68+
const ac = new AbortController();
69+
const server = Deno.serve({
70+
hostname: "127.0.0.1",
71+
port: 0,
72+
signal: ac.signal,
73+
onListen: () => {},
74+
}, app.handler());
75+
76+
const port = server.addr.port;
77+
78+
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`);
79+
const received = new Promise<string>((resolve, reject) => {
80+
ws.onmessage = (e) => resolve(e.data);
81+
ws.onerror = (e) => reject(e);
82+
});
83+
ws.onopen = () => ws.send("hello");
84+
85+
const msg = await received;
86+
expect(msg).toEqual("echo: hello");
87+
88+
ws.close();
89+
ac.abort();
90+
await server.finished;
6091
});
6192

62-
Deno.test({
63-
name: "ctx.upgrade() - bare overload",
64-
sanitizeOps: false,
65-
sanitizeResources: false,
66-
async fn() {
67-
const app = new App()
68-
.get("/ws", (ctx) => {
69-
const { socket, response } = ctx.upgrade();
70-
socket.onmessage = (e) => socket.send(`bare: ${e.data}`);
71-
return response;
72-
});
73-
74-
const ac = new AbortController();
75-
const server = Deno.serve({
76-
hostname: "127.0.0.1",
77-
port: 0,
78-
signal: ac.signal,
79-
onListen: () => {},
80-
}, app.handler());
81-
82-
const port = server.addr.port;
83-
84-
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`);
85-
const received = new Promise<string>((resolve, reject) => {
86-
ws.onmessage = (e) => resolve(e.data);
87-
ws.onerror = (e) => reject(e);
93+
Deno.test("ctx.upgrade() - bare overload", async () => {
94+
const app = new App()
95+
.get("/ws", (ctx) => {
96+
const { socket, response } = ctx.upgrade();
97+
socket.onmessage = (e) => socket.send(`bare: ${e.data}`);
98+
return response;
8899
});
89-
ws.onopen = () => ws.send("hello");
90100

91-
const msg = await received;
92-
expect(msg).toEqual("bare: hello");
101+
const ac = new AbortController();
102+
const server = Deno.serve({
103+
hostname: "127.0.0.1",
104+
port: 0,
105+
signal: ac.signal,
106+
onListen: () => {},
107+
}, app.handler());
108+
109+
const port = server.addr.port;
110+
111+
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`);
112+
const received = new Promise<string>((resolve, reject) => {
113+
ws.onmessage = (e) => resolve(e.data);
114+
ws.onerror = (e) => reject(e);
115+
});
116+
ws.onopen = () => ws.send("hello");
117+
118+
const msg = await received;
119+
expect(msg).toEqual("bare: hello");
120+
121+
ws.close();
122+
ac.abort();
123+
await server.finished;
124+
});
93125

94-
ws.close();
95-
ac.abort();
96-
await server.finished;
97-
},
126+
Deno.test("app.ws() - echo shorthand", async () => {
127+
const app = new App()
128+
.ws("/ws", {
129+
message(socket, event) {
130+
socket.send(`ws: ${event.data}`);
131+
},
132+
});
133+
134+
const ac = new AbortController();
135+
const server = Deno.serve({
136+
hostname: "127.0.0.1",
137+
port: 0,
138+
signal: ac.signal,
139+
onListen: () => {},
140+
}, app.handler());
141+
142+
const port = server.addr.port;
143+
144+
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`);
145+
const received = new Promise<string>((resolve, reject) => {
146+
ws.onmessage = (e) => resolve(e.data);
147+
ws.onerror = (e) => reject(e);
148+
});
149+
ws.onopen = () => ws.send("hello");
150+
151+
const msg = await received;
152+
expect(msg).toEqual("ws: hello");
153+
154+
ws.close();
155+
ac.abort();
156+
await server.finished;
98157
});
99158

100-
Deno.test({
101-
name: "app.ws() - echo shorthand",
102-
sanitizeOps: false,
103-
sanitizeResources: false,
104-
async fn() {
105-
const app = new App()
106-
.ws("/ws", {
107-
message(socket, event) {
108-
socket.send(`ws: ${event.data}`);
159+
Deno.test("ctx.upgrade() - open and close handlers fire", async () => {
160+
const events: string[] = [];
161+
const closed = Promise.withResolvers<void>();
162+
163+
const app = new App()
164+
.get("/ws", (ctx) =>
165+
ctx.upgrade({
166+
open() {
167+
events.push("open");
109168
},
110-
});
111-
112-
const ac = new AbortController();
113-
const server = Deno.serve({
114-
hostname: "127.0.0.1",
115-
port: 0,
116-
signal: ac.signal,
117-
onListen: () => {},
118-
}, app.handler());
119-
120-
const port = server.addr.port;
121-
122-
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`);
123-
const received = new Promise<string>((resolve, reject) => {
124-
ws.onmessage = (e) => resolve(e.data);
125-
ws.onerror = (e) => reject(e);
126-
});
127-
ws.onopen = () => ws.send("hello");
169+
close() {
170+
events.push("close");
171+
closed.resolve();
172+
},
173+
}));
128174

129-
const msg = await received;
130-
expect(msg).toEqual("ws: hello");
175+
const ac = new AbortController();
176+
const server = Deno.serve({
177+
hostname: "127.0.0.1",
178+
port: 0,
179+
signal: ac.signal,
180+
onListen: () => {},
181+
}, app.handler());
131182

132-
ws.close();
133-
ac.abort();
134-
await server.finished;
135-
},
136-
});
183+
const port = server.addr.port;
137184

138-
Deno.test({
139-
name: "ctx.upgrade() - open and close handlers fire",
140-
sanitizeOps: false,
141-
sanitizeResources: false,
142-
async fn() {
143-
const events: string[] = [];
144-
const closed = Promise.withResolvers<void>();
145-
146-
const app = new App()
147-
.get("/ws", (ctx) =>
148-
ctx.upgrade({
149-
open() {
150-
events.push("open");
151-
},
152-
close() {
153-
events.push("close");
154-
closed.resolve();
155-
},
156-
}));
157-
158-
const ac = new AbortController();
159-
const server = Deno.serve({
160-
hostname: "127.0.0.1",
161-
port: 0,
162-
signal: ac.signal,
163-
onListen: () => {},
164-
}, app.handler());
165-
166-
const port = server.addr.port;
167-
168-
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`);
169-
await new Promise<void>((resolve) => {
170-
ws.onopen = () => resolve();
171-
});
172-
ws.close();
185+
const ws = new WebSocket(`ws://127.0.0.1:${port}/ws`);
186+
await new Promise<void>((resolve) => {
187+
ws.onopen = () => resolve();
188+
});
189+
ws.close();
173190

174-
await closed.promise;
175-
expect(events).toEqual(["open", "close"]);
191+
await closed.promise;
192+
expect(events).toEqual(["open", "close"]);
176193

177-
ac.abort();
178-
await server.finished;
179-
},
194+
ac.abort();
195+
await server.finished;
180196
});

0 commit comments

Comments
 (0)