-
Notifications
You must be signed in to change notification settings - Fork 248
fix: export jsonResponse field #397
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
Conversation
|
Why should JsonResponse be exported? What is the use-case? The reason it is unexported is that we didn't understand why it is necessary. |
Thanks for asking for clarification. |
|
@CCpro10 my question is really why the server needs to reply with application/json. I'm not sure what you mean by "reply in JSON format for the SSE stream". Do you mean "reply with application/json rather than text/event-stream"? Can you clarify why you need to have your server reply with application/json? |
|
@findleyr "Yes, that's correct. My intention is for the server to reply with application/json rather than text/event-stream. If the server supports application/json for these responses, it would make the MCP client much easier to handle." "As per the official MCP documentation, it states: "Therefore, enabling the server to respond with application/json (by allowing configuration via the JsonResponse field) is a valid and explicitly supported option according to the protocol." |
|
@CCpro10 yes, the spec states that the server must return one of text/event-stream or application/json. The SDK, by default, returns text/event-stream. When I added jsonResponse, I was asked why it was needed, when always preferring text/event-stream also conforms to the spec. What is your client? If you are using a spec-conformant client, then it must support text/event-stream, and therefore it also doesn't matter from the client's perspective. Put differently, if your client needs to connect to servers that you don't control, then it must support text/event-stream. I'm not opposed to exposing JSONResponse, but I'd like to have a real reason to do so. Again, it was originally exported in the CL that added it, but I didn't have a good reason for it. So I'd like to understand your use-case better. FWIW, I do hear that some proxies have trouble with text/event-stream, so maybe that's a good enough reason, but it sounds like that's not the reason you need it. |
|
@findleyr |
|
@CCpro10 I think defaulting to application/json and upgrading to SSE would be fine, though it's not trivial. I can send a CL implementing that. I'll file a proposal for exporting JSONResponse, to discuss if there are use cases. If not, we can remove it. |
|
@findleyr OK, that's great |
|
Actually, in #211, there was some feedback about the necessity of this for an MCP gateway, which I think is the same feedback as we'd heard about proxies. Therefore, I reopened that proposal, and we can merge this (we'll leave the proposal open for some time). |
bc7a4bb to
ddbce9e
Compare
|
@findleyr I have revised the code as you requested. Json --> JSON |
ddbce9e to
0a8137a
Compare

StreamableHTTPOptions.JsonResponse should be exported.