-
Notifications
You must be signed in to change notification settings - Fork 1.6k
v2 Context API in callbacks
#1241
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
base: main
Are you sure you want to change the base?
v2 Context API in callbacks
#1241
Conversation
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
mattzcarey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big fan
🦋 Changeset detectedLatest commit: 9b0ed8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Will now have to be a Since we can now afford breaking changes, will rework Note to self: original context interface was done pre-tasks, need a type generic version with is aware when taskStore, taskId etc. is there so that the end user does not need to check on property existence in tasks handlers TODO:
Edit: All todos done |
v2 Context API in callbacks
|
Note to self: Found some fixes to do, where EDIT: done |
…ntContextInterface respectively, add examples
…ypescript-sdk into feature/ctx-in-callbacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[deleted erroneously posted review]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[deleted]
dismissing own in progress review
| data: `Periodic notification #${counter} at ${new Date().toISOString()}` | ||
| }, | ||
| extra.sessionId | ||
| ctx.mcpCtx.sessionId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[style] would this be cool if it were ctx.mcp.sessionId rather than repeating ctx. Same with requestCtx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @KKonstantinov thanks for this!
I think the motivation of trying to improve RequestHandlerExtra to not be a catch-all grab bag of stuff is good. Making convenience methods available easily on the context is great.
Especially separation into Server/Client Context seems sound. However, this PR introduces pretty significant type & class hierarchy complexity, to the point where I'm not sure we're making things really easier to maintain here. It looks like we went from a single type:
- RequestHandlerExtraTo having 14 new types and generics:
- McpContext (type)
- BaseRequestContext (type)
- TaskContext (type)
- BaseContextArgs (interface)
- ContextInterface (interface, 3 generics)
- BaseContext (abstract class, 4 generics, implements ContextInterface)
- ServerRequestContext (type, extends BaseRequestContext)
- ServerContextInterface (interface, 2 generics, extends ContextInterface)
- LoggingMessageNotificationSenderInterface (interface)
- ServerLogger (class, implements LoggingMessageNotificationSenderInterface)
- ServerContext (class, 3 generics, extends BaseContext, implements ServerContextInterface)
- ClientRequestContext (type, extends BaseRequestContext)
- ClientContextInterface (type alias, 2 generics)
- ClientContext (class, 3 generics, extends BaseContext, implements ClientContextInterface)Is this really the simplest way we can clean up? Would it be possible for example to do something like this instead:
// Shared base - just a type, not a class
type BaseContext = {
requestId: RequestId;
method: string;
sessionId?: string;
signal: AbortSignal;
_meta?: RequestMeta;
taskCtx?: TaskContext; // grouped because conditionally present
sendNotification: (...) => Promise<void>;
sendRequest: (...) => Promise<...>;
};
// Server adds its stuff
type ServerContext = BaseContext & {
authInfo?: AuthInfo;
headers: Headers;
closeSSEStream?: () => void;
closeStandaloneSSEStream?: () => void;
// Convenience methods
log: LoggingHelper;
requestSampling: (...) => Promise<...>;
elicitInput: (...) => Promise<...>;
};
// Client is minimal
type ClientContext = BaseContext;
// TaskContext stays grouped (conditional)
type TaskContext = {
id: string;
store: RequestTaskStoreInterface;
requestedTtl: number | null;
};If not why not? Is there anything we're enabling with this complexity that we can't get with a couple of types?
I.e. we'd only separate out Server and Client context and have tasks have its own context. TaskContext is grouped because all its fields are always present or absent together, so a single if (ctx.taskCtx) gives you type narrowing on everything -- that's a justified reason to nest. But mcpCtx and requestCtx are always present and always fully populated, so nesting them just adds verbosity for the most common operations (ctx.mcpCtx.requestId vs ctx.requestId).
I had a look at some other major frameworks and they all seem to err on the side of keeping the context quite flat (Express, Koa, Hono). The Python SDK also exposes the convenience methods at the top level (Context class -- ctx.request_id, ctx.log(), ctx.info(), ctx.elicit() etc. are all directly on the context).
| /** | ||
| * Sends a sampling request to the client. | ||
| */ | ||
| public requestSampling(params: CreateMessageRequest['params'], options?: RequestOptions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requestSampling doesn't pass relatedRequestId, but elicitInput below does ({ ...options, relatedRequestId: this.mcpCtx.requestId }). This means sampling requests sent via the context won't be associated with the originating request on the transport. This is a regression -- the old extra.sendRequest path handled this correctly.
| /** | ||
| * Sends a logging message. | ||
| */ | ||
| public async log(params: LoggingMessageNotification['params'], sessionId?: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ServerLogger calls this.server.sendLoggingMessage() directly, bypassing the context's sendNotification. This means logging via ctx.loggingNotification.info(...) won't include relatedRequestId or relatedTask metadata, unlike calling ctx.sendNotification() directly. The logger should be scoped to the request context.
| /** | ||
| * Logger for sending logging messages to the client. | ||
| */ | ||
| loggingNotification: LoggingMessageNotificationSenderInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the name loggingNotification leaks protocol implementation details into the user-facing API. Consider log to match the Python SDK's pattern.
| signal: abortController.signal, | ||
| authInfo: extra?.authInfo, | ||
| // URL is not available in MessageExtraInfo, use a placeholder | ||
| uri: new URL('mcp://request'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exposes a hardcoded placeholder on ServerRequestContext.uri, typed as URL (not optional). Users accessing ctx.requestCtx.uri will get mcp://request for both stdio and HTTP transports. Consider making uri optional (URL | undefined) or removing it until the transport layer can actually populate it.
| protected buildMcpContext(args: { request: JSONRPCRequest; sessionId: string | undefined }): { | ||
| requestId: RequestId; | ||
| method: string; | ||
| _meta: Record<string, unknown> | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: McpContext types _meta as RequestMeta, but this return type annotation says Record<string, unknown> | undefined. Should be consistent.
This PR introduces a new Context API for request handlers in the TypeScript SDK, replacing the flat
RequestHandlerExtratype with a structured, extensible context system.The new context provides:
mcpCtx,requestCtx,taskCtx)ServerContext) and client (ClientContext) handlersMotivation and Context
The previous
RequestHandlerExtrahad several design issues:1. POJO Creation Overhead
Every incoming request created a new plain JavaScript object with all properties and methods inline:
This approach has performance implications:
sendNotificationandsendRequestarrow functions are created as new function objects for every request, each capturing variables from the enclosing scope (request,relatedTaskId,taskStore,this). These closures are heap allocations that add GC pressure.The new Context API uses class instances with methods on the prototype. While both approaches allocate a new object per request, the class approach avoids per-request function allocations by referencing shared prototype methods instead.
2. Mixed Concerns: Properties Irrelevant to Client or Server
RequestHandlerExtrawas a single type used by bothClientandServer, but many properties only applied to one side:requestInfocloseSSEStreamcloseStandaloneSSEStreamauthInfotaskId,taskStore,taskRequestedTtlThis led to:
closeSSEStreamin autocomplete but it's always undefined?), making the type less useful for type checkingThe new Context API uses separate types (
ServerContext,ClientContext) with appropriate properties for each, and nested structures (requestCtx.stream) to group related optional features.3. Flat Structure Limits Extensibility
Adding new features to
RequestHandlerExtrameant:The new nested structure (
mcpCtx,requestCtx,taskCtx) provides clear namespaces for different concerns, making it easy to add features without polluting the top-level API.4. No Convenience Methods
Common operations like logging, sampling, and elicitation required manual construction:
The new Context API introduces a clean separation:
ctx.mcpCtx- MCP protocol-level context (requestId, method, sessionId, _meta)ctx.requestCtx- Transport/request-level context (signal, authInfo, and server-specific: uri, headers, stream controls)ctx.taskCtx- Task context when tasks are enabled (id, store, requestedTtl)sendNotification(),sendRequest()directly on the contextloggingNotification,requestSampling(),elicitInput()onServerContextHow Has This Been Tested?
test/integration/test/server/context.test.tsmcpCtx,requestCtx,taskCtx)Breaking Changes
This is a breaking change for code that accesses properties directly on the handler's second parameter:
extra._metactx.mcpCtx._metaextra.sessionIdctx.mcpCtx.sessionIdextra.requestIdctx.mcpCtx.requestIdextra.signalctx.requestCtx.signalextra.authInfoctx.requestCtx.authInfoextra.sendNotification(...)ctx.sendNotification(...)extra.sendRequest(...)ctx.sendRequest(...)Migration: Update property access paths as shown above. The
sendNotificationandsendRequestmethods remain at the top level of the context.Types of changes
Checklist
Additional context
Logging from ServerContext
Elicitation from ServerContext
Sampling from ServerContext
Accessing MCP Context
Task Context (when tasks are enabled)