Repository navigation
Simplified, Express-like API - #117
Conversation
| return expr | ||
| .slice(operator.length) | ||
| .split(",") | ||
| .map((name) => name.replace("*", "").trim()) |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI almost 2 years ago
To fix the problem, we need to ensure that all occurrences of the asterisk character are removed from the name string. This can be achieved by using a regular expression with the global flag (g) in the replace method. This change will ensure that every asterisk in the string is replaced, not just the first one.
| @@ -118,3 +118,3 @@ | ||
| .split(",") | ||
| .map((name) => name.replace("*", "").trim()) | ||
| .map((name) => name.replace(/\*/g, "").trim()) | ||
| .filter((name) => name.length > 0); |
| for (let i = 0; i < names.length; i++) { | ||
| const { name, exploded } = names[i]; | ||
| const value = match[i + 1]; | ||
| const cleanName = name.replace("*", ""); |
Check failure
Code scanning / CodeQL
Incomplete string escaping or encoding
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI almost 2 years ago
To fix the problem, we need to ensure that all occurrences of the asterisk (*) character in the name string are replaced. This can be achieved by using a regular expression with the global flag (g). Specifically, we should replace name.replace("*", "") with name.replace(/\*/g, "").
| @@ -300,3 +300,3 @@ | ||
| const value = match[i + 1]; | ||
| const cleanName = name.replace("*", ""); | ||
| const cleanName = name.replace(/\*/g, ""); | ||
|
|
|
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). |
jerome3o-anthropic
left a comment
There was a problem hiding this comment.
This looks awesome! very clean. Some questions - one concern about the RFC 6570 encoding of spaces otherwise
Once that's resolved 🚀 ship it 🚀
jerome3o-anthropic
left a comment
There was a problem hiding this comment.
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
Introduced a few months ago in v1.3.0 of the MCP SDK. modelcontextprotocol/typescript-sdk#117
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.