Skip to content

v2: move registered primitives to classes#1453

Closed
KKonstantinov wants to merge 9 commits intomodelcontextprotocol:mainfrom
KKonstantinov:feature/v2-registered-definitions-classes
Closed

v2: move registered primitives to classes#1453
KKonstantinov wants to merge 9 commits intomodelcontextprotocol:mainfrom
KKonstantinov:feature/v2-registered-definitions-classes

Conversation

@KKonstantinov
Copy link
Copy Markdown
Contributor

@KKonstantinov KKonstantinov commented Feb 2, 2026

Refactor RegisteredTool, RegisteredPrompt, RegisteredResource to proper classes

Motivation and Context

The RegisteredTool, RegisteredPrompt, RegisteredResource, and RegisteredResourceTemplate types were previously plain object types. This refactoring converts them to proper classes with:

  • Private fields using # syntax for encapsulation
  • Getter methods for read-only access to properties
  • Simplified update() method using Object.assign for protocol fields
  • Constructor callbacks pattern for communicating with McpServer
  • New getter methods on McpServer to retrieve all registered items
  • Added icons support to RegisteredTool (for parity with protocol Tool type)

How Has This Been Tested?

  • pnpm lint:fix:all - passes
  • pnpm typecheck:all - passes
  • pnpm test:all - 487 tests passing

Breaking Changes

Yes, users will need to update their code:

v1 (update field) v2 (update field) Applies to
paramsSchema inputSchema RegisteredTool.update()
callback handler RegisteredTool.update()

Note: In v1, the RegisteredTool.update() method used paramsSchema which did not match the inputSchema field used in registerTool(). This inconsistency has been fixed in v2 — both now use inputSchema.

New getter methods on McpServer:

  • mcpServer.toolsReadonlyMap<string, RegisteredTool>
  • mcpServer.promptsReadonlyMap<string, RegisteredPrompt>
  • mcpServer.resourcesReadonlyMap<string, RegisteredResource>
  • mcpServer.resourceTemplatesReadonlyMap<string, RegisteredResourceTemplate>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

New file structure

packages/server/src/server/primitives/
├── index.ts          # Re-exports all classes and types
├── types.ts          # Shared callback types (OnUpdate, OnRename, OnRemove)
├── tool.ts           # RegisteredTool class
├── prompt.ts         # RegisteredPrompt class
├── resource.ts       # RegisteredResource class
└── resourceTemplate.ts # RegisteredResourceTemplate class + ResourceTemplate class

Key design decisions

  1. Private fields with #: Uses JavaScript private class fields for true encapsulation
  2. Protocol fields grouped: Protocol-derived fields stored in #protocolFields object, SDK-specific fields stored separately
  3. Simplified update(): Uses Object.assign(this.#protocolFields, protocolUpdates) so new protocol fields automatically work
  4. Constructor callbacks: Classes receive onUpdate, onRename, onRemove callbacks for communicating state changes to McpServer
  5. Type derivation with Omit: Config types derived from protocol types using Omit to automatically include new fields

@KKonstantinov KKonstantinov requested a review from a team as a code owner February 2, 2026 23:50
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Feb 2, 2026

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/client@1453

@modelcontextprotocol/server

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server@1453

@modelcontextprotocol/express

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/express@1453

@modelcontextprotocol/hono

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/hono@1453

@modelcontextprotocol/node

npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/node@1453

commit: 904e613

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Feb 2, 2026

⚠️ No Changeset found

Latest commit: 1f8f073

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Copy Markdown
Contributor

@felixweinberger felixweinberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@KKonstantinov
Copy link
Copy Markdown
Contributor Author

KKonstantinov commented Mar 5, 2026

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:

  • Encapsulation - protects the notification contract - with plain objects, all fields were publicly mutable. A user who gets a RegisteredTool could do anything, bypassing our notification system and leaving the client out of sync. With this PR, there's no way to do that, they have to go through the update()/enable()/disable() methods.
  • Protocol conversion - each class now owns its conversion logic - toProtocolTool() / toProtocolPrompt() / toProtocolResource() method - e.g. zod-to-json-schema conversion, previously just sitting inside mcp.ts, mixing protocol serialization with request routing etc.
  • mcp.ts - had become quite a monolith, just reducing its sheer size and cognitive complexity
  • Update logic is forward-compatible - new protocol fields added to Tool/Prompt/Resource types automatically flow through update() without modifying the class. The old approach required manually handling each field (and we have several bugs which this PR fixes - there are new fields added to the spec and to the Tool/Prompt/Resources, which main and v1 of the SDK is missing in the update schema)
  • Splitting the callback pattern makes the contract quite explicit / testable - primitives do not know about McpServer at all.
  • There is a small argument on classes vs POJOs, although it probably does not matter much in this case, as Servers would not register a sheer number of tools/resources/prompts. However, even with V8 optimizations, doing POJO objects and assigning methods to them, means we actually have N amount of enable, disable, methods in-memory, where N is the number of tools+resources+prompts; This is negligible impact for the number of definitions MCP server have nowadays, but worth mentioning. With classes, there is just one instance of these methods sitting on the class prototype. Each closure also retains a reference to its respective enclosing scope (the vars it captured), which prevents GC of those scope objects, although this is just a general statement and I've not measured the actual impact.
  • A lot of the net new code is actually TSDocs (probably around 200-300 lines)

@felixweinberger
Copy link
Copy Markdown
Contributor

felixweinberger commented Mar 9, 2026

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:

  • Encapsulation - protects the notification contract - with plain objects, all fields were publicly mutable. A user who gets a RegisteredTool could do anything, bypassing our notification system and leaving the client out of sync. With this PR, there's no way to do that, they have to go through the update()/enable()/disable() methods.
  • Protocol conversion - each class now owns its conversion logic - toProtocolTool() / toProtocolPrompt() / toProtocolResource() method - e.g. zod-to-json-schema conversion, previously just sitting inside mcp.ts, mixing protocol serialization with request routing etc.
  • mcp.ts - had become quite a monolith, just reducing its sheer size and cognitive complexity
  • Update logic is forward-compatible - new protocol fields added to Tool/Prompt/Resource types automatically flow through update() without modifying the class. The old approach required manually handling each field (and we have several bugs which this PR fixes - there are new fields added to the spec and to the Tool/Prompt/Resources, which main and v1 of the SDK is missing in the update schema)
  • Splitting the callback pattern makes the contract quite explicit / testable - primitives do not know about McpServer at all.
  • There is a small argument on classes vs POJOs, although it probably does not matter much in this case, as Servers would not register a sheer number of tools/resources/prompts. However, even with V8 optimizations, doing POJO objects and assigning methods to them, means we actually have N amount of enable, disable, methods in-memory, where N is the number of tools+resources+prompts; This is negligible impact for the number of definitions MCP server have nowadays, but worth mentioning. With classes, there is just one instance of these methods sitting on the class prototype. Each closure also retains a reference to its respective enclosing scope (the vars it captured), which prevents GC of those scope objects, although this is just a general statement and I've not measured the actual impact.
  • A lot of the net new code is actually TSDocs (probably around 200-300 lines)

A user who gets a RegisteredTool could do anything

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.

Encapsulation

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?

Update logic forward compatibility

Object.assign should work on POJOs too I think, we don't need classes for it? That should also help with catching the missing fields in update()

Splitting the callback pattern makes it explicit

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?

Classes vs POJOs

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.

TSDocs make up a lot of the code

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?

  1. getters + icons support + paramsSchema->inputSchema + Object.assign in update() to make it forward compatible. I think that fixes the missing field bugs and makes it more ergonomic
  2. You also mentioned that this PR fixes a bunch of bugs / issues - can we add regression tests to demonstrate which specific things would fail with the existing code? + mention or link the issues if there are any so we can close.
  3. The class refactor I would prefer to defer - I think encapsulation is valid, but feels potentially premature when we can get the wins from something a bit leaner above?

@KKonstantinov
Copy link
Copy Markdown
Contributor Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants