-
Notifications
You must be signed in to change notification settings - Fork 261
feat: rename server property to mcp in MCP-related classes for improved developer experience #427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ed developer experience - **BREAKING**: Renamed `server` property to `mcp` in `McpAgent` and updated related examples. - **BREAKING**: Renamed `mcp` property to `mcpClientManager` in `Agent` class to avoid naming conflicts. - Added backward compatibility for `server` property in `McpAgent` with a deprecation warning. - Updated all MCP examples to reflect the new naming convention. - Improved property naming consistency across the MCP implementation.
🦋 Changeset detectedLatest commit: c8994e2 The changes in this PR will be included in the next version bump. 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 |
mcp: MCPClientManager = new MCPClientManager(this._ParentClass.name, "0.0.1"); | ||
mcpClientManager: MCPClientManager = new MCPClientManager( | ||
this._ParentClass.name, | ||
"0.0.1" | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In retrospect, I do not think this change is necessary and I'm happy to back it out. This is the most breaking thing about this PR. Though again I don't think anyone is intended to use this property (it's also undocumented).
get mcpClientManager() { | ||
return this._agent.mcpClientManager; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bit is technically breaking for anyone who's using the client manager, but I don't think anyone is and it's not documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very happy with this change. I think this is super confusing at the moment.
We should keep the mcp()
and mark it as deprecated to be removed in the next version.
* @deprecated Use the `mcp` property instead for better developer experience. | ||
*/ | ||
server?: MaybePromise<McpServer | Server>; | ||
/** | ||
* This is only set as optional for backward compatibility in case you're | ||
* using the legacy `server` property. It's recommended to use `mcp`. | ||
* | ||
* In the future, we'll make this a required abstract property to implement | ||
* and remove the `server` property. | ||
*/ | ||
abstract server: MaybePromise<McpServer | Server>; | ||
mcp?: MaybePromise<McpServer | Server>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the primary focus of this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be open to this changing to mcpServer() ?
|
||
export class MyMCP extends McpAgent<Env, State, {}> { | ||
server = new McpServer({ | ||
mcp = new McpServer({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe mcp
could be a getter that returns server
? That way it would be totally backward compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that would allow me to do this:
export class MyMCP extends McpAgent {
mcp = new McpServer({}); // <-- this is the desired API
async init() {
this.mcp.server.setRequestHandler(
SetLevelRequestSchema,
async (request, extra) => {
// ... handler logic
},
);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this?
export class MyMCP extends McpAgent {
get mcp(): McpServer {
return this.server;
}
server = new McpServer({});
async init() {
this.mcp.server.setRequestHandler(
SetLevelRequestSchema,
async (request, extra) => {
// ... handler logic
},
);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could do that myself without having to change anything in agents
. That's actually a reasonable workaround if they decide this is more trouble than it's worth. Thanks for the idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, actually I can't do that because mcp
is already set to the mcpClientManager
🙁
commit: |
@@ -0,0 +1,31 @@ | |||
--- | |||
"@cloudflare/agents": major |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to major because there was an example of using the previous existing mcp
property so people may actually be using that.
}; | ||
|
||
const result = await this.mcp.callTool({ | ||
const result = await this.mcpClientManager.callTool({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the breaking change.
FYI I'm not against this, I think the change makes life better. My concern it's a breaking change, if we do so, I'd like to make a bigger release and then also prepare people for this migration. |
I'm fine waiting 👍 |
get mcpServer(): MaybePromise<McpServer | Server> { | ||
const server = this.mcp ?? this.server; | ||
if (!server) { | ||
throw new Error( | ||
'Neither the `mcp` nor `server` property is set. Please set "mcp".' | ||
); | ||
} | ||
|
||
return server; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this adds to the confusion right now.
IMO mcpServer should be the new name for server()
. Thereby we can have a deprecation notice on the mcp()
naming and keep everything clean. Much happier for more descriptive names especially as this class gets bigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed server.server is super annoying and doesnt give you any info on what each class should do. Also agreed that the clientManager being called mcp is confusing.
To make migration a bit smoother I would propose:
- mcp -> mcpClientManager
- server -> mcpServer
the 2. also emulates the Anthropic way of doing things in MCP TS SDK where the McpServer is a higher level abstraction and the Server is the lower level one. This will be easy to land as we can deprecate the old names with no conflict.
but that still means writing mcpServer.server |
i hate that much less. You also only need to write that when accessing the underlying sdk |
Technically there are some breaking changes here, but they're either done in a non-breaking way or they're breaking features I do not think are used (and are not documented).
Closes #425
server
property tomcp
inMcpAgent
and updated related examples.mcp
property tomcpClientManager
inAgent
class to avoid naming conflicts.server
property inMcpAgent
with a deprecation warning.