chore: switch to module ESNext + moduleResolution bundler#2095
chore: switch to module ESNext + moduleResolution bundler#2095mattzcarey wants to merge 1 commit into
Conversation
Internal-only build configuration change. Consumers are not affected:
they continue to import from the built `.mjs`/`.d.mts` files declared
in each package's `exports` map.
What changed:
- `common/tsconfig/tsconfig.json`: `module: NodeNext` → `module: ESNext`,
`moduleResolution: NodeNext` → `moduleResolution: bundler`.
- `examples/{client,server}-quickstart/tsconfig.json`: same flip
(they extend a different base and overrode the resolution).
- Strip `.js` extensions from every relative TypeScript import across
packages/, examples/, scripts/, test/.
- Update CLAUDE.md to reflect the new import convention.
Why:
- Removes the long-standing footgun of having to write `from './foo.js'`
in `.ts` source files. Bundler resolution treats the path as a module
reference and lets the tooling resolve it.
- Aligns with what the bundler (`tsdown`), vitest, and downstream
consumers' bundlers actually do at runtime.
Verification: `pnpm typecheck:all`, `pnpm lint:all`, `pnpm test:all`,
`pnpm build:all` all pass.
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "module": "ESNext", | ||
| "moduleResolution": "bundler", |
There was a problem hiding this comment.
🔴 This change flips the two quickstart tsconfig.json files to ESNext/bundler, but docs/client-quickstart.md (lines 88–104) and docs/server-quickstart.md (lines 103–118) still tell users to create a tsconfig with "module": "Node16" / "moduleResolution": "Node16" — those blocks are plain ```json fences (no source=), so `sync:snippets --check` doesn't catch the drift. Since the quickstarts are user-facing templates that build with plain `tsc` and run the emitted `./build/index.js` directly under Node (no bundler involved), I'd lean toward leaving these two tsconfigs on `Node16`/`NodeNext` and only flipping the shared `common/tsconfig`; otherwise the two doc files need updating to match.
Extended reasoning...
What the issue is. This PR changes examples/client-quickstart/tsconfig.json and examples/server-quickstart/tsconfig.json from "module": "Node16" / "moduleResolution": "Node16" to "module": "ESNext" / "moduleResolution": "bundler". However, the user-facing quickstart guides that these example projects mirror — docs/client-quickstart.md (the "Create a tsconfig.json" block at lines 88–104) and docs/server-quickstart.md (lines 103–118) — were not updated and still instruct users to write "module": "Node16" / "moduleResolution": "Node16". Before this PR the two example tsconfigs and the two doc snippets were field-for-field identical; after it they diverge on module/moduleResolution.
Why CI doesn't catch it. Only the .ts code regions in those docs are auto-synced via source=-tagged fences; the tsconfig blocks are plain ```json fences with no source= attribute, so `pnpm sync:snippets --check` (which the PR description lists as passing under `pnpm lint:all`) does not compare them against the example files. This is exactly the case the repo review checklist calls out: "behavior change: check whether docs/**/.md describes the old behavior and needs updating; flag prose that now contradicts the implementation"*.
Why bundler is questionable for these two projects specifically. The PR description justifies moduleResolution: bundler on the basis that "tsdown, vitest, and downstream consumers' bundlers" handle resolution. That is true for the published packages, but the quickstarts are different: examples/client-quickstart/package.json has "build": "tsc" and "bin": { "mcp-client-cli": "./build/index.js" }, and examples/server-quickstart/package.json has "build": "tsc && chmod 755 build/index.js" with "bin": "./build/index.js" and "type": "module". They are compiled with raw tsc and the emitted .js is executed directly by Node — no bundler is involved at any stage. These two tsconfigs were standalone (they don't extend common/tsconfig) and were deliberately kept on Node16 precisely because they are copy-paste starter templates for end users running under Node.
Step-by-step proof.
docs/client-quickstart.md:93–94reads"module": "Node16","moduleResolution": "Node16".- After this PR,
examples/client-quickstart/tsconfig.json:5–6reads"module": "ESNext","moduleResolution": "bundler". - The doc fence at line 88 is
```json(nosource=…), sosync:snippets --checkignores it. - The same holds for
docs/server-quickstart.md:107–108vsexamples/server-quickstart/tsconfig.json:4–5. - Result: the published quickstart docs and the in-repo example projects they are meant to mirror now disagree on
module/moduleResolution.
Impact. Today each quickstart has only a single src/index.ts with no relative imports, so the example build itself doesn't break, and a user who follows the (stale) docs verbatim still gets a working project. But the docs and examples are now inconsistent, and if a user following the new example config adds a second source file, module: ESNext + moduleResolution: bundler will let tsc accept extensionless relative imports and emit them unchanged — which then fail at runtime under Node's ESM loader (ERR_MODULE_NOT_FOUND). The Node16 setting that the docs still recommend is the one that protects against that.
How to fix. Two options:
- Preferred: revert
examples/client-quickstart/tsconfig.jsonandexamples/server-quickstart/tsconfig.jsontoNode16/Node16(orNodeNext/NodeNext) and only keep theESNext/bundlerflip incommon/tsconfig/tsconfig.json. The quickstarts are "how a downstream consumer sets up a project," not "how this monorepo builds," so the internal-build rationale doesn't apply to them. - Alternatively: update the
tsconfig.jsonblocks indocs/client-quickstart.mdanddocs/server-quickstart.mdto matchESNext/bundler(and consider giving those blocks asource=attribute sosync:snippetscatches future drift).
| vi.mock('../../src/client/auth.js', async () => { | ||
| const actual = await vi.importActual<typeof import('../../src/client/auth.js')>('../../src/client/auth.js'); | ||
| const actual = await vi.importActual<typeof import('../../src/client/auth')>('../../src/client/auth.js'); |
There was a problem hiding this comment.
🟡 The codemod missed the runtime string arguments to vi.mock('../../src/client/auth.js', ...) and vi.importActual('../../src/client/auth.js') on lines 7-8 — these are the only two surviving relative .js specifiers in any .ts source after this PR. Line 8 is even half-migrated: the typeof import('../../src/client/auth') type argument was stripped but the adjacent runtime string was not. Not a runtime break (vitest normalizes both forms to the same module so the mock still applies), just a consistency cleanup to finish the migration.
Extended reasoning...
What was missed
This PR migrates every relative TypeScript import in the repo from the ./foo.js form to extensionless ./foo, and updates CLAUDE.md to say "no .js extension on relative imports". A repo-wide grep after the change shows exactly two surviving relative .js specifiers in .ts sources, both in packages/client/test/client/middleware.test.ts:
vi.mock('../../src/client/auth.js', async () => {
const actual = await vi.importActual<typeof import('../../src/client/auth')>('../../src/client/auth.js');
...
});The PR explicitly edited this file — it changed lines 4, 5, 16, and even the typeof import('../../src/client/auth') type argument on line 8 itself — but left the two adjacent runtime string literals untouched. Line 8 in particular is now visibly half-migrated: the type argument has no .js, the runtime argument right next to it still does.
Why the codemod missed it
The migration appears to have targeted ESM import/export declarations and import() expressions (it did update dynamic await import('../../src/client/auth.js') calls in streamableHttp.test.ts and validators.test.ts). vi.mock(...) and vi.importActual(...) take their specifier as an ordinary string argument to a function call, not as a syntactic import, so they would not be caught by an AST pass over import nodes.
Why nothing breaks
Vitest's module resolver normalizes '../../src/client/auth.js' and '../../src/client/auth' to the same on-disk file (packages/client/src/client/auth.ts). The system-under-test (middleware.ts) was migrated to import from './auth', and the test file's static import { auth, extractWWWAuthenticateParams } from '../../src/client/auth' on line 16 was also migrated. Because all three resolve to the same module record, the vi.mock('.../auth.js') factory still intercepts the extensionless imports, mockAuth / mockExtractWWWAuthenticateParams are still the mocked functions, and the test suite continues to pass.
Step-by-step proof of the leftover
- Before the PR, line 8 read:
vi.importActual<typeof import('../../src/client/auth.js')>('../../src/client/auth.js'). - The PR diff for this file changes the type argument to
typeof import('../../src/client/auth')(no.js), and changes the static import on line 16 tofrom '../../src/client/auth'. - The runtime argument
'../../src/client/auth.js'on line 8 and thevi.mockargument on line 7 are unchanged in the diff. grep -rn "'\.\./.*\.js'" packages/**/*.tsafter the PR returns only these two lines (the one other repo hit is aspawnarg pointing at a builtdist/file, which is unrelated to source-import resolution).- Therefore the PR's stated goal — "strip
.jsextensions from every relative TypeScript import" — is incomplete by exactly these two specifiers, in a file the PR otherwise edited.
Impact and fix
Pure consistency/cleanup: the file now mixes both conventions on adjacent lines, and these two strings are the only thing standing between the PR and a fully clean grep for the old pattern. Fix is to drop the .js suffix from both string literals:
vi.mock('../../src/client/auth', async () => {
const actual = await vi.importActual<typeof import('../../src/client/auth')>('../../src/client/auth');
...
});1 participant
Summary
common/tsconfig/tsconfig.jsonfrommodule: NodeNext/moduleResolution: NodeNexttomodule: ESNext/moduleResolution: bundler.Node16overrides inexamples/client-quickstart/tsconfig.jsonandexamples/server-quickstart/tsconfig.json..jsextensions from every relative TypeScript import acrosspackages/,examples/,scripts/, andtest/.CLAUDE.mdto reflect the new import convention.Why
Under
moduleResolution: NodeNext, every relative TS import has to be written with a.jsextension (import x from './foo.js') — a long-standing footgun for new contributors and a frequent source of confusion when scripts/codemods generate imports.moduleResolution: bundlertreats the path as a module reference, matching whattsdown,vitest, and downstream consumers' bundlers already do at runtime.This is an internal-only build configuration change. Consumers are not affected: they continue to import from the built
.mjs/.d.mtsfiles declared in each package'sexportsmap, which still carry the.jsextensions Node's NodeNext resolver requires at runtime.Test plan
pnpm typecheck:allpassespnpm lint:allpasses (includingsync:snippets --check)pnpm test:allpassespnpm build:allpasses