-
Notifications
You must be signed in to change notification settings - Fork 34
feat: support StreamableHTTPClientTransport #64
feat: support StreamableHTTPClientTransport #64
Conversation
|
Per the MCP spec, the streamable HTTP transport is meant to replace the SSE transport. Per their backward compatibility guide, clients should attempt to connect to URLs as though they are streamable HTTP first, and then fall back to SSE. Doing this would fix another problem. Prior to this change the transport type can be inferred from the structure of the config entry. That is, the |
|
I pushed some updates for the above feedback, but I only had time last night to rework some of the tests. IIRC there are 3 tests still failing. We should also make updates to the README & examples to drop the |
7b94c69 to
b33a2de
Compare
|
The build is green, however I added a new example to capture the Streamable HTTPs w/ fallback to SSE behavior, and it's not running successfully on the default (Streamable HTTP) case. Seems to work fine for the SSE case, however. |
|
When would this PR be merged? I'm in an urgent need of it. 🙏 |
|
OK, I think this is ready to go, but because I've mostly rewritten the original PR I need to request an internal review from another colleague. |
2002b2e to
9be061b
Compare
9be061b to
0f42f28
Compare
| | `prefixToolNameWithServerName` | boolean | `true` | If true, prefixes all tool names with the server name (e.g., `serverName__toolName`) | | ||
| | `additionalToolNamePrefix` | string | `mcp` | Additional prefix to add to tool names (e.g., `prefix__serverName__toolName`) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These defaults weren't actually changed in this PR (see checks in tests to verify) - just updated in README and config schema to reflect the actual defaults
| const conf = client.config; | ||
| expect(conf.additionalToolNamePrefix).toBe("mcp"); | ||
| expect(conf.prefixToolNameWithServerName).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this during test troubleshooting because the defaults reverted due to me accidentally setting default(...).optional() in the zod schema updates (needs to be .optional().default(...) else the defaults aren't applied during parsing).
| @@ -0,0 +1,173 @@ | |||
| import express from "express"; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this file for manual testing & for use via the added example because we don't yet have an integration suite.
| if (code == null) { | ||
| const m = streamableError.message.match(/\(HTTP (\d\d\d)\)/); | ||
| if (m && m.length > 1) { | ||
| code = parseInt(m[1], 10); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the grossest part - I needed this during early testing because for some reason the code property of the error wasn't being set. It appears that it is being set now, but I decided to leave it in for reliability.
@Dandelion-F will depend on code review - if all goes well it could be merged/released within a few hours. |
6b352ee to
84f0f70
Compare
__tests__/client.basic.test.ts
Outdated
| url: "http://localhost:8000/sse", | ||
| }, | ||
| server3: { | ||
| transport: "streamable", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) why "streamable" and not "streamable-http" or "streamable_http"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't decide the key - honestly I think http is better since this option should be used for all HTTP endpoints (including SSE). Will change to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed - will likely need to add strings to this union as other popular client configs come out and decide they want to use some other key, however.
Description
Adds support for Streamable HTTP. Related to #59
Fixes # (issue)
Type of change
How Has This Been Tested?
Checklist: