feat: add streamable http [MCP-55] #359
Conversation
fmenezes
changed the title
README.md
Outdated
| | `transport` | stdio | Either 'stdio' or 'http'. | | ||
| | `httpPort` | 3000 | Port number. | | ||
| | `httpHost` | 127.0.0.1 | Host to bind the http server. | | ||
| | `logger` | disk,mcp | Comma separated values, possible values are `mcp`, `disk` and `stderr`. | |
There was a problem hiding this comment.
nit: can we expand a little bit here or below in the features the logger purpose and usage examples?
| app.use(express.json()); | ||
| app.enable("trust proxy"); // needed for reverse proxy support | ||
| app.use(express.urlencoded({ extended: true })); | ||
| app.use(express.json()); |
There was a problem hiding this comment.
[q] do we need to call app.use(express.json()); twice?
| "streamableHttpTransport", | ||
| `Error handling request: ${error instanceof Error ? error.message : String(error)}` | ||
| ); | ||
| res.sendStatus(400); |
There was a problem hiding this comment.
nit: we have a task for this but we'll need to return JSON-RPC response error body, so this is a non-blocker, just in case its too simple to add here
| ); | ||
| }); | ||
|
|
||
| transport.onclose = () => { |
There was a problem hiding this comment.
[q] should we call this in index.ts?
There was a problem hiding this comment.
no, mcpServer.close() should close the transport
| // This function creates a StdioServerTransport and replaces the internal readBuffer with EJsonReadBuffer | ||
| // that uses EJson.parse instead. | ||
| export function createEJsonTransport(): StdioServerTransport { | ||
| export function createStdioTransport(): StdioServerTransport { |
There was a problem hiding this comment.
nit: why not call stdio.ts if we're under transports folder? same for streamableHttp
| ); | ||
| res.status(400).json({ | ||
| jsonrpc: "2.0", | ||
| error: { |
There was a problem hiding this comment.
based on the spec we need to also add a request ID field here
There was a problem hiding this comment.
(note: feel free to address in the rpc ticket, probably worth just assigning it to you already)
There was a problem hiding this comment.
I see null ids everywhere in the SDK (https://github.com/modelcontextprotocol/typescript-sdk/blob/a9c907dd700a5c4b977624ac47fd8cc24c6bd473/src/server/streamableHttp.ts#L291). I'll leave it for now.
| jsonrpc: "2.0", | ||
| error: { | ||
| code: JSON_RPC_ERROR_CODE_PROCESSING_REQUEST_FAILED, | ||
| message: `Error handling request: ${error instanceof Error ? error.message : String(error)}`, |
There was a problem hiding this comment.
nit: should we throw the classic error message and populate any extra info in the error.data optional field?
2 participants
Proposed changes
Adding streamable http to MCP-42 feature branch.
Note: state management and tests will be added in a future PR.
Checklist