@W-22169002: Use composition over inheritance for Server class#321
@W-22169002: Use composition over inheritance for Server class#321anyoung-tableau wants to merge 4 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the internal Server wrapper to use composition rather than inheritance with the MCP SDK’s McpServer, enabling future scenarios where multiple server variants (e.g., web + desktop) can share a single McpServer instance.
Changes:
- Replace
Server extends McpServerwithServerholding areadonly mcpServer: McpServer. - Update transports and logging to call MCP SDK APIs via
server.mcpServer.*(connect/close/notification/registerTool). - Update unit tests and global test mocks to match the new
Servershape.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/pulse/listMetricSubscriptions/listPulseMetricSubscriptions.test.ts | Updates test mocking to stub server.mcpServer.registerTool. |
| src/testSetup.ts | Updates the mocked Server shape to include mcpServer.server.notification. |
| src/server/express.ts | Uses server.mcpServer.connect() / .close() with HTTP transport lifecycle. |
| src/server.ts | Core refactor: Server now composes an McpServer instance and delegates registrations/handlers through it. |
| src/server.test.ts | Updates expectations and stubs to use server.mcpServer.registerTool and server.mcpServer.server.setRequestHandler. |
| src/logging/notification.ts | Sends notifications via server.mcpServer.server.notification. |
| src/logging/notification.test.ts | Updates notification assertions to match the new nested server path. |
| src/index.ts | Uses server.mcpServer.connect() for stdio transport startup. |
| package.json | Bumps package version to 1.18.8. |
| package-lock.json | Updates lockfile version metadata to 1.18.8. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| res.on('close', () => { | ||
| transport.close(); | ||
| server.close(); | ||
| server.mcpServer.close(); |
There was a problem hiding this comment.
Small concern with exposing mcpServer directly now that Server is composition-based
We already reach through it for connect() and close(), and res.on('close') closing the shared inner server feels risky if we ever have multiple wrappers over one McpServer
Do we want to keep mcpServer private and expose wrapper-owned lifecycle methods instead, so the ownership model stays clear?
|
|
||
| registerRequestHandlers = (): void => { | ||
| this.server.setRequestHandler(SetLevelRequestSchema, async (request) => { | ||
| this.mcpServer.server.setRequestHandler(SetLevelRequestSchema, async (request) => { |
There was a problem hiding this comment.
Will this leave us exposed to handler collisions in the shared-server case?
setRequestHandler(SetLevelRequestSchema, ...) is a single slot on the underlying SDK server, so if multiple wrappers call registerRequestHandlers() on the same McpServer, the later registration wins
Do we want that handler owned at the shared layer instead of per-wrapper?
These changes update the
Serverclass so it no longer inherits from theMcpServerclass from the SDK and instead keeps an instance ofMcpServerinside it. This change is being made to ultimately support a world where multipleServerinstances can contain a sharedMcpServerinstance. Concrete example:WebMcpServerandDesktopMcpServercan both exist and share a single instance ofMcpServer, allowing each to register their tools simultaneously. Seeindex.combined.tsin #331