Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(client/sse): add {EventSource,Request}Init options param #109

Merged
merged 1 commit into from
Jan 2, 2025
+15 −8
Merged

feat(client/sse): add {EventSource,Request}Init options param #109

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions src/client/sse.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@ export class SSEClientTransport implements Transport {
private _endpoint?: URL;
private _abortController?: AbortController;
private _url: URL;
private _eventSourceInit?: EventSourceInit;
private _requestInit?: RequestInit;

onclose?: () => void;
onerror?: (error: Error) => void;
onmessage?: (message: JSONRPCMessage) => void;

constructor(url: URL) {
constructor(url: URL, opts?: { eventSourceInit?: EventSourceInit, requestInit?: RequestInit }) {
this._url = url;
this._eventSourceInit = opts?.eventSourceInit;
this._requestInit = opts?.requestInit;
}

start(): Promise<void> {
Expand All @@ -29,7 +33,7 @@ export class SSEClientTransport implements Transport {
}

return new Promise((resolve, reject) => {
this._eventSource = new EventSource(this._url.href);
this._eventSource = new EventSource(this._url.href, this._eventSourceInit);
this._abortController = new AbortController();

this._eventSource.onerror = (event) => {
Expand Down Expand Up @@ -90,14 +94,17 @@ export class SSEClientTransport implements Transport {
}

try {
const response = await fetch(this._endpoint, {
const headers = new Headers(this._requestInit?.headers);
headers.set("content-type", "application/json");
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should set this before the user's given values, so they can apply a different content type if needed. I could see an argument either way (the argument to do it this way being: well, we will actually send JSON).

What are your thoughts?

Copy link
Contributor Author

@chrisdickinson chrisdickinson Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah – I lean towards the library's content-type taking precedence because, like you said, the library ultimately controls the body of the request.

That said, in similar libraries I've had some success letting the user inject their own fetch / EventSource for greater control; that would let the user replace the body with their own encoding (if say, they wanted to send the request encoded in msgpack or XML instead of JSON.) I waffled a bit on adding that interface to this PR since it can feel like "over-injection", but it'd look something like this:

diff --git a/src/client/sse.ts b/src/client/sse.ts
index 0e6e7eb..b6a37c0 100644
--- a/src/client/sse.ts
+++ b/src/client/sse.ts
@@ -1,6 +1,9 @@
 import { Transport } from "../shared/transport.js";
 import { JSONRPCMessage, JSONRPCMessageSchema } from "../types.js";
 
+type Fetch = typeof globalThis.fetch
+type EventSourceCtor = typeof globalThis.EventSource
+
 /**
  * Client transport for SSE: this will connect to a server using Server-Sent Events for receiving
  * messages and make separate POST requests for sending messages.
@@ -14,15 +17,24 @@ export class SSEClientTransport implements Transport {
   private _url: URL;
   private _eventSourceInit?: EventSourceInit;
   private _requestInit?: RequestInit;
+  private _fetch: Fetch;
+  private _EventSource: EventSourceCtor;
 
   onclose?: () => void;
   onerror?: (error: Error) => void;
   onmessage?: (message: JSONRPCMessage) => void;
 
-  constructor(url: URL, opts?: { eventSourceInit?: EventSourceInit, requestInit?: RequestInit }) {
+  constructor(url: URL, opts?: {
+    eventSourceInit?: EventSourceInit,
+    requestInit?: RequestInit,
+    fetch?: Fetch,
+    EventSource?: EventSourceCtor
+  }) {
     this._url = url;
     this._eventSourceInit = opts?.eventSourceInit;
     this._requestInit = opts?.requestInit;
+    this._fetch = opts?.fetch ?? globalThis.fetch;
+    this._EventSource = opts?.EventSource ?? globalThis.EventSource;
   }
 
   start(): Promise<void> {
@@ -33,7 +45,7 @@ export class SSEClientTransport implements Transport {
     }
 
     return new Promise((resolve, reject) => {
-      this._eventSource = new EventSource(this._url.href, this._eventSourceInit);
+      this._eventSource = new this._EventSource(this._url.href, this._eventSourceInit);
       this._abortController = new AbortController();
 
       this._eventSource.onerror = (event) => {
@@ -104,7 +116,7 @@ export class SSEClientTransport implements Transport {
         signal: this._abortController?.signal
       };
 
-      const response = await fetch(this._endpoint, init);
+      const response = await this._fetch(this._endpoint, init);
 
       if (!response.ok) {
         const text = await response.text().catch(() => null);

(If you're interested I can open this up as a separate PR!)

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, thanks!

I think my preference would be to stick to standard fetch and EventSource, and suggest that extreme levels of customization should just go into a custom, user-defined transport class. Transports are meant to be quite pluggable, specifically to support stuff like this. 🙂

const init = {
...this._requestInit,
method: "POST",
headers: {
"Content-Type": "application/json",
},
headers,
body: JSON.stringify(message),
signal: this._abortController?.signal,
});
signal: this._abortController?.signal
};

const response = await fetch(this._endpoint, init);

if (!response.ok) {
const text = await response.text().catch(() => null);
Expand Down
Loading