Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| function protoOf(target: object): ClassMeta { | ||
| return (target as { constructor: { prototype: ClassMeta } }).constructor.prototype; | ||
| } |
There was a problem hiding this comment.
🟡 nit: For legacy instance-method decorators, target is already the class prototype, so target.constructor.prototype === target and this indirection is a no-op. It's also a footgun: if a user applies @McpTool to a static method, target is the constructor, target.constructor is Function, and ownArray ends up writing __mcpTools onto the global Function.prototype. Consider simplifying to return target as ClassMeta; (and optionally noting that only instance methods are supported).
Extended reasoning...
What the issue is
protoOf(target) returns (target as …).constructor.prototype. In legacy TypeScript method decorators, the first argument for an instance method is already the class's prototype object. So target.constructor is the class constructor, and target.constructor.prototype is… the same prototype you started with. The function is a round-trip identity — return target as ClassMeta would do exactly the same thing for every call site in this file.
The indirection isn't just redundant; it implies there's a reason target itself isn't the right object, which can mislead readers of what's explicitly positioned as "copy the ~60 lines of decorator plumbing into your own project" reference code.
The static-method footgun
For a static method, legacy decorators pass the constructor function as target. Then:
target.constructor→Function(every JS function's constructor isFunction)target.constructor.prototype→ the globalFunction.prototype
ownArray(Function.prototype, '__mcpTools') would then define __mcpTools as an own property on Function.prototype, polluting every function and class in the process. And registerClass wouldn't even find those entries, since Object.getPrototypeOf(instance) is the class prototype, whose chain doesn't include Function.prototype — so the user gets silent global pollution and an unregistered tool.
Why nothing prevents it
protoOf's signature accepts any object, and the decorator return types don't constrain target to a prototype. TypeScript will happily compile @McpTool({...}) static foo(). There's no typeof target === 'function' guard or comment limiting the decorators to instance methods.
Step-by-step proof
class C {
@McpTool({ name: 'x' })
static x(): CallToolResult { return { content: [] }; }
}- Legacy emit calls the decorator with
target = C(the constructor). protoOf(C)→C.constructor.prototype→Function.prototype.ownArray(Function.prototype, '__mcpTools')—Object.hasOwn(Function.prototype, '__mcpTools')is false, so it assignsFunction.prototype.__mcpTools = []and pushes the entry.- Now
(() => {}).__mcpTools,Array.__mcpTools, every function everywhere, all see that array. registerClass(server, new C())readsObject.getPrototypeOf(instance).__mcpTools→C.prototype.__mcpTools→undefined. Tool is never registered.
Impact
The shipped example only decorates instance methods, so it runs correctly as-is — hence nit. But since this is copy-paste reference code, the redundant indirection is confusing and the static-method case is a real (if unlikely) global-pollution hazard.
Fix
function protoOf(target: object): ClassMeta {
return target as ClassMeta;
}Optionally inline it entirely (ownArray(target as ClassMeta, '__mcpTools')) and/or add a one-line note that the decorators are intended for instance methods only. With the simplified version, a static decorator would write to the constructor itself rather than Function.prototype — still not what registerClass reads, but no longer a global side-effect.
| // Stage-3 decorator metadata uses `Symbol.metadata`. Node < 22 does not | ||
| // ship it yet, so register it if missing. TypeScript's decorator emit | ||
| // writes to `ctor[Symbol.metadata]` during class initialisation and will | ||
| // crash without this polyfill on older Node. | ||
| ((Symbol as { metadata?: symbol }).metadata as symbol | undefined) ??= Symbol.for('Symbol.metadata'); |
There was a problem hiding this comment.
🟡 nit: This comment says "Node < 22 does not ship it yet" (and the PR description says the polyfill "is a no-op on Node 22+"), but Symbol.metadata has not shipped in any stable Node/V8 release — verified undefined on Node v22.22.2. Since this is copy-paste reference code, a Node 22+ user who reads this and drops the polyfill will get context.metadata === undefined from TypeScript's __esDecorate emit, and ownArray() will throw on Object.hasOwn(undefined, key). Suggest dropping the version number, e.g. "No current Node release ships it yet, so register it if missing."
Extended reasoning...
What the comment claims vs. what is true
Lines 23-26 read:
Stage-3 decorator metadata uses
Symbol.metadata. Node < 22 does not ship it yet, so register it if missing.
and the PR description's "Design notes" repeat the claim: "The Symbol.metadata polyfill is guarded (??=) so it is a no-op on Node 22+." Both statements imply that Node 22 (or later) ships Symbol.metadata natively. That is factually incorrect. Symbol.metadata is defined by the TC39 decorator-metadata proposal, which is still Stage 3 and has not been implemented in any stable V8, so no stable Node release — 20, 22, 23, or 24 — exposes it.
Empirical verification
On the review machine:
$ node -e 'console.log(process.version, typeof Symbol.metadata)'
v22.22.2 undefinedSo on the very Node line the comment singles out as "already shipping it", the polyfill is not a no-op — it is the only thing making the example work.
Why this matters for copy-paste reference code
The example as committed runs fine, because line 27 unconditionally executes (Symbol as …).metadata ??= Symbol.for('Symbol.metadata') regardless of Node version. The risk is purely in the documentation: this file is explicitly positioned as "copy the ~60 lines of decorator plumbing into your own project", and the comment gives readers a concrete version cutoff that invites them to delete the polyfill when targeting Node 22+.
Step-by-step proof of what happens if a Node 22 user drops the polyfill
- User is on Node 22.x, reads "Node < 22 does not ship it yet", concludes the polyfill line is dead code for their target, and removes line 27.
- At class-initialisation time, TypeScript's
__esDecoratehelper (emitted for Stage-3 decorators) computes the metadata object roughly as:Withvar metadata = typeof Symbol === "function" && Symbol.metadata ? Object.create(base?.[Symbol.metadata] ?? null) : void 0;
Symbol.metadataabsent,metadataisundefined, and thatundefinedis what gets passed ascontext.metadatato every method decorator. McpToolcallsownArray(metaOf(context), '__mcpTools').metaOfsimply returnscontext.metadatacast toMcpClassMetadata, i.e.undefined.ownArrayimmediately callsObject.hasOwn(undefined, '__mcpTools')→TypeError: Cannot convert undefined or null to object, thrown during class definition (module load), beforemain()ever runs.
So the misleading version number turns a one-line comment into a load-time crash for anyone who acts on it.
Why nothing else catches it
There is no runtime guard in metaOf() or ownArray() for a missing metadata object (nor should there be — the polyfill is the correct fix). TypeScript types context.metadata as DecoratorMetadataObject | undefined but the cast in metaOf discards the undefined, so the compiler won't flag it either.
Suggested fix
Reword the comment to avoid naming a specific Node version, since the real gate is V8 shipping the Stage-3 proposal, not a Node release line:
// Stage-3 decorator metadata uses `Symbol.metadata`. No current Node/V8
// release ships it yet, so register it if missing. TypeScript's decorator
// emit sets `context.metadata` to `undefined` when the symbol is absent,
// which would break `ownArray()` below.And drop the "no-op on Node 22+" sentence from the PR description's Design notes. The code itself needs no change.
1 participant
Adds a pair of self-contained examples showing how to author MCP tools, resources, and prompts as decorated methods on a class, in both decorator dialects TypeScript supports.
Motivation and Context
Follow-up to #116 ("Declarative, high-level MCP server API"). That thread evolved into a discussion of class-based
@Tool/@Resource/@Promptdecorators as an alternative to the function-basedMcpServerAPI. The issue was closed as not planned — decorators in TS/JS only work on class methods, so shipping them in the SDK would impose an OO style on everyone — with the position that this pattern is straightforward for users to build themselves on top of the existing SDK. These examples are the reference implementation of exactly that: copy-paste-friendly proof that the pattern works end-to-end, with no SDK changes required.The SDK's
McpServerAPI is function-based: you call.tool(),.resource(),.prompt()at registration time. That works well, but a recurring question from users is "can I group related handlers onto a class and register them declaratively?" — the answer is yes, with a small amount of decorator plumbing and no new SDK dependency.These examples show the full pattern end-to-end so users can copy ~60 lines into their own project:
@McpTool/@McpResource/@McpResourceTemplate/@McpPromptmethod decoratorsregisterClass(server, instance)helper that reflects over the class and wires each decorated method intoMcpServerTool,Prompt,Resource,ResourceTemplateType) viaOmit<…, identity>, so any spec-level field (icons, annotations,_meta, …) is automatically supported without touching the exampleBecause
experimentalDecoratorsis project-wide in TypeScript, covering both dialects requires a second tsconfig — which is itself useful to document.How Has This Been Tested?
pnpm tsx src/legacyClassDecoratorsExample.ts(uses the package's existingexperimentalDecorators: truetsconfig).pnpm tsx --tsconfig=tsconfig.standard-decorators.json src/classDecoratorsExample.ts.pnpm --filter @modelcontextprotocol/server-examples typechecknow runstsgoagainst both tsconfigs, so the standard-decorators file is type-checked in CI.Breaking Changes
None. These are additive example files; no SDK code changes.
Types of changes
Checklist
Additional context
Background references
experimentalDecorators.classDecoratorsExample.ts.Design notes
(target, propertyKey, descriptor)vs(value, context)) and howregisterClassretrieves the collected metadata. This makes them easy to diff if you're porting one codebase to the other.reflect-metadata. The legacy version stashes arrays on the prototype; the standard version uses Stage-3's built-incontext.metadataobject.Symbol.metadatapolyfill is guarded (??=) so it is a no-op on Node 22+.