Skip to content

Conversation

@4t145
Copy link
Collaborator

@4t145 4t145 commented May 18, 2025

  1. Reorganize features, remove tls related options
  2. Make reqwest a non-default dependent, so people can toggle its features theirselves.

Motivation and Context

fix error introduced by merge

How Has This Been Tested?

Breaking Changes

  1. rename feature transport-sse => transport-sse-client (it should be done in refactor: Transport trait and worker transport, and streamable http client with those new features. #167, but seems missed due to merge?)
  2. make reqwest client an optional choice.
  3. simplify tls provider featrues to reqwest and reqwest-tls-no-provider, otherwise there would be too many tls option features.

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

It breaks the changes of #155.
cc @msardara

@4t145 4t145 requested review from Copilot and jokemanfire May 18, 2025 03:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors feature flags to fix a merge error, streamlines TLS/provider options, and makes reqwest an optional dependency.

  • Renamed SSE transport feature (transport-ssetransport-sse-client) and consolidated TLS flags into reqwest and reqwest-tls-no-provider.
  • Updated example projects and documentation to use new feature names and bumped reqwest to v0.12.
  • Adjusted internal module cfg guards (__authauth, __transport-* → new feature names).

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
examples/simple-chat-client/src/config.rs Switched from SseTransport to SseClientTransport and updated call to start
examples/simple-chat-client/Cargo.toml Bumped reqwest version; replaced git dependency with workspace = true and updated rmcp features
examples/rig-integration/Cargo.toml Renamed transport-sse feature to transport-sse-client
examples/clients/Cargo.toml Renamed transport-sse to transport-sse-client and added reqwest feature
docs/OAUTH_SUPPORT.md Updated example to use transport-sse-client feature
crates/rmcp/src/transport/streamable_http_client.rs Changed feature guard from __auth to auth
crates/rmcp/src/transport/sse_client.rs Removed URL and join error variants; switched to auth guard
crates/rmcp/src/transport/common/reqwest.rs Aligned feature guard to transport-streamable-http-client
crates/rmcp/src/transport/common/auth.rs Aligned feature guard to transport-streamable-http-client
crates/rmcp/src/transport/common.rs Changed reqwest guard to __reqwest
crates/rmcp/src/transport.rs Updated multiple feature gates for SSE, auth, and streamable HTTP
crates/rmcp/Cargo.toml Reorganized feature definitions and updated test feature requirements
Comments suppressed due to low confidence (3)

crates/rmcp/src/transport/sse_client.rs:29

  • The Url error variant was removed, but parse errors may still occur; reintroduce or properly handle URL parsing failures to avoid unhandled errors or compilation issues.
    #[error("Url error: {0}")]

examples/simple-chat-client/Cargo.toml:16

  • [nitpick] Using workspace = true here while other examples use path may lead to inconsistent crate resolution; consider using a consistent path = "../../crates/rmcp" reference for local development.
rmcp = { workspace = true, features = [

crates/rmcp/Cargo.toml:150

  • [nitpick] The test required-features now includes __reqwest but not its TLS sub-features; ensure any dependent TLS flags (e.g., rustls-tls) are also enabled if tests exercise HTTP functionality.
    "__reqwest",

pub mod http_header;

#[cfg(feature = "reqwest")]
#[cfg(feature = "__reqwest")]
Copy link

Copilot AI May 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The doc(cfg(...)) annotation references feature = "reqwest" but the guard uses __reqwest, causing a mismatch in generated docs; align the documented feature name with the actual flag.

Suggested change
#[cfg(feature = "__reqwest")]
#[cfg(feature = "reqwest")]

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be bad looking to see an internal feature "__reqwest" in the document.

@4t145 4t145 changed the title refactor: fix error introduced by merge, reorganize features and outsource reqwest refactor: fix error introduced by merge, reorganize features May 18, 2025
@4t145 4t145 merged commit 5e4e77e into modelcontextprotocol:main May 18, 2025
9 checks passed
@gau-nernst gau-nernst mentioned this pull request May 19, 2025
3 tasks
@github-actions github-actions bot mentioned this pull request Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants