fix(core): add | undefined to Transport optional callbacks for exactOptionalPropertyTypes compatibility#1661
Conversation
…xactOptionalPropertyTypes compatibility Resolves TS2420 errors when users implement the Transport interface in projects with `exactOptionalPropertyTypes: true` enabled. Optional callback properties (onclose, onerror, onmessage, setProtocolVersion, setSupportedProtocolVersions) now include explicit `| undefined` in their types, satisfying the stricter assignability rules. Fixes modelcontextprotocol#1314
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
🦋 Changeset detectedLatest commit: 5275ece The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@claude review |
|
|
||
| /** | ||
| * The session ID generated for this connection. |
There was a problem hiding this comment.
🟡 The sessionId property at line 122 is missing | undefined, making the fix incomplete. All five callback properties got | undefined added, but sessionId?: string was left unchanged. The test class itself declares sessionId?: string | undefined (line 19), which would fail TS2420 against the current interface under exactOptionalPropertyTypes: true.
Extended reasoning...
What the bug is
This PR adds | undefined to the five optional callback/method properties on the Transport interface to fix exactOptionalPropertyTypes compatibility. However, the sessionId property at line 122 was not updated:
sessionId?: string; // should be: sessionId?: string | undefined;Why this matters
Under exactOptionalPropertyTypes: true, TypeScript treats T | undefined and T as distinct types in optional property contexts. A class that declares sessionId?: string | undefined is not assignable to an interface that declares sessionId?: 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:19 declares:
class ExplicitUndefinedTransport implements Transport {
sessionId?: string | undefined; // line 19
...
}This class compiles today only because the project does not have exactOptionalPropertyTypes enabled. Under that flag, the test would fail with TS2420 on sessionId specifically, since the interface still has sessionId?: string without | undefined.
Step-by-step example
- User enables
exactOptionalPropertyTypes: truein theirtsconfig.json - User writes a class implementing
TransportwithsessionId?: string | undefined(a natural pattern, and the exact pattern shown in the test) - TypeScript sees that the interface declares
sessionId?: string(no| undefined) - Under
exactOptionalPropertyTypes,string | undefinedis not assignable tostringin optional property position - TS2420 error: "Class incorrectly implements interface Transport"
Fix
Change line 122 of packages/core/src/shared/transport.ts from:
sessionId?: string;to:
sessionId?: string | undefined;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.
| // 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(); | ||
| }); |
There was a problem hiding this comment.
🟡 This type test does not actually verify exactOptionalPropertyTypes compatibility because the project tsconfig does not enable that flag (it is not part of strict mode). Without exactOptionalPropertyTypes: true in the compilation, TypeScript treats prop?: T and prop?: T | undefined identically, so this test passes with or without the PR changes. Consider adding a dedicated tsconfig with exactOptionalPropertyTypes: true for this test file, or using @ts-expect-error annotations to assert the old signatures would fail.
Extended reasoning...
The issue
The test file transport.types.test.ts is intended to verify that the updated Transport interface works correctly under exactOptionalPropertyTypes: true. The comment at lines 16-17 explicitly states: "With the old Transport interface (no | undefined on callbacks), this class would produce TS2420 under exactOptionalPropertyTypes: true." However, the test is not actually compiled with that flag enabled.
Why it does not work
The project tsconfig chain is: packages/core/tsconfig.json → @modelcontextprotocol/tsconfig (at common/tsconfig/tsconfig.json). The base config sets "strict": true but does not set "exactOptionalPropertyTypes": true. This is a common misconception — exactOptionalPropertyTypes is a separate opt-in flag that is not included in strict mode.
Without exactOptionalPropertyTypes: true, TypeScript treats prop?: T and prop?: T | undefined as equivalent for assignability purposes. This means the ExplicitUndefinedTransport class would successfully implement the old Transport interface (without | undefined) just fine.
Step-by-step proof
- Consider the old interface:
onclose?: () => void; - The test class declares:
onclose?: (() => void) | undefined; - Without
exactOptionalPropertyTypes, TypeScript internally expandsonclose?: () => voidtoonclose?: (() => void) | undefinedanyway. - So the class property matches the interface property — no TS2420 error, regardless of the PR changes.
- If someone reverted the
| undefinedadditions intransport.ts, this test would still pass.
Impact
The test cannot catch regressions. If a future change accidentally removes the | undefined from the Transport interface properties, this test will continue to pass, giving false confidence that the exactOptionalPropertyTypes fix is still in place.
How to fix
Either:
- Create a separate
tsconfig.exactOptional.jsonthat extends the base config and adds"exactOptionalPropertyTypes": true, then configure this test to use it. - Use
@ts-expect-errorannotations with a negative test case that asserts the old signatures would fail. - Add a triple-slash directive or use
tsd/dtslintfor more robust type-level testing.
Note: The actual code fix in transport.ts is correct and valuable for downstream consumers who enable exactOptionalPropertyTypes. This is purely a test quality issue.
|
Thanks for the contribution! Closing in favor of #1766. |
|
Thank you Felix! |
Summary
Fixes TS2420 errors when implementing the
Transportinterface in projects withexactOptionalPropertyTypes: trueenabled.The five optional callback/method properties on
Transportpreviously lacked explicit| undefined:With
exactOptionalPropertyTypes: true, TypeScript treatsT | undefinedandTas distinct in optional property contexts. A concrete class that declaresonclose?: (() => void) | undefinedis not assignable to an interface that declaresonclose?: () => voidunder this flag, producing TS2420.The fix adds explicit
| undefinedto all five optional members:This is backwards-compatible: projects not using
exactOptionalPropertyTypesare unaffected.Breaking Changes
None.
Test plan
packages/core/test/shared/transport.types.test.ts— compile-time type test verifying a class with explicit| undefinedcallbacks implementsTransportwithout errorspnpm typecheck:allpasses clean across all packagesFixes #1314