Skip to content

Conversation

@joulei
Copy link
Contributor

@joulei joulei commented Aug 14, 2025

Closes #349

Motivation and Context

The transport-streamable-http-client feature makes use of the reqwest crate, but does not declare it as a component feature in the rmcp Cargo.toml. This prevents a project with rmcp from being compiled if they make use of the feature.

How Has This Been Tested?

I've managed to now compile my application using transport-streamable-http-client.

Breaking Changes

No

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@github-actions github-actions bot added T-dependencies Dependencies related changes T-config Configuration file changes labels Aug 14, 2025
@4t145
Copy link
Collaborator

4t145 commented Aug 15, 2025

How about moving this block to crates/rmcp/src/transport/common/reqwest/streamable_http_client.rs

The "this block" I say means the convertion from reqwest::Error to StreamableHttpError<reqwest::Error> in crates/rmcp/src/transport/streamable_http_client.rs

impl From<reqwest::Error> for StreamableHttpError<reqwest::Error> {
    fn from(e: reqwest::Error) -> Self {
        StreamableHttpError::Client(e)
    }
}

This will allow user to choose the client.

- Fix compilation error when using transport-streamable-http-client feature due to missing dependency
- Move From<reqwest::Error> implementation to reqwest module
@github-actions github-actions bot added T-core Core library changes T-transport Transport layer changes labels Aug 15, 2025
@joulei
Copy link
Contributor Author

joulei commented Aug 15, 2025

How about moving this block to crates/rmcp/src/transport/common/reqwest/streamable_http_client.rs

The "this block" I say means the convertion from reqwest::Error to StreamableHttpError<reqwest::Error> in crates/rmcp/src/transport/streamable_http_client.rs

impl From<reqwest::Error> for StreamableHttpError<reqwest::Error> {
    fn from(e: reqwest::Error) -> Self {
        StreamableHttpError::Client(e)
    }
}

This will allow user to choose the client.

Good catch, updated.
Let me know if this is what you expected (I imagined you meant this as a incremental improvement) or if you you meant this as not hardcoding the reqwest dependency into the feature and already making this configurable from the outside.

@4t145
Copy link
Collaborator

4t145 commented Aug 15, 2025

I mean the latter. I have to admit that it could be confusing for user, so maybe we should add some guide, maybe in the document of StreamableHttpClientTransport

@github-actions github-actions bot added T-documentation Documentation improvements T-examples Example code changes labels Aug 15, 2025
@joulei
Copy link
Contributor Author

joulei commented Aug 15, 2025

@4t145 Ok, I've made the transports client-agnostic by:

  • Moving reqwest-specific code to reqwest/ modules for both streamable HTTP and SSE clients
  • Creating separate reqwest features (don't love the verbosity but I guess is very obvious):
    • transport-streamable-http-client-reqwest
    • transport-sse-client-reqwest
  • Making base features reqwest-independent so users can choose their HTTP client

As you'll see I had to fix the same issue for SSE client since it was also forcing reqwest dependencies.
Also I did have to update the HeaderValue type to use String in the base modules to avoid reqwest-specific types.

Let me know if this what you what you had in mind.

@4t145
Copy link
Collaborator

4t145 commented Aug 18, 2025

Looks good now, just fix the ci

- Added reqwest features for reqwest-based implementations.
- Updated documentation
- Modified error handling in SSE transport to use `String` for content type.
- Updated examples to include new features
@joulei
Copy link
Contributor Author

joulei commented Aug 18, 2025

Oops sorry should pass now

@4t145
Copy link
Collaborator

4t145 commented Aug 19, 2025

format check CI failed, just use just fmt

- Added reqwest features for reqwest-based implementations
- Updated documentation
- Modified error handling in SSE transport to use `String`
- Updated examples to include new features
@github-actions github-actions bot added T-test Testing related changes T-handler Handler implementation changes labels Aug 19, 2025
@github-actions github-actions bot removed T-test Testing related changes T-handler Handler implementation changes labels Aug 19, 2025
@4t145
Copy link
Collaborator

4t145 commented Aug 20, 2025

Thanks!

@4t145 4t145 merged commit 7f20a87 into modelcontextprotocol:main Aug 20, 2025
10 of 11 checks passed
@github-actions github-actions bot mentioned this pull request Aug 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-config Configuration file changes T-core Core library changes T-dependencies Dependencies related changes T-documentation Documentation improvements T-examples Example code changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The transport-streamable-http-client feature should include reqwest

2 participants