Skip to content

Update AnyNotification to include notification params #86

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

Merged
merged 3 commits into from
Apr 27, 2025

Conversation

stallent
Copy link
Contributor

@stallent stallent commented Apr 22, 2025

Motivation and Context

When the client receives a notification from the server, the params that are included are not accessible from the client. It appears they were intended to be considering the README (specifcally the ResourceUpdatedNotification example), but they in fact come through as an instance of Empty().

How Has This Been Tested?

Example note...

event: message
id: _GET_stream_1745335731559_yxppqadb
data: {"method":"notifications/message","jsonrpc":"2.0","params":{"level":"info","data":"Hello, world! 2025-04-22T15:28:51.558Z"}}

Example client code

struct GenericNotification: MCP.Notification, Sendable {
    static var name: String { "notifications/message" }
}
...
await client.onNotification(GenericNotification.self) { message in
    print("NOTE PARAMS: \(message.params)") // prints Empty()
}
struct GenericNotification: MCP.Notification, Sendable {
    static var name: String { "notifications/message" }
    typealias Parameters = Value
}
...
await client.onNotification(GenericNotification.self) { message in
    print("NOTE PARAMS: \(message.params.data)") // prints ["level": info, "data": Hello, world! 2025-04-22T16:34:14.059Z]
}
struct GenericNotification: MCP.Notification, Sendable {
    static var name: String { "notifications/message" }
    
    public struct Parameters: Hashable, Codable, Sendable {
        let level:String
        let data:String
    }
}
...
await client.onNotification(GenericNotification.self) { message in
    print("NOTE PARAMS: \(message.params.data)") // prints Hello, world! 2025-04-22T15:29:31.652Z
}

Breaking Changes

none

Types of changes

  • [x ] 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
  • [n/a] I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@mattt
Copy link
Contributor

mattt commented Apr 27, 2025

Thanks for catching this, @stallent. Just kicked off CI for this. I agree that this was an oversight, and this looks like the right fix.

@mattt
Copy link
Contributor

mattt commented Apr 27, 2025

Hang on, this is a repeat of #22 🫠. Adding some more test coverage to make sure we get this right.

Copy link
Contributor

@mattt mattt left a comment

Choose a reason for hiding this comment

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

Thanks again for your help with this, @stallent!

@mattt mattt merged commit 99517b0 into modelcontextprotocol:main Apr 27, 2025
4 checks passed
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