feat(server)\!: drop McpHttpHandler.node; ship toNodeHandler(handler) in @modelcontextprotocol/node#2331
Conversation
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export type { FetchLikeMcpHandler, NodeIncomingMessageLike, NodeMcpRequestHandler, NodeServerResponseLike } from './toNodeHandler.js'; | ||
| export { toNodeHandler } from './toNodeHandler.js'; |
There was a problem hiding this comment.
🟡 The @modelcontextprotocol/node package README's 'Exports' section and usage examples were not updated for toNodeHandler, which this PR adds to the package and makes the only way to mount a createMcpHandler handler on Node frameworks. Consider adding toNodeHandler (and the new types) to the Exports list plus a short createMcpHandler + toNodeHandler mounting example, since users coming from the removed handler.node face will land on this package's npm page looking for the replacement.
Extended reasoning...
What's missing. This PR adds toNodeHandler (plus FetchLikeMcpHandler, NodeIncomingMessageLike, NodeMcpRequestHandler, NodeServerResponseLike) to packages/middleware/node/src/index.ts, and — as a breaking change on the server side — removes McpHttpHandler.node, making toNodeHandler(handler) the only sanctioned way to mount createMcpHandler on Express/Fastify/plain node:http. However, packages/middleware/node/README.md is untouched: its explicit Exports section still lists only NodeStreamableHTTPServerTransport and StreamableHTTPServerTransportOptions, and its usage examples show only the legacy transport wiring. toNodeHandler and createMcpHandler appear nowhere in it.\n\nWhy it matters. The README is the package's npm landing page. The migration path for the removed .node face is "add a dependency on @modelcontextprotocol/node and import toNodeHandler" — so users following the breaking change will end up on exactly this package's page, where the replacement API is not mentioned. The enumerated Exports list also reads as authoritative and becomes misleadingly incomplete for the package's headline new export. The repo's review checklist asks new features to carry prose documentation, and the sibling express middleware README does keep its Exports list current (it documents hostHeaderValidation).\n\nConcrete walk-through. A user on @modelcontextprotocol/server 2.x-alpha upgrades and gets a type error on handler.node(req, res, req.body). The changeset / migration guide tells them to install @modelcontextprotocol/node and use toNodeHandler. They open that package's npm page (the README) to check the API: the Exports section lists only NodeStreamableHTTPServerTransport / StreamableHTTPServerTransportOptions, and every example wires the legacy sessionful transport. Nothing on the page confirms toNodeHandler exists or shows how to mount it; they have to fall back to the JSDoc or the repo's docs/migration.md.\n\nWhy this is only a nit (addressing the refutation). A refuting verifier correctly points out that the new API is well documented elsewhere — docs/migration.md carries the full before/after Node mounting recipe, docs/migration-SKILL.md and both changesets describe the move, toNodeHandler.ts has an extensive module-level usage doc, and 8 examples were swept to the new pattern — and that the README was already stale before this PR (it also omits the previously-added hostHeaderValidation/originValidation exports), so nothing in it becomes wrong: the NodeStreamableHTTPServerTransport wiring it shows still works. All true, and that is exactly why this is filed as a non-blocking documentation suggestion rather than a defect. Still, this PR is the change that makes toNodeHandler the package's most important export and the designated replacement for a removed API, so updating the package README here (rather than letting the staleness grow) is the natural place to do it.\n\nSuggested fix. Add toNodeHandler (and optionally the four new types) to the README's Exports section and a short mounting example, e.g.:\n\nts\nimport { createMcpHandler } from '@modelcontextprotocol/server';\nimport { toNodeHandler } from '@modelcontextprotocol/node';\n\nconst handler = createMcpHandler(factory);\napp.all('/mcp', toNodeHandler(handler));\n// or, behind express.json():\nconst node = toNodeHandler(handler);\napp.all('/mcp', (req, res) => void node(req, res, req.body));\n
| }); | ||
| } catch { | ||
| response = internalServerErrorResponse(echoableRequestId(parsedBody)); | ||
| } | ||
|
|
||
| const headers: Record<string, string> = {}; | ||
| for (const [name, value] of response.headers) { | ||
| headers[name] = value; | ||
| } |
There was a problem hiding this comment.
🟡 The bare catch {} in toNodeHandler only builds the 500/-32603 fallback, whereas the removed nodeFace also called reportError(toError(error)) — so request-conversion failures and the closed-handler throw lose their onerror reporting and become silent 500s. Consider an optional onerror option on toNodeHandler(handler, { onerror }) (or a console.error in the catch), or explicitly note the reporting loss in the changeset/migration docs.
Extended reasoning...
What changed. The old nodeFace in createMcpHandler.ts wrapped nodeRequestToFetchRequest() + fetchFace() in catch (error) { reportError(toError(error)); response = internalServerErrorResponse(...) }, so adapter-level failures were reported through the handler's onerror callback before the 500 was written. The new toNodeHandler.ts reproduces only the response fallback: the catch {} at lines 100–102 builds the 500/-32603 body but reports nothing, and the adapter is structurally typed over FetchLikeMcpHandler = { fetch } with no onerror option and no logging.
Which failures are affected. Two classes that are by definition not entry-internal:
- Node→
Requestconversion failures — e.g. a request body stream that errors mid-read, or header valuesHeadersrejects.handler.fetchis never called, so the handler'sonerrorcannot fire. - The closed-handler throw —
fetchFacethrows'This MCP handler has been closed'before its own try/catch, so a late request afterhandler.close()also never reachesreportError.
Entry-internal failures (factory throws, connect failures, classification rejections) are unaffected — they are still reported inside fetchFace as before.
Step-by-step example. An operator runs Express with toNodeHandler(handler) and onerror: e => log.error(e). They call handler.close() during a rolling deploy while a request is still in flight at the LB. (1) The late POST reaches the adapter; (2) handler.fetch throws 'This MCP handler has been closed'; (3) the bare catch {} swallows it and writes the 500/-32603 body; (4) nothing reaches onerror or any log. On the pre-PR .node face, step (3) was reportError(toError(error)) followed by the same 500, so the operator's logger saw the event. The new test 'answers with a 500 JSON-RPC error when handler.fetch throws (closed handler)' pins only the response shape — no reporting assertion — so the loss is unpinned.
Why this isn't fully covered by the PR's stated rationale. The description says the adapter "reproduces the adapter-level error fallback ... entry-internal failures continue to be reported via fetch", which is accurate, and the structural decoupling means the adapter genuinely cannot reach the handler's onerror without an API addition. A userland workaround also exists (wrap fetch with a .catch(e => { log(e); throw e }) before passing to toNodeHandler). So this is not a correctness bug — the client-visible response is identical and the 500 still shows in access logs — but it is a real, undocumented observability regression for the two adapter-level failure classes.
Suggested fix. Either accept an optional second argument toNodeHandler(handler, { onerror }) and call it in the catch, or at minimum mention in handler-drop-node-face.md / the migration docs that adapter-level failures (conversion errors, post-close requests) are no longer routed to the handler's onerror and how to wrap fetch if that reporting is needed.
…dler) in @modelcontextprotocol/node
createMcpHandler now returns the web-standards-idiomatic { fetch, close, notify, bus } shape — what Workers/Bun/Deno expect from `export default`. The duck-typed .node(req, res, parsedBody?) face is removed; Node frameworks (Express, Fastify, plain node:http) wrap the handler once with toNodeHandler(handler) from @modelcontextprotocol/node, which converts the Node request to a web-standard Request, calls handler.fetch, and writes the Response back honoring write backpressure.
NodeIncomingMessageLike / NodeServerResponseLike move from @modelcontextprotocol/server to @modelcontextprotocol/node. The Node-conversion helpers and the .node-face unit tests move with them (toNodeHandler.test.ts).
Consumers updated: examples/server (dualEraStreamableHttp, multiRoundTrip), test/integration createMcpHandler over real HTTP, test/conformance everythingServer.
BREAKING CHANGE: McpHttpHandler.node is removed. Use toNodeHandler(handler) from @modelcontextprotocol/node.
felixweinberger
force-pushed
the
fweinberger/on-tonodehandler
branch
from
f9069fa to
c8779f8
Compare
felixweinberger
force-pushed
the
fweinberger/on-small-fixes
branch
from
761bd17 to
e230a8e
Compare
🦋 Changeset detectedLatest commit: c8779f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🦋 Changeset detectedLatest commit: c8779f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
1 participant
Removes the
.nodeface fromMcpHttpHandlerand ships the Node adapter as a free functiontoNodeHandler(handler)in@modelcontextprotocol/node. Stacked onfweinberger/on-small-fixes.Motivation and Context
{ fetch, close, notify, bus }is the web-standards-idiomatic handler shape (Workers / Bun / Denoexport default). Carrying a Node-specific.node(req, res, body?)method on the same interface couples the runtime-neutral entry to a Nodehttpadapter and forces every host to take the Node types.This PR:
McpHttpHandler.nodefrom the interface and implementationNodeIncomingMessageLike/NodeServerResponseLikeand thenodeRequestToFetchRequestbody verbatim intopackages/middleware/node/src/toNodeHandler.tsastoNodeHandler(handler): (req, res, parsedBody?) => Promise<void>overhandler.fetch(structurally typedFetchLikeMcpHandler, so hand-wiredisLegacyRequestcompositions also adapt)fetchNode*Liketype re-exports from@modelcontextprotocol/server; addstoNodeHandler+ 4 types to@modelcontextprotocol/node.node(...)call site inexamples/(8 sites acrossharness.ts,legacy-routing,bearer-auth,oauth,oauth-client-credentials,json-response,stateless-legacy,subscriptions) and the integration/conformance fixtures; six@mcp-examples/*packages gain a@modelcontextprotocol/nodeworkspace depHow Has This Been Tested?
.node-face unit tests +nodeRequestResponsefixture re-homed topackages/middleware/node/test/toNodeHandler.test.ts(+1 new closed-handler 500 test); server suite keeps the detach-safe-fetch test only — pure relocation following the implementation, no assertions weakened.fetchonly — no change neededjson-schema-2020-12,server-sse-polling)Breaking Changes
Yes.
McpHttpHandler.nodeis removed. Migration:migration.md+migration-SKILL.mdcarry the before/after; the existingcreate-mcp-handler.mdchangeset is reworded and a newhandler-drop-node-face.mdchangeset is added (server major / node minor).Types of changes
Checklist
Additional context
The only remaining
handler.nodereference is the intentional historical mapping indocs/migration-SKILL.md.