-
Notifications
You must be signed in to change notification settings - Fork 0
Align JavaScript SDK with existing Python SDK #1
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
* Exclude node_modules, build artifacts, and development files * Include standard Node.js and TypeScript ignore patterns * Prevent accidental commits of generated content
* Add JavaScript-style method aliases to McpdClient: * getServers() -> servers() * getTools() -> tools() * getServerHealth() -> serverHealth() * getAgentTools() -> agentTools() * Remove internal class exports (DynamicCaller, ServerProxy, FunctionBuilder, AgentFunction) * Maintain full backward compatibility with Python-style methods * Align public API surface with Python SDK conventions
* Replace all Python-style method calls with JavaScript equivalents * Maintain comprehensive test coverage (41 tests passing) * Verify backward compatibility through aliasing approach
* Update all examples to demonstrate JavaScript-style method calls * Revise README.md API documentation to show new method names * Update code snippets and usage patterns throughout documentation * Maintain backward compatibility notes where appropriate
* Add comprehensive project documentation (SECURITY, CODE_OF_CONDUCT, CONTRIBUTING) * Ensure proper open source project structure * Provide clear guidelines for contributors and security reporting
const health = await client.getServerHealth(); | ||
for (const [serverName, serverHealth] of Object.entries(health)) { |
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.
nit: I'm assuming this returns an object where the keys are the server names and the values are an object containing the health status? if so we could rename it to something that encompasses that better like healthResponseByServer / healthGroupedByServer or serverHealthsMap or similar
const health = await client.getServerHealth(); | |
for (const [serverName, serverHealth] of Object.entries(health)) { | |
const healthByServer = await client.getServerHealth(); | |
for (const [serverName, serverHealth] of Object.entries(healthByServer)) { |
console.log(); | ||
|
||
// Check if get_current_time tool exists | ||
if (await client.hasTool('time', 'get_current_time')) { |
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.
nit: this doesn't seem very clear because its calling client.hasTool
when clients don't really have tools, it also expects users to know about both the server name and the tool name so I'm not sure what its usecase is; alas I would rename it to something like client.hasServerWithTool
or similar, wdyt?
const utcTime = await client.call.time.get_current_time({ timezone: 'UTC' }); | ||
console.log(' UTC:', utcTime); | ||
|
||
const tokyoTime = await client.call.time.get_current_time({ timezone: 'Asia/Tokyo' }); | ||
console.log(' Tokyo:', tokyoTime); | ||
|
||
const nyTime = await client.call.time.get_current_time({ timezone: 'America/New_York' }); | ||
console.log(' New York:', nyTime); | ||
} |
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.
.call
is a reserved method on functions in javascript so it can be confusing https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call
Is there really a need for this intermediary call
object? how about calling it directly with client.time
or client.servers.time
instead? Would be nice to have the tool calls as camelCase aswell
try { | ||
// Try to call a non-existent tool | ||
await client.call.nonexistent_server.nonexistent_tool(); | ||
} catch (error) { | ||
if (error instanceof McpdError) { | ||
console.log(` Caught expected error: ${error.name} - ${error.message}`); | ||
} else { | ||
throw error; | ||
} | ||
} | ||
|
||
} catch (error) { | ||
if (error instanceof McpdError) { | ||
console.error('mcpd error:', error.message); | ||
|
||
// Show specific error handling | ||
if (error.name === 'ConnectionError') { | ||
console.error('Cannot connect to mcpd daemon. Is it running?'); | ||
console.error('Start it with: mcpd start'); | ||
} else if (error.name === 'AuthenticationError') { | ||
console.error('Authentication failed. Check your API key.'); | ||
} | ||
} else { | ||
console.error('Unexpected error:', error); | ||
} | ||
process.exit(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.
nested try/catches can be difficult to follow through, I would rather consolidate them into a single try/catch with conditional logic based on the error type accordingly
try { | |
// Try to call a non-existent tool | |
await client.call.nonexistent_server.nonexistent_tool(); | |
} catch (error) { | |
if (error instanceof McpdError) { | |
console.log(` Caught expected error: ${error.name} - ${error.message}`); | |
} else { | |
throw error; | |
} | |
} | |
} catch (error) { | |
if (error instanceof McpdError) { | |
console.error('mcpd error:', error.message); | |
// Show specific error handling | |
if (error.name === 'ConnectionError') { | |
console.error('Cannot connect to mcpd daemon. Is it running?'); | |
console.error('Start it with: mcpd start'); | |
} else if (error.name === 'AuthenticationError') { | |
console.error('Authentication failed. Check your API key.'); | |
} | |
} else { | |
console.error('Unexpected error:', error); | |
} | |
process.exit(1); | |
} | |
try { | |
// Try to call a non-existent tool | |
await client.call.nonexistent_server.nonexistent_tool(); | |
} catch (error) { | |
if (error instanceof McpdError) { | |
console.error('mcpd error:', error.message); | |
// Show specific error handling | |
if (error.name === 'ConnectionError') { | |
console.error('Cannot connect to mcpd daemon. Is it running?'); | |
console.error('Start it with: mcpd start'); | |
} else if (error.name === 'AuthenticationError') { | |
console.error('Authentication failed. Check your API key.'); | |
} | |
} else { | |
console.error('Unexpected error:', error); | |
} | |
process.exit(1) | |
} |
if (require.main === module) { | ||
main().catch(console.error); | ||
} |
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.
if (require.main === module) { | |
main().catch(console.error); | |
} | |
main().catch(console.error); |
private readonly endpoint: string; | ||
private readonly apiKey: string | undefined; | ||
private readonly timeout: number; | ||
private readonly serverHealthCache: LRUCache<string, ServerHealth | Error>; | ||
private readonly functionBuilder: FunctionBuilder; | ||
private readonly cacheableExceptions = new Set([ | ||
ServerNotFoundError, | ||
ServerUnhealthyError, | ||
AuthenticationError, | ||
]); |
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.
nit: private
in typescript doesn't really mean private see https://stackoverflow.com/questions/12713659/why-can-i-access-typescript-private-members-when-i-shouldnt-be-able-to
if you really want to make these private you can prefix the variable/method names with #
see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Classes/Private_elements
* console.log(servers); // ['time', 'fetch', 'git'] | ||
* ``` | ||
*/ | ||
async servers(): Promise<string[]> { |
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.
nit: would be nice to have function methods include a verb like getServers
or fetchServes
or similar, but I understand if we want to keep it consistent with the python sdk version
return new Proxy(this, { | ||
get: (target, serverName: string | symbol) => { | ||
if (typeof serverName !== 'string') { | ||
return undefined; | ||
} | ||
return new ServerProxy(target.client, serverName); | ||
}, | ||
}); | ||
} |
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 unnecessarily complicated, do we really need to use a proxy?
return new Proxy(this, { | ||
get: (target, toolName: string | symbol) => { | ||
if (typeof toolName !== 'string') { | ||
return undefined; | ||
} | ||
|
||
// Return a function that will call the tool | ||
return async (args?: Record<string, any>) => { | ||
// Check if the tool exists | ||
const hasTool = await target.client.hasTool(target.serverName, toolName); | ||
if (!hasTool) { | ||
throw new ToolNotFoundError( | ||
`Tool '${toolName}' not found on server '${target.serverName}'. ` + | ||
`Use client.tools('${target.serverName}') to see available tools.`, | ||
target.serverName, | ||
toolName | ||
); | ||
} | ||
|
||
// Perform the tool call | ||
return target.client._performCall(target.serverName, toolName, args); | ||
}; | ||
}, | ||
}); | ||
} |
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.
same here, I'm not sure it adds much value compared to directly calling the corresponding methods or having it directly accessible without a proxy, whats the idea behind having a caller or a .call
proxy on the client as opposed to just calling/finding the server and/or its tools accordingly via the other methods?
{ | ||
"name": "@mozilla-ai/mcpd", | ||
"version": "0.1.0", | ||
"description": "JavaScript/TypeScript SDK for interacting with mcpd", |
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 we need to add a "type": "module"
to support es6 imports https://stackoverflow.com/questions/61401475/why-is-type-module-in-package-json-file
- Add zod dependency for JSON Schema to Zod conversion - Add centralized API path constants in apiPaths.ts - Update package-lock.json for new dependencies This establishes the foundation for dual framework compatibility and cleaner API path management in the SDK.
- Update AgentFunction interface for LangChain JS and Vercel AI SDK compatibility - Add JSON Schema to Zod conversion with proper type handling - Use .nullable().optional() for OpenAI structured outputs compatibility - Fix empty schema handling to return z.object({}) instead of errors - Add invoke/execute methods for framework-specific execution patterns - Replace __name__/__doc__ with standard name/description properties This enables generated functions to work directly with both AI frameworks without requiring conversion layers.
- Add ToolFormat type ('array' | 'object' | 'map') for cross-framework compatibility - Add TypeScript overloads for getAgentTools() with proper return type inference - Replace framework-specific methods with single data structure-based approach - Update API path handling to use centralized apiPaths constants - Align tools() method behavior with Python SDK patterns - Improve tool call error handling and response parsing - Fix tool call payload format to match mcpd daemon expectations This delivers the unified developer experience where getAgentTools() returns objects that work directly with both LangChain JS and Vercel AI SDK without requiring conversion functions.
See: https://github.com/mozilla-ai/mcpd-sdk-python