codemod iterations 5#2398
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // both reasons target the same import declaration and the diagnostic dedups per declaration. The | ||
| // sync usage is the more fundamental blocker (it cannot await even after migrating the type usage), | ||
| // so it is recorded first and wins. | ||
| for (const local of localBindings(imp)) { | ||
| for (const ref of findValueRefs(sourceFile, local)) { | ||
| if (!isAwaitSafe(ref)) { | ||
| blockers.push({ symbol: local, line: ref.getStartLineNumber(), decl: imp.decl }); | ||
| } | ||
| } | ||
| } | ||
| // A named binding referenced as both a value and a type cannot be converted: removing its static | ||
| // import to load the value dynamically would orphan the surviving `: name` / `as name` type usage | ||
| // (TS2304). Diagnose rather than convert (the conservative, type-safe choice — we do not auto-split | ||
| // the value and type usages into a retained `import type` in this pass). | ||
| for (const n of imp.named) { | ||
| const local = n.getAliasNode()?.getText() ?? n.getName(); | ||
| if (hasTypeReference(sourceFile, local)) { | ||
| blockers.push({ symbol: local, line: imp.decl.getStartLineNumber(), decl: imp.decl, isValueAndType: true }); | ||
| } | ||
| } | ||
| } | ||
| return { convertible: blockers.length === 0, blockers }; | ||
| } | ||
|
|
||
| const HELPER_ID = '_mcpDynImport'; | ||
| const HELPER_DECL = | ||
| '// eslint-disable-next-line no-new-func, @typescript-eslint/no-implied-eval\n' + |
There was a problem hiding this comment.
🔴 The value-and-type guard in analyzeConvertibility() only checks named import specifiers (imp.named), so a namespace import (import * as mcp from "@modelcontextprotocol/core") that is used as a value inside an async function and as a type qualifier elsewhere (e.g. let x: mcp.SomeType) is still deemed convertible — convertFile() then removes the whole namespace import and only injects const mcp = await … inside the async body, orphaning the type reference (TS2304). Run the same hasTypeReference() check on imp.namespace (and imp.defaultImport) so this case is diagnosed like the equivalent named-import case.
Extended reasoning...
The bug. In analyzeConvertibility() (packages/codemod/src/migrations/v1-to-v2/transforms/commonjsInterop.ts, ~lines 196–222), the "used as both a value and a type" guard is implemented as a loop over only the named specifiers:
for (const n of imp.named) {
const local = n.getAliasNode()?.getText() ?? n.getName();
if (hasTypeReference(sourceFile, local)) {
blockers.push({ symbol: local, ..., isValueAndType: true });
}
}imp.namespace is never passed through hasTypeReference(). So a namespace binding that is referenced both as a value and as a type-position qualifier slips past every blocker check.
The code path that triggers it. Consider a CommonJS file:
import * as mcp from "@modelcontextprotocol/core";
let cached: mcp.CallToolResult | undefined; // module-level TYPE usage
export async function call(body: unknown) {
return mcp.CallToolResultSchema.parse(body); // VALUE usage inside async fn
}Step-by-step:
collectV2ValueImports()collects the namespace binding because the value usage insidecall()givesfindValueRefs('mcp')≥ 1 reference.- The sync-context loop over
localBindings(imp)does not flag the module-levelmcp.CallToolResultannotation: for that identifier the parent is aQualifiedName, andisTypePositionReference()walks up the qualified-name chain to theTypeReferenceand returns true, sofindValueRefs()excludes it. No sync-context blocker is recorded (correctly — it isn't a value usage). - The value-and-type loop only iterates
imp.named, which is empty here, so noisValueAndTypeblocker is recorded either. - Result:
convertible === true, andconvertFile()runs. Its namespace branch callsimp.decl.remove(), deleting the entireimport * as mcpdeclaration, and injectsconst mcp = await _mcpCore;only inside the async function body. - The module-level
mcp.CallToolResultannotation now references a binding that no longer exists → TS2304/TS2503. The generatedimport type * as _McpCoreTypes from "@modelcontextprotocol/core"uses a different identifier, so it does not rescue the orphaned reference.
Why nothing else prevents it. The exact same scenario with a named binding is correctly diagnosed via the isValueAndType blocker (and covered by the test "diagnoses (does not convert) a symbol used as both a value and a type"), so this is an unintended gap in the namespace path rather than a design choice. The PR's own contract is all-or-nothing: "diagnose rather than emit broken code" — this path emits code that fails typecheck instead. The trigger is realistic: the import * as x idiom (which the PR itself cites as pervasive in firebase-tools) combined with x.SomeType qualified type annotations is common in CommonJS projects, the very target of this transform.
Impact. A convertible-looking file produces type-broken output (new typecheck errors after the codemod), undermining the PR's headline guarantee of "0 new typecheck errors" for files it chooses to convert. The post-codemod typecheck would surface the breakage, but the user gets broken code rather than the actionable diagnostic the transform is designed to emit.
Fix. In analyzeConvertibility(), run the same hasTypeReference() check for the namespace binding (and imp.defaultImport, though defaults are already always diagnosed), e.g. push an isValueAndType blocker when imp.namespace && hasTypeReference(sourceFile, imp.namespace.getText()). Alternatively, convertFile() could rewrite namespace type qualifiers to the generated _…Types namespace, but the conservative diagnose-first fix matches the existing named-import behavior and the PR's stated design.
felixweinberger
left a comment
felixweinberger
left a comment
There was a problem hiding this comment.
Putting back in your queue for fixing the build issue + bot finding
2 participants
Teach the v1→v2 codemod to migrate CommonJS projects onto the ESM-only v2 packages: detect the target's module system, auto-convert v2 value imports to runtime dynamic
import()where it is safe, and emit actionable diagnostics where it is not — all without changing the target's tsconfig.Motivation and Context
The v2 SDK packages are ESM-only (
"type": "module",exportsexpose only animportcondition, the bundles use top-levelawait). A CommonJS project — nopackage.json"type", tsconfigmodule: commonjs— that runs the codemod gets a migration that typechecks and builds but fails at load: the emitted CJSrequire()s an ESM-only package and throwsERR_PACKAGE_PATH_NOT_EXPORTED/ERR_MODULE_NOT_FOUND. The codemod previously had no awareness of the target's module system and silently produced this runtime-broken output.This was surfaced by the batch test against firebase-tools (a large CommonJS CLI): the migration was typecheck-green, but its MCP server (
src/mcp/onemcp/onemcp_server.ts) died at load withERR_MODULE_NOT_FOUND.Two facts shaped the design:
module: commonjs→nodenextproduces 1144 new type errors across its own code (its pervasiveimport * as x from 'y'; x()CJS-interop idiom), unrelated to MCP. So the codemod must keep the project CommonJS.module: commonjs, TypeScript downlevels a literalimport()back intorequire(), so even hand-written dynamic imports re-break. A real runtimeimport()only survives if it's hidden from the compiler.What this PR adds:
analyzeProjectnow resolves amoduleSystem('esm' | 'commonjs' | 'unknown') frompackage.json"type"+ tsconfigmodule(viats.parseConfigFileTextToJson, never throws).commonjsInteroptransform that runs last (after every specifier-rewriting transform, so it observes the final v2 import shape). For a CommonJS target, for each file where every v2 value usage is inside anasyncblock, it rewrites the static v2 value imports to runtime dynamicimport()via a compiler-proofnew Function("s","return import(s)")indirection, with aimport type * as _PkgTypes from "<pkg>"declaration supplying types (kept type-only so it's erased at runtime, and recognized by the dependency-detection so the package stays inpackage.json). The destructured value bindings load inside each async function; call sites are unchanged.How Has This Been Tested?
package.json/tsconfig precedence rules) and the transform: value conversion shape, the namespace/alias/idempotency edge cases, and every diagnose path (sync constructor, default import, value re-export, async parameter default, value-vs-type, both-value-and-type). Full codemod suite green.package.json).--codemod local --sdk published), reproduced across multiple fresh clones:tc=0 test=0 lint=0→ post-codemodtc=0 test=0, 0 new typecheck errors.src/mcp/onemcp/onemcp_server.tsconverts to dynamicimport()and its spec goes fromERR_MODULE_NOT_FOUNDto 16 passing;@modelcontextprotocol/coreis correctly added topackage.jsonand installed.index.ts's sync constructor +SSEServerTransportused as both value and type;logging-transport.ts's syncStdioServerTransport) are correctly diagnosed for manual ESM work rather than mis-converted.lintflags onlyprettier/prettierformatting of the generated code (auto-fixable;eslint --fix→ 0 errors) — the usual "run your formatter after migration" cosmetic, not a functional issue.Breaking Changes
None to any public API. The change is additive to the codemod. For ESM targets and type-only imports it is a no-op (existing migration behavior is unchanged). For CommonJS targets it changes the codemod's output for the better — previously it emitted runtime-broken static imports; it now converts where safe and diagnoses where not.
Types of changes
Checklist
Additional context
new Functionindirection: undermodule: commonjsTS downlevels a literalimport()torequire(), which re-breaks ESM-only v2. Hiding the import behindnew Function("s","return import(s)")keeps a real nativeimport()at runtime. It's contained to one helper per converted file with aneslint-disable. Trade-off: opaque to bundlers — fine for Node apps shippingnode_modules(e.g. firebase-tools); noted as a caveat for bundled consumers.nodenextflip's 1144-error blast radius (above) makes it unviable as an automatic codemod step.lintdelta is entirely auto-fixable formatting.