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.

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

Conversation

chrisdickinson
Copy link
Contributor

@chrisdickinson chrisdickinson commented Dec 31, 2024

Motivation and Context

This enables users of the SSE client to optionally send cookie-based credentials to the event source MCP server using EventSourceInit 1 & set outgoing request headers for the POST call using RequestInit 2.

How Has This Been Tested?

I ran through npm test; if there's interest in this I'll add some additional tests and make sure the change integrates cleanly with @modelcontextprotocol/inspector

Breaking Changes

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

Alternatively, giving the user the ability to inject their own EventSource/fetch implementations would achieve the same goal a bit more flexibly.

(Also hi! First time contributing here – great project, thanks for all the hard work!)

This enables users of the SSE client to optionally send cookie-based credentials
to the event source MCP server using `EventSourceInit` [1] parameters and set
the outgoing request headers for the POST call.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/EventSource/EventSource#options
@chrisdickinson
Copy link
Contributor Author

This might cover the same needs as #105, too, while allowing additional headers to be set on the outgoing POST requests.

Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great change! Just one comment for discussion, but I'll go ahead and merge this and we can follow up if needed.

@@ -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

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. 🙂

@jspahrsummers jspahrsummers merged commit 4658fd0 into modelcontextprotocol:main Jan 2, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants