Move TypeScript to node16 module resolution - #355
Conversation
1e489f5 to
d87774e
Compare
1886da7 to
d16732d
Compare
commonjs implies node10 resolution, which cannot read package.json
exports — the algorithm Node, esbuild, and vitest all use. That
mismatch already broke tsc-only on the vscode-languageclient v10
upgrade (exports-only package), patched over with a paths shim that's
now removed since node16 resolves it natively. Also fixes a latent
mcp-server issue where tsc type-checked the SDK's ESM declarations
while esbuild bundled its CJS implementation, agreeing only by luck.
Fallout: extensionless relative specifiers in vi.mock('vscode', ...)
factories now need .js. And node16 has tsc start compiling the
webview scripts (plain .js, allowJs) into CJS modules with an
`exports` stamp; readWebviewScript's __dirname-adjacent lookup would
have picked up that compiled copy under F5 and thrown in the webview
DOM at runtime, so it now reads from the sibling src/ tree only.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
esbuild and vitest already transpile file-by-file; this makes tsc enforce the assumption they both make. Zero errors across all three workspaces, kept as its own commit so a bisect can tell it apart from the node16 migration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
d16732d to
2fd713e
Compare
ericwinger
left a comment
ericwinger
left a comment
There was a problem hiding this comment.
Lots of files but claude didn't find anything super scary.
Probably should at least consider doing the F5 in a follow-up.
Some of my other PR's will need .js on their vscode mocks also.
Hope the documentation that an agent might read is up to date.
| if (fs.existsSync(beside)) { | ||
| return fs.readFileSync(beside, 'utf8'); | ||
| } | ||
| return fs.readFileSync(path.join(__dirname, '..', 'src', subdir ?? '', fileName), 'utf8'); |
There was a problem hiding this comment.
The real runtime fix — correct, but the one thing automation can't reach. Reading only ../src/… is right: the __dirname-adjacent copy in out/ is the allowJs compile output, which node16 stamps as CJS (exports) and throws in the webview DOM. Packaging is covered (.vscodeignore whitelist L63–73 + the scan test), and the unit test pins this path.
But the tests mock fs and only assert the resolved path — they can't exercise the actual webview-DOM load that regressed. Since that failure (ReferenceError: exports is not defined) is exactly what this PR fixes and nothing automated covers it, worth doing the F5 check the PR lists (open a Debugger view / refactoring panel, confirm a clean console) before merge.
| } | ||
| "allowJs": true | ||
| }, | ||
| "include": ["src/**/*.ts", "src/**/*.js"] |
There was a problem hiding this comment.
LOW / optional: with allowJs + "src/**/*.js" in include, tsc still emits these webview scripts into out/<subdir>/ as the CJS-stamped copies this PR now deliberately avoids reading — they're still generated, just dead. If you'd rather remove the footgun at the source than route around it, dropping src/**/*.js from include (or excluding the *View.js scripts) would stop tsc producing the broken copies at all. Not blocking.
2 participants
Summary
tsconfig.base.jsonnow sets"module": "node16"instead ofcommonjs, sotsc's resolution algorithm can readpackage.jsonexportsmaps the way Node/esbuild/vitest already do.pathsshim inclient/tsconfig.jsonthat worked aroundvscode-languageclientv10's exports-only layout, and the node10 pin inacceptance/tsconfig.json.readWebviewScript(client/src/webviewAssets.ts) used to check the__dirname-adjacent path first, which undernode16could pick up a stale,exports-stamped compiled copy of a webview script fromclient/outduring F5 debugging and throw in the webview DOM. It now only ever reads from the siblingsrc/tree..jsto the 104vi.mock('vscode', () => import(...))specifiers that needed it, and drops a redundant dynamicfsimport inextension.ts.isolatedModulesas a separate, easily-bisectable follow-up (zero new errors).Test plan
npm run compile— all four projects (client, client-bin, server, mcp-server), 0 errorsnpm run bundle— esbuild output rewritten cleanlynpm test— all three workspaces green (4287+322+92 tests passing)npm run lint && npm run format:check— cleanReferenceError: exports is not definedin the Dev Tools console — this repo's own CLAUDE.md flags F5 as a human-only verification step🤖 Generated with Claude Code