-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(compat): re-export StreamableHTTPServerTransport + EventStore types from /node #1904
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
Open
felixweinberger
wants to merge
1
commit into
main
Choose a base branch
from
fweinberger/v2-bc-transport-aliases
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+31
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/node': patch | ||
| --- | ||
|
|
||
| Add v1-compat re-exports: `StreamableHTTPServerTransport` (alias for `NodeStreamableHTTPServerTransport`) and the `EventStore` / `EventId` / `StreamId` types, so v1 imports from `@modelcontextprotocol/sdk/server/streamableHttp.js` map cleanly onto `@modelcontextprotocol/node`. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,8 @@ | ||
| export * from './streamableHttp.js'; | ||
|
|
||
| // v1-compat re-exports. | ||
| export { | ||
| /** v1 name. The v2-preferred name is {@linkcode NodeStreamableHTTPServerTransport}. */ | ||
| NodeStreamableHTTPServerTransport as StreamableHTTPServerTransport | ||
| } from './streamableHttp.js'; | ||
|
Check warning on line 7 in packages/middleware/node/src/index.ts
|
||
| export type { EventId, EventStore, StreamId } from '@modelcontextprotocol/server'; | ||
|
claude[bot] marked this conversation as resolved.
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { describe, expect, expectTypeOf, it } from 'vitest'; | ||
|
|
||
| import { NodeStreamableHTTPServerTransport, StreamableHTTPServerTransport } from '../src/index.js'; | ||
| import type { EventId, EventStore, StreamId } from '../src/index.js'; | ||
|
|
||
| describe('v1 compat exports from @modelcontextprotocol/node', () => { | ||
| it('StreamableHTTPServerTransport aliases NodeStreamableHTTPServerTransport', () => { | ||
| expect(StreamableHTTPServerTransport).toBe(NodeStreamableHTTPServerTransport); | ||
| expectTypeOf<StreamableHTTPServerTransport>().toEqualTypeOf<NodeStreamableHTTPServerTransport>(); | ||
| }); | ||
|
|
||
| it('re-exports EventStore / EventId / StreamId types', () => { | ||
| // Type-level assertions: these compile only if the types are exported. | ||
| expectTypeOf<EventId>().toBeString(); | ||
| expectTypeOf<StreamId>().toBeString(); | ||
| expectTypeOf<EventStore>().toHaveProperty('storeEvent'); | ||
| expectTypeOf<EventStore>().toHaveProperty('replayEventsAfter'); | ||
| }); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🟡 This supersedes the earlier inline comment, which now misquotes the diff after the force-push — the current code has no
@deprecatedtag (line 5 reads only "v1 name. The v2-preferred name is…"), no "keep until v3" comment, and noeslint-disablein the test. That's internally consistent with the existingStreamableHTTPServerTransportOptionsalias, but it contradicts the PR description's "additive@deprecatedshim. Removed in v3" framing: consumers of the v1 name get zero deprecation signal at any level. Either restore@deprecatedvia theexport const+export typepattern so it survives dts bundling, or drop the "Removed in v3" claim from the description/changeset to match the code.Extended reasoning...
What changed since the last review
The earlier claude[bot] inline comment on this file quotes source that no longer exists in the PR. It references:
/** @deprecated Use {@linkcode NodeStreamableHTTPServerTransport}. Removed in v3. */// v1-compat re-exports — keep until v3.eslint-disable @typescript-eslint/no-deprecatedline incompat.test.tsNone of those appear in the current single-commit diff (e5af3b3, force-pushed over the earlier 3d48f5e per the pkg-pr-new comment). The current state is:
and
compat.test.tsimportsStreamableHTTPServerTransportwith no lint suppression.grep -r '@deprecated' packages/middleware/returns zero hits. So the previous comment's premise ("@deprecated is present in source but stripped by the bundler") is no longer accurate — there is now no@deprecatedanywhere, source or dist.The remaining mismatch
The PR description's Breaking Changes section still says:
That no longer describes the code. What ships is a plain alias with an informational JSDoc hint (and per the previous review's build evidence, even that hint is dropped from
dist/index.d.mtsbecause tsdown flattens export specifiers). A consumer who writesimport { StreamableHTTPServerTransport } from '@modelcontextprotocol/node'gets no IDE strikethrough and no@typescript-eslint/no-deprecatedwarning.Step-by-step
packages/middleware/node/src/index.tscontains the JSDoc/** v1 name. The v2-preferred name is {@linkcode NodeStreamableHTTPServerTransport}. */— no@deprecatedtoken.compat.test.tsline 3 importsStreamableHTTPServerTransportdirectly, with no// eslint-disable-next-line @typescript-eslint/no-deprecated. Lint passes, confirming the symbol is not marked deprecated even at source level.pnpm --filter @modelcontextprotocol/node build→dist/index.d.mtsemits the alias in a flattenedexport { … NodeStreamableHTTPServerTransport as StreamableHTTPServerTransport … }line with no JSDoc at all (same bundler behaviour the previous comment documented).Why this might be intentional
The pre-existing
StreamableHTTPServerTransportOptionsalias atstreamableHttp.ts:22also carries no@deprecated— just a "for backward compatibility" note. So dropping@deprecatedhere is consistent with prior art in this package. The changeset file (.changeset/node-transport-v1-aliases.md) also doesn't claim deprecation, so what's published to npm is self-consistent. The only thing out of sync is the PR description.Two valid resolutions
(a) If v3 removal is real, restore the deprecation in a form that survives dts bundling:
(and add the
eslint-disableback to the test, and consider doing the same forStreamableHTTPServerTransportOptions).(b) If the alias is meant to be a stable, non-deprecated convenience export (matching the Options precedent), just drop "
@deprecatedshim. Removed in v3" from the PR description so the record matches what ships.