v2: move registered primitives to classes#1453
v2: move registered primitives to classes#1453KKonstantinov wants to merge 9 commits intomodelcontextprotocol:mainfrom
v2: move registered primitives to classes#1453Conversation
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
felixweinberger
left a comment
There was a problem hiding this comment.
What benefit does this refactor provide? What we can we do with classes here that we couldn't do before?
I vaguely remember we had a previous version of this diff (#1426 (comment)) but aligned not to add this full complexity - it seems to just add boilerplate for things we can already do today?
Adding the getters seems valuable, but not sure I see the value of rewriting the enable/update/disable methods that were already working.
Might be wrong though, keen to understand the benefit here.
It's a good Q, I need to remember as well, as we had some discussion some time ago.. Looking at it now with a fresh mind:
|
As that's not something in the docs I think that could be considered user error - there's a bunch of possible misuses in the SDK that I don't think we can catch all of them.
Tbh I don't really see the benefit. Again it's a lot of speculative abstraction where we don't necessarily know what it enables or allows yet. Do we have specific issues or problems that we can solve with this that we didn't have before? I just don't see what adding these wrappers, setters, classes enables that's not provided by just doing the getters and calling it a day?
I don't think the 3-callback constructor buys much over the current closure — it's still coupled, just via 3 function refs instead of 1 scope capture. What test would we write with this we can't write today?
I think the performance gain is likely negligible compared to the maintenance cost added. Unless we have strong empirical data to support killing POJOs, I'd call this a non-goal and focus on keeping it as simple as possible instead.
That's fair, it's very verbose and unavoidable given we're adding it to all classes. As concrete next steps, can we reduce the scope of this PR?
|
|
As discussed, will make another version of this PR with a simpler go / less overhead while keeping the benefits identified; will also add regression tests (like the missing spec types on the update methods, e.g. having that working dynamically) |
Refactor RegisteredTool, RegisteredPrompt, RegisteredResource to proper classes
Motivation and Context
The
RegisteredTool,RegisteredPrompt,RegisteredResource, andRegisteredResourceTemplatetypes were previously plain object types. This refactoring converts them to proper classes with:#syntax for encapsulationupdate()method usingObject.assignfor protocol fieldsMcpServerMcpServerto retrieve all registered itemsiconssupport toRegisteredTool(for parity with protocolTooltype)How Has This Been Tested?
pnpm lint:fix:all- passespnpm typecheck:all- passespnpm test:all- 487 tests passingBreaking Changes
Yes, users will need to update their code:
paramsSchemainputSchemaRegisteredTool.update()callbackhandlerRegisteredTool.update()Note: In v1, the
RegisteredTool.update()method usedparamsSchemawhich did not match theinputSchemafield used inregisterTool(). This inconsistency has been fixed in v2 — both now useinputSchema.New getter methods on
McpServer:mcpServer.tools→ReadonlyMap<string, RegisteredTool>mcpServer.prompts→ReadonlyMap<string, RegisteredPrompt>mcpServer.resources→ReadonlyMap<string, RegisteredResource>mcpServer.resourceTemplates→ReadonlyMap<string, RegisteredResourceTemplate>Types of changes
Checklist
Additional context
New file structure
Key design decisions
#: Uses JavaScript private class fields for true encapsulation#protocolFieldsobject, SDK-specific fields stored separatelyObject.assign(this.#protocolFields, protocolUpdates)so new protocol fields automatically workonUpdate,onRename,onRemovecallbacks for communicating state changes toMcpServerOmitto automatically include new fields