fix(client): Some mcp server need default env(#393, #196)#394
fix(client): Some mcp server need default env(#393, #196)#394ihrpr merged 12 commits intomodelcontextprotocol:mainfrom
Conversation
…ontextprotocol#393, modelcontextprotocol#196) Signed-off-by: sunrabbit123 <qudwls185@naver.com>
sunrabbit123
changed the title
| test("should work with actual node mcp server", async () => { | ||
| const client = new StdioClientTransport({ | ||
| command: "npx", | ||
| args: ["-y", "@wrtnlabs/calculator-mcp"], |
There was a problem hiding this comment.
it's the simple mcp server
i make it for test
There was a problem hiding this comment.
There was a problem hiding this comment.
we should not be using real servers in tests
…led(modelcontextprotocol/typescript-sdk#394) Signed-off-by: sunrabbit123 <qudwls185@naver.com>
| test("should work with actual node mcp server", async () => { | ||
| const client = new StdioClientTransport({ | ||
| command: "npx", | ||
| args: ["-y", "@wrtnlabs/calculator-mcp"], |
There was a problem hiding this comment.
we should not be using real servers in tests
Signed-off-by: sunrabbit123 <qudwls185@naver.com>
|
@ihrpr can i ask you to request review? |
|
Thank you for working on this. Sorry about the delays in reviews. I'm closing it for now as it's a breaking change that could affect existing implementations. |
|
The method presented in #196 works quite well. The reason this approach is effective is that when users inject an Env, the SDK essentially ignores the default environment variables used internally and exclusively uses the environment variables provided by the user. However, if we shift this responsibility entirely to users, it could potentially create significant challenges when developing third-party libraries that utilize this SDK. Most MCP users generally prefer configuration files that work seamlessly out of the box, as typically no one really wants to manually retrieve environment variables and configure them in their own setup files. |
|
The current env merging logic differs from our Python SDK implementation. The Python SDK uses a different approach for this - we should align both SDKs to maintain consistency. env: ({**get_default_environment(), **server.env} if server.env is not None else get_default_environment())This creates inconsistent behavior across SDKs. related PR's related issue's |
ihrpr
left a comment
ihrpr
left a comment
There was a problem hiding this comment.
Thank you for pointing out inconsistency between SDKs! Sorry, I didn't realise it was changed recently.
Some comments around tests 🙏 and then we can merge
| [], | ||
| expect.objectContaining({ | ||
| env: customEnv | ||
| env: { |
There was a problem hiding this comment.
but.. i don't understand this comment, can i ask you to explain with detail?
Signed-off-by: sunrabbit123 <qudwls185@naver.com>
|
thank you for your comment, i fix it! @ihrpr but, i don't understand some comment sorry |
Signed-off-by: sunrabbit123 <qudwls185@naver.com>
Signed-off-by: sunrabbit123 <qudwls185@naver.com>
ihrpr
left a comment
ihrpr
left a comment
There was a problem hiding this comment.
Thank you!
(simplified tests a bit)
2 participants
Motivation and Context
ref: #393, #196
How Has This Been Tested?
npm run testBreaking Changes
We now always include the default environment variables when executing.
Types of changes
Checklist
Additional context