Skip to content

fix: use RequestInit['headers'] in normalizeHeaders - #3129

Open
theOguz16 wants to merge 2 commits into
modelcontextprotocol:v1.xfrom
theOguz16:fix/v1-normalize-headers-type
Open

theOguz16 wants to merge 2 commits into
modelcontextprotocol:v1.xfrom
theOguz16:fix/v1-normalize-headers-type

Conversation

@theOguz16

Copy link
Copy Markdown

Problem

normalizeHeaders is exported with a bare HeadersInit parameter. HeadersInit is declared by the DOM lib only, so consumers that compile the SDK without DOM fail:

"lib": ["ES2023"], "types": ["node"], "skipLibCheck": false
→ dist/cjs/shared/transport.d.ts(7,51): error TS2304: Cannot find name 'HeadersInit'.

RequestInit is declared globally by @types/node (via undici-types) as well as by DOM, so indexing into it resolves in both environments.

Change

-export function normalizeHeaders(headers: HeadersInit | undefined): Record<string, string> {
+export function normalizeHeaders(headers: RequestInit['headers'] | undefined): Record<string, string> {

This matches the signature already on main (packages/core-internal/src/shared/transport.ts), so it is a v1 backport of the same shape rather than a new API decision.

The HeadersInit references left in src/client/sse.ts and src/client/streamableHttp.ts are local variables and never reach the emitted declarations, so they are intentionally untouched to keep the diff minimal.

Verification

Packed the branch and type-checked a consumer with "lib": ["ES2023"], "types": ["node"], skipLibCheck: false:

  • @modelcontextprotocol/sdk@1.30.0 → TS2304 (see issue)
  • this branch → clean compile, and the emitted declaration reads headers: RequestInit['headers']

Also ran npm run typecheck, eslint src/shared/transport.ts, prettier --check, and vitest run test/shared/transport.test.ts (50/50).

A patch changeset is included.

Fixes #2568

The exported signature referenced the bare global HeadersInit, which is
declared by the DOM lib only. Node-only consumers that compile with
"lib": ["ES2023"], "types": ["node"] and skipLibCheck: false failed
with TS2304 on the emitted dist/{esm,cjs}/shared/transport.d.ts.

RequestInit is also provided globally by @types/node via undici-types, so
its indexed access type resolves without DOM. This matches the signature
already shipped on main.

Refs modelcontextprotocol#2568
@theOguz16
theOguz16 requested a review from a team as a code owner October 29, 2026 19:12
@changeset-bot

changeset-bot Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9cfb5d7

The changes in this PR will be included in the next version bump.

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

@pkg-pr-new

pkg-pr-new Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@modelcontextprotocol/sdk@3129

commit: 9cfb5d7

@arhancanli

Copy link
Copy Markdown

Read c2dae37 and grepped src for other DOM-only names that could reach emitted declarations (HeadersInit, BodyInit, RequestCache, RequestMode). The only remaining HeadersInit uses are the two locals in client/sse.ts:148 and client/streamableHttp.ts:216, so your claim that they never reach .d.ts holds. RequestInfo there is the SDK's own interface from types.ts, not the DOM one. I did not run the build.

Two small things:

  • The JSDoc on normalizeHeaders (line 6) still says "Normalizes HeadersInit"; harmless, but it will read oddly next to the new signature.
  • Nothing stops the bare global coming back in a later change. A tiny consumer fixture compiled with lib: [ES2023], types: [node], skipLibCheck: false against the packed tarball would catch it, since the current typecheck runs with DOM available. Is that something you would add here or leave for a separate PR?

@theOguz16

Copy link
Copy Markdown
Author

Thanks — that grep is the check I should have run myself, and it is reassuring that the two locals in client/sse.ts / client/streamableHttp.ts really are the only remaining ones.

  • Updated the doc comment in 9cfb5d7: it now says RequestInit['headers'], so the DOM-only name does not survive in prose next to the new signature.
  • On the fixture: tsconfig.json currently has skipLibCheck: true, so the existing typecheck could never catch this class of regression. A consumer fixture only helps if something actually runs it, which means a script plus a CI step — a different kind of change from a one-line type fix. I would rather open that as a follow-up PR so this one stays reviewable, but if you would prefer it here I will add it.

@arhancanli

Copy link
Copy Markdown

A follow-up PR is the right call; this one stays a one-line type fix that is easy to review, and I have nothing further on it.

For the fixture, the cheapest version I can think of is a tiny consumer file that imports the built .d.ts and is checked with tsc --noEmit --skipLibCheck false --lib es2022 (no dom), so any leaked DOM-only name fails there. Would it fit as a step in the existing build job, or does it need its own so a missing dom lib shows up on its own line?

@theOguz16

Copy link
Copy Markdown
Author

A step in the existing build job fits, right after npm run build — the check needs the artifact, and each step gets its own line in the job log, so a missing-dom failure still shows up on its own.

I prepared it as a stacked branch (test/consumer-node-only-fixture): a test/consumer/ fixture that imports @modelcontextprotocol/sdk/shared/transport.js through a paths mapping onto dist/esm, with "lib": ["ES2023"], "types": ["node"], "skipLibCheck": false, plus npm run check:consumer wired into the build job.

Verified both directions: with the RequestInit['headers'] signature it passes; flipping the emitted dist declaration back to HeadersInit reproduces TS2304 there. I will open it as a PR as soon as this one lands, since the fixture cannot pass against v1.x before the fix is in.

@arhancanli

Copy link
Copy Markdown

Good, that covers both directions. One thing to settle in the follow-up: skipLibCheck: false will also check @types/node and every other declaration the fixture pulls in, so a later @types/node bump can fail the build step for a reason unrelated to the SDK. Pinning types to ["node"] as you have helps, but is the fixture going to use the repo's lockfile version of @types/node, or its own? That decides whether a red step means the SDK broke.

@claude claude Bot added the v1 Issues / PRs related to v1.x label Oct 30, 2026
@theOguz16

Copy link
Copy Markdown
Author

It uses the repo's own @types/node from the lockfile — the fixture lives inside the workspace and has no separate install, so the version is fixed per commit.

That means a dependency bump is the only thing that can move it, and if it does, the red step shows up in that bump PR, which is where someone should be looking anyway. I would rather keep that than pin a second copy of @types/node for the fixture and have the check disagree with what the rest of the repo compiles against.

"types": ["node"] already keeps the surface to that one package; if a future bump makes it noisy in practice, we can tighten the fixture then.

@arhancanli

Copy link
Copy Markdown

Agreed, a red step in the bump PR is the right place for it, and one shared @types/node keeps the fixture honest about what the repo itself compiles against.

One thing about the "flip dist back to HeadersInit" check you ran by hand: will it stay a one-off, or do you want it as a negative control in CI (a second tiny file that deliberately uses a DOM-only name and is expected to fail tsc)? Without it, a future change to the paths mapping or lib could make the fixture pass vacuously and nobody would see it.

@theOguz16

Copy link
Copy Markdown
Author

Not a one-off — you're right that without a control the fixture can go vacuous, so I added one to the stacked branch.

  • test/consumer/dom-only.ts uses a DOM-only name (HeadersInit) and should never compile under lib: ["ES2023"].
  • test/consumer/expect-dom-only-failure.mjs runs tsc over it and requires exactly one outcome: a non-zero exit and TS2304 in the output. A clean build means the fixture environment drifted and the positive check is no longer testing anything; any other error means the control itself broke.

Verified the guard by simulating the drift — adding DOM to the negative config's lib — and the script fails with "the DOM-only control compiled, so the fixture is no longer DOM-free". Restored, it passes.

So npm run check:consumer is now tsc -p test/consumer/tsconfig.json && node test/consumer/expect-dom-only-failure.mjs, still one step after npm run build.

@arhancanli

Copy link
Copy Markdown

Simulating the drift by adding DOM to the negative config is the right test of the guard, and the two-sided condition (non-zero exit and the expected code) is what stops it from passing on any unrelated failure.

One tightening: TS2304 alone is satisfied by any Cannot find name, so if someone later adds a second identifier to dom-only.ts that is missing for another reason (a typo, a removed @types/node global), the control still passes. Matching the whole diagnostic, i.e. dom-only.ts(<line>,<col>): error TS2304: Cannot find name 'HeadersInit', ties it to the one name it exists to test. Do you parse tsc --pretty false output for that, or would you rather keep the code-only check and keep the file to a single identifier?

@theOguz16

Copy link
Copy Markdown
Author

Tightened to match the whole diagnostic rather than the code alone: error TS2304: Cannot find name 'HeadersInit', parsed from tsc --pretty false.

I left line and column out on purpose — they move whenever the file changes and would make the guard brittle — but the message ties it to the one name the control exists to test, so an unrelated "Cannot find name" no longer satisfies it.

Checked all three outcomes:

  • normal: passes
  • DOM added to the negative config's lib: fails with "the DOM-only control compiled"
  • DOM added and an unrelated missing identifier in dom-only.ts: fails with expected "error TS2304: Cannot find name 'HeadersInit'", got: … 'NotAThing'

The file stays a single identifier anyway, but the guard no longer depends on that being true.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v1 Issues / PRs related to v1.x

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants