Skip to content

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