You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Is your feature request related to a problem? Please describe.
This library is hard to subclass and leverage in other projects. There are some relatively small changes to make it much extensible and easier to consume. Overly complex method overloading has caused bugs when extending the base classes. Using typescript is very hard because you have to discover the internal file structure to understand how types are exported. Some private member variables should be defined as. protected or public or protected utility methods should be added. Some simple cleanup in the code is needed for efficiency.
Describe the solution you'd like
Overly complex method overloading has caused bugs when extending classes. (opened separate issue). The current approach is just too brittle and results in a complex method to check parameters and fallbacks instead of simply defining a simple, and more declarative definition:
requires consumers to get the order correct, and much worse -- bundling two unrelated types as one parameter like -->
paramsSchemaOrAnnotations: Args | ToolAnnotation
makes it really confusing.
Please consider adding to a simple too() method with a nicely typed props or config object with specific elements for each type of object -- it will be easier to read, and configure, and less prone to error. You can add a secondary method if you want to keep the prior as backwards compatable.
import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js";
ugh? I have to find like 5 different files based on the type I'm importing, and then they are postfixed with .js??
just export them all with unique names like import {McpServer, Transport} from "@mcp/types" or something simple -- this is an sdk -- should be super simple to import without main types without having to get into the weeds of the actual library
Consider making some elements like _registeredTools protected or add a helper method and fire the event when it changes. I've had subclass the main class to work around a bug -- but now i have to keep a fork active because internal member variables are private -- again as it is an sdk -- it should be open enough to subclass with fair warnings to the consumer of the service
General code cleanup. While debugging some of these other items, noticed some poor logic conditions like
for (const message of messages) {
onMessage?(message)
}
checking a conditional on each iteration instead of wrapping it in an if clause to check if the for loop should even execute -- saw this a couple of times
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered:
Strongly agree with revisiting the tool registration path. The current overloading structure is pretty challenging to work with! Adding a well-typed options object to register tools with seems like a no-brainer, though I haven't taken as deep of a look here as Pete has.
Is your feature request related to a problem? Please describe.
This library is hard to subclass and leverage in other projects. There are some relatively small changes to make it much extensible and easier to consume. Overly complex method overloading has caused bugs when extending the base classes. Using typescript is very hard because you have to discover the internal file structure to understand how types are exported. Some private member variables should be defined as. protected or public or protected utility methods should be added. Some simple cleanup in the code is needed for efficiency.
Describe the solution you'd like
For example:
tool(name: string, cb: ToolCallback): RegisteredTool;
tool(name: string, description: string, cb: ToolCallback): RegisteredTool;
.....
requires consumers to get the order correct, and much worse -- bundling two unrelated types as one parameter like -->
paramsSchemaOrAnnotations: Args | ToolAnnotation
makes it really confusing.
Please consider adding to a simple too() method with a nicely typed props or config object with specific elements for each type of object -- it will be easier to read, and configure, and less prone to error. You can add a secondary method if you want to keep the prior as backwards compatable.
ugh? I have to find like 5 different files based on the type I'm importing, and then they are postfixed with .js??
just export them all with unique names like import {McpServer, Transport} from "@mcp/types" or something simple -- this is an sdk -- should be super simple to import without main types without having to get into the weeds of the actual library
Consider making some elements like _registeredTools protected or add a helper method and fire the event when it changes. I've had subclass the main class to work around a bug -- but now i have to keep a fork active because internal member variables are private -- again as it is an sdk -- it should be open enough to subclass with fair warnings to the consumer of the service
General code cleanup. While debugging some of these other items, noticed some poor logic conditions like
for (const message of messages) {
onMessage?(message)
}
checking a conditional on each iteration instead of wrapping it in an if clause to check if the for loop should even execute -- saw this a couple of times
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context or screenshots about the feature request here.
The text was updated successfully, but these errors were encountered: