v2 Context API in callbacks#1241
v2 Context API in callbacks#1241KKonstantinov wants to merge 36 commits intomodelcontextprotocol:mainfrom
v2 Context API in callbacks#1241Conversation
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
🦋 Changeset detectedLatest commit: b79763f 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 |
|
Note to self: Found some fixes to do, where EDIT: done |
…ntContextInterface respectively, add examples
…ypescript-sdk into feature/ctx-in-callbacks
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.
[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.
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.
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.
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.
Nit: the name loggingNotification leaks protocol implementation details into the user-facing API. Consider log to match the Python SDK's pattern.
packages/server/src/server/server.ts
Outdated
| 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.
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.
packages/core/src/shared/protocol.ts
Outdated
| protected buildMcpContext(args: { request: JSONRPCRequest; sessionId: string | undefined }): { | ||
| requestId: RequestId; | ||
| method: string; | ||
| _meta: Record<string, unknown> | undefined; |
There was a problem hiding this comment.
Minor: McpContext types _meta as RequestMeta, but this return type annotation says Record<string, unknown> | undefined. Should be consistent.
… into feature/ctx-in-callbacks
…ypescript-sdk into feature/ctx-in-callbacks
|
Oh no. @mattzcarey 😭 10+ files conflict :D |
c698143 to
b79763f
Compare
Replaces the flat RequestHandlerExtra type with a structured context system using 4 plain types: BaseContext, ServerContext, ClientContext, TaskContext. - Nested structure groups related fields (mcpReq, http, task, notification) - Separate ServerContext/ClientContext types for server and client handlers - Server-specific convenience methods (logging, sampling, elicitation) - Plain object (POJO) construction with closures - no classes - Type-safe contexts without runtime instanceof checks Based on the design from PR #1241, simplified from class hierarchy to plain types. BREAKING CHANGE: RequestHandlerExtra is replaced by BaseContext with nested structure. See docs/migration.md for migration guide.
Replaces the flat RequestHandlerExtra type with a structured context system using 4 plain types: BaseContext, ServerContext, ClientContext, TaskContext. - Nested structure groups related fields (mcpReq, http, task, notification) - Separate ServerContext/ClientContext types for server and client handlers - Server-specific convenience methods (logging, sampling, elicitation) - Plain object (POJO) construction with closures - no classes - Type-safe contexts without runtime instanceof checks Based on the design from PR #1241, simplified from class hierarchy to plain types. BREAKING CHANGE: RequestHandlerExtra is replaced by BaseContext with nested structure. See docs/migration.md for migration guide.
Replaces the flat RequestHandlerExtra type with a structured context system using 4 plain types: BaseContext, ServerContext, ClientContext, TaskContext. - Nested structure groups related fields (mcpReq, http, task, notification) - Separate ServerContext/ClientContext types for server and client handlers - Server-specific convenience methods (logging, sampling, elicitation) - Plain object (POJO) construction with closures - no classes - Type-safe contexts without runtime instanceof checks Based on the design from PR #1241, simplified from class hierarchy to plain types. BREAKING CHANGE: RequestHandlerExtra is replaced by BaseContext with nested structure. See docs/migration.md for migration guide.
Replaces the flat RequestHandlerExtra type with a structured context system using 4 plain types: BaseContext, ServerContext, ClientContext, TaskContext. - Nested structure groups related fields (mcpReq, http, task, notification) - Separate ServerContext/ClientContext types for server and client handlers - Server-specific convenience methods (logging, sampling, elicitation) - Plain object (POJO) construction with closures - no classes - Type-safe contexts without runtime instanceof checks Based on the design from PR #1241, simplified from class hierarchy to plain types. BREAKING CHANGE: RequestHandlerExtra is replaced by BaseContext with nested structure. See docs/migration.md for migration guide.
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)