-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(core): add | undefined to Transport optional callbacks for exactOptionalPropertyTypes compatibility #1661
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@modelcontextprotocol/core': patch | ||
| --- | ||
|
|
||
| Fix TS2420 error when implementing Transport with exactOptionalPropertyTypes enabled | ||
|
|
||
| Optional callback properties on the Transport interface (`onclose`, `onerror`, `onmessage`, `setProtocolVersion`, `setSupportedProtocolVersions`) now explicitly include `| undefined` in their type signature. This makes the interface compatible with TypeScript's `exactOptionalPropertyTypes` compiler option, which was previously causing TS2420 "Class incorrectly implements interface" errors for users with that flag enabled. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| /** | ||
| * Compile-time type checks for the Transport interface. | ||
| * | ||
| * Verifies that a class declaring optional callback properties as `T | undefined` | ||
| * (the pattern required by `exactOptionalPropertyTypes: true`) is assignable to | ||
| * Transport without TS2420 errors. | ||
| * | ||
| * See: https://github.com/modelcontextprotocol/typescript-sdk/issues/1314 | ||
| */ | ||
| import { test } from 'vitest'; | ||
|
|
||
| import type { Transport } from '../../src/shared/transport.js'; | ||
| import type { JSONRPCMessage, MessageExtraInfo } from '../../src/types/types.js'; | ||
|
|
||
| // A concrete class that uses the explicit `| undefined` union form for optional callbacks. | ||
| // With the old Transport interface (no `| undefined` on callbacks), this class would produce | ||
| // TS2420 under `exactOptionalPropertyTypes: true`. | ||
| class ExplicitUndefinedTransport implements Transport { | ||
| sessionId?: string | undefined; | ||
| onclose?: (() => void) | undefined; | ||
| onerror?: ((error: Error) => void) | undefined; | ||
| onmessage?: (<T extends JSONRPCMessage>(message: T, extra?: MessageExtraInfo) => void) | undefined; | ||
| setProtocolVersion?: ((version: string) => void) | undefined; | ||
| setSupportedProtocolVersions?: ((versions: string[]) => void) | undefined; | ||
|
|
||
| async start(): Promise<void> {} | ||
| async close(): Promise<void> {} | ||
| async send(_message: JSONRPCMessage): Promise<void> {} | ||
| } | ||
|
|
||
| test('Transport allows explicit | undefined on optional callback properties', () => { | ||
| const transport: Transport = new ExplicitUndefinedTransport(); | ||
| // The mere fact this file compiles is the assertion. | ||
| // We also verify runtime assignability here. | ||
| expect(transport).toBeDefined(); | ||
| }); | ||
|
Comment on lines
+15
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 This type test does not actually verify Extended reasoning...The issueThe test file Why it does not workThe project tsconfig chain is: Without Step-by-step proof
ImpactThe test cannot catch regressions. If a future change accidentally removes the How to fixEither:
Note: The actual code fix in |
||
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.
🟡 The
sessionIdproperty at line 122 is missing| undefined, making the fix incomplete. All five callback properties got| undefinedadded, butsessionId?: stringwas left unchanged. The test class itself declaressessionId?: string | undefined(line 19), which would fail TS2420 against the current interface underexactOptionalPropertyTypes: true.Extended reasoning...
What the bug is
This PR adds
| undefinedto the five optional callback/method properties on theTransportinterface to fixexactOptionalPropertyTypescompatibility. However, thesessionIdproperty at line 122 was not updated:Why this matters
Under
exactOptionalPropertyTypes: true, TypeScript treatsT | undefinedandTas distinct types in optional property contexts. A class that declaressessionId?: string | undefinedis not assignable to an interface that declaressessionId?: string. This produces the same TS2420 error that this PR is trying to fix.Proof via the test file
The PR's own test class at
transport.types.test.ts:19declares:This class compiles today only because the project does not have
exactOptionalPropertyTypesenabled. Under that flag, the test would fail with TS2420 onsessionIdspecifically, since the interface still hassessionId?: stringwithout| undefined.Step-by-step example
exactOptionalPropertyTypes: truein theirtsconfig.jsonTransportwithsessionId?: string | undefined(a natural pattern, and the exact pattern shown in the test)sessionId?: string(no| undefined)exactOptionalPropertyTypes,string | undefinedis not assignable tostringin optional property positionFix
Change line 122 of
packages/core/src/shared/transport.tsfrom:to:
This is consistent with the treatment applied to all other optional properties in this PR and matches the pattern already used in the test class.