-
Notifications
You must be signed in to change notification settings - Fork 757
Simplified, Express-like API #117
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
Conversation
Ready for early review. This needs README updates and probably some other niceties, but would love to get any feedback on the APIs as defined and implemented. Note that most of the added lines are tests. |
README updated too now. Ready for review. |
Rather than using optional parameters, would it be better to pass along an options object? Something like: server.tool({
name: "save",
description: "It saves stuff, ofc",
}, async () => {
return {
content: [
{
type: "text",
text: "Saved successfully.",
},
],
};
}); Having optional parameters might have limiting implications for compatibility in future versions (i.e. adding a new field will be Hard) |
IMO too many named parameters makes the API unreadable, even though they should ostensibly be improving its understandability. I don't think backward compatibility is a massive concern here, since changes would also have to go through the spec (where it's a much bigger constraint than the SDK itself). |
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 looks awesome! very clean. Some questions - one concern about the RFC 6570 encoding of spaces otherwise
Once that's resolved 🚀 ship it 🚀
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.
Awesome
One parting thought - how do progress notifications / resource update notifications fit into this nicely? perhaps this could be added to RequestHandlerExtra
in the future? From what I see we can add that after this is merged.
Yeah, something like this. I agree it can come later. |
@jspahrsummers It's your api but tend to differ in my experience that having named parameters makes it harder to read, especially without ide dev hints. When I first looked at this I was wondering what the first two params are because there are overloads. When do I provide a schema and when is it correct to give the chat context? server.tool("echo", { text: z.string() }, ...
server.tool(
"add",
"Adds two numbers together", ... The consumer needs to know what the magic of the first two params is, it makes extending it require overloads. React query ,v1-v3 had this concept of a "query key" and query function that was the first parameters and they resolved it by making it a named param because they had overloads that caused issues with typing and knowing which version of the api was being used. I think we can learn from their evolution to named params. In addition, if we look at the apis that this looks most similar to we can look to ts-rest, and the vercel ai sdk we can see that named params are used. This also makes the rule simpler, instead of "make the first or second param defined and then the second or third param the options object" to be "use named params always" in addition to the points raised by @anaisbetts. Let me add that this api definitely looks a lot better than before and simpler, it's just that the named params (which can still be supported with another, and hopefully final, overload) could make this api even better 😄 |
…/ashwin/state refactor: extract draggable pane and connection logic into hooks
Inspired by #116 and some of the MCP SDK wrappers that have popped up in the ecosystem, this is an attempt to bring a more Express-style API into the SDK.
This diverges from the existing wrapper libraries through liberal use of overloads to achieve a highly ergonomic API supporting a variable number of parameters.
Zod (already used in the SDK) is further utilized to declare tool input shapes, and perform automatic parsing and validation. I tried to limit its syntactic overhead, though.
Examples
Tools
Resources
Prompts
Alternatives considered
Decorators, although still not fully standardized in ECMAScript, are at stage 3 and already supported by TypeScript. I believe it would be possible to use them to craft some nice APIs that could be used like so:
However, decorators are too foreign to many JS developers, and unfortunately are not supported on free functions—they can only annotate class members—so would not achieve an idiomatic API quite like FastMCP in the Python SDK.
A decorator-based API might be best as a community extension to the base SDK, which needs to be accessible to as wide an audience as possible.