Fix: Fix JSONRPC Types#1026
Merged
dsp-ant merged 5 commits intomodelcontextprotocol:mainmodelcontextprotocol/modelcontextprotocol:mainfrom Aug 28, 2025
hesreallyhim-forks:fix/fix-JSONRPC-typeshesreallyhim-forks/modelcontextprotocol:fix/fix-JSONRPC-typesCopy head branch name to clipboard
Merged
Fix: Fix JSONRPC Types#1026dsp-ant merged 5 commits intomodelcontextprotocol:mainmodelcontextprotocol/modelcontextprotocol:mainfrom hesreallyhim-forks:fix/fix-JSONRPC-typeshesreallyhim-forks/modelcontextprotocol:fix/fix-JSONRPC-typesCopy head branch name to clipboard
dsp-ant merged 5 commits intomodelcontextprotocol:mainmodelcontextprotocol/modelcontextprotocol:mainfrom
hesreallyhim-forks:fix/fix-JSONRPC-typeshesreallyhim-forks/modelcontextprotocol:fix/fix-JSONRPC-typesCopy head branch name to clipboard
Conversation
Member
|
LGTM! 👍 |
Contributor
Author
|
thanks @dsp-ant I merged in |
dsp-ant
approved these changes
Aug 28, 2025
cliffhall
added a commit
to cliffhall/mcp-typescript-sdk
that referenced
this pull request
Aug 29, 2025
- Omit spec.types.test.ts for now unti we can figure out how to make it work. - Changes in the spec file are causing it to fail vis-à-vis JSONRPCNotification vs Notification - see modelcontextprotocol/modelcontextprotocol#1026
9 tasks
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the schema, JSONRPC{Request,Response,Notification,Error} generally extends {Request,Response,Notification,Error}. But then nothing actually consumes the JSONRPCX interface - Request sub-types extend Request; Response sub-types extend Response; etc. In this PR, I fix this confusion by having the sub-types extend the JSONRPC types.
Motivation and Context
JSONRPCX extends X.
SpecificSubtypeX extends X.
Therefore:
JSONRPCX and SpecificSubtypeX are siblings in the type hierarchy. But intuitively, the JSONRPC types are supposed to be the base/foundation. One expects: JSONRPC-SUPER-TYPE -> TYPE -> SUB-TYPE. Maybe there is a reallllly good reason for this.
Concrete example:
(So far, so good....)
OK, let's gooooo:
How Has This Been Tested?
There are no tests of this kind in this repo - I am happy to write tests in whatever style is preferred. The assumption was that
tscwould catch type errors. But this isn't a type/compilation error - the schema is valid, it just implies that an InitializationRequest is not a JSON-RPC 2.0 Request.If you want to see for yourself, you can paste the existing schema into a IDE/playground (with type-checking), and then observe what happens with these statements:
type _SchemaTest = InitializeRequest extends JSONRPCRequest ? true : never;Also, Claude was nice enough to provide a little snippet:
interface-compatibility-test.txt
Breaking Changes
As far as I can tell, basically no. It seems to me that each SDK declares the types themselves. I searched on GitHub and couldn't find one instance of anyone importing from this repo directly. So in that sense - no.
Types of changes
Checklist
I will complete the items on the checklist once I get confirmation that this analysis is correct.
Additional context
We probably don't need
Requestif my analysis is correct.Resultis OK because it's a property of a JSONRPCResult. Also I pulled out "Error" into its own interface because it was stylistically aberrant.