-
Notifications
You must be signed in to change notification settings - Fork 11
feat: migrate 17 leaf modules from JavaScript to TypeScript #553
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
Changes from 2 commits
81b7a2f
f5f83cb
4586d2b
04e64ef
4425dbd
d6366de
21de04f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,46 @@ | ||||||||||||||||||||
| #!/usr/bin/env node | ||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Test runner wrapper that configures Node.js for the TypeScript migration | ||||||||||||||||||||
| * before spawning vitest. Adds --experimental-strip-types on Node >= 22.6 | ||||||||||||||||||||
| * so child processes can execute .ts files natively. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import { spawnSync } from 'node:child_process'; | ||||||||||||||||||||
| import { fileURLToPath, pathToFileURL } from 'node:url'; | ||||||||||||||||||||
| import { dirname, resolve } from 'node:path'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||||||||||||||||||||
| const hook = pathToFileURL(resolve(__dirname, 'ts-resolver-hook.js')).href; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const args = process.argv.slice(2); | ||||||||||||||||||||
| const vitestBin = resolve(__dirname, '..', 'node_modules', 'vitest', 'vitest.mjs'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const [major, minor] = process.versions.node.split('.').map(Number); | ||||||||||||||||||||
| const supportsStripTypes = major > 22 || (major === 22 && minor >= 6); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Build NODE_OPTIONS: resolver hook + type stripping (Node >= 22.6) | ||||||||||||||||||||
| const hookImport = `--import ${hook}`; | ||||||||||||||||||||
| const existing = process.env.NODE_OPTIONS || ''; | ||||||||||||||||||||
| const parts = [ | ||||||||||||||||||||
| existing.includes(hookImport) ? null : hookImport, | ||||||||||||||||||||
| supportsStripTypes && !existing.includes('--experimental-strip-types') | ||||||||||||||||||||
| ? '--experimental-strip-types' | ||||||||||||||||||||
| : null, | ||||||||||||||||||||
|
Comment on lines
+26
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The dedup guard uses A more robust check would test for both exact flag tokens:
Suggested change
Using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — the dedup check uses |
||||||||||||||||||||
| existing || null, | ||||||||||||||||||||
| ].filter(Boolean); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Spawn vitest via node directly — avoids shell: true and works cross-platform | ||||||||||||||||||||
| const result = spawnSync(process.execPath, [vitestBin, ...args], { | ||||||||||||||||||||
| stdio: 'inherit', | ||||||||||||||||||||
| env: { | ||||||||||||||||||||
| ...process.env, | ||||||||||||||||||||
| NODE_OPTIONS: parts.join(' '), | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (result.error) { | ||||||||||||||||||||
| process.stderr.write(`[test runner] Failed to spawn vitest: ${result.error.message}\n`); | ||||||||||||||||||||
| process.exit(1); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| process.exit(result.status ?? 1); | ||||||||||||||||||||
|
Comment on lines
+39
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Add an explicit error check:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — added explicit |
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| /** | ||
| * Node.js module resolution hook for incremental TypeScript migration. | ||
| * | ||
| * Registered via --import. Uses the module.register() API (Node >= 20.6) | ||
| * to install a resolve hook that falls back to .ts when .js is missing. | ||
| */ | ||
|
|
||
| import { register } from 'node:module'; | ||
|
|
||
| register('./ts-resolver-loader.js', import.meta.url); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| /** | ||
| * ESM loader: resolve .js → .ts fallback for incremental migration. | ||
| * | ||
| * - resolve hook: when a .js specifier is not found, retry with .ts | ||
| * - load hook: strip type annotations from .ts files using Node's built-in | ||
| * amaro (Node >= 22.6) so the loader works outside of Vitest/Vite too | ||
| */ | ||
|
|
||
| import { readFile } from 'node:fs/promises'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| export async function resolve(specifier, context, nextResolve) { | ||
| try { | ||
| return await nextResolve(specifier, context); | ||
| } catch (err) { | ||
| if (err.code !== 'ERR_MODULE_NOT_FOUND' || !specifier.endsWith('.js')) throw err; | ||
|
|
||
| const tsSpecifier = specifier.replace(/\.js$/, '.ts'); | ||
| try { | ||
| return await nextResolve(tsSpecifier, context); | ||
| } catch { | ||
| throw err; | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+12
to
+25
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The loader only implements the This matters today because Either document explicitly that this hook is Vitest-only and must never be used standalone, or add a import { readFile } from 'node:fs/promises';
import { fileURLToPath } from 'node:url';
export async function load(url, context, nextLoad) {
if (url.endsWith('.ts')) {
// Delegate to Vite/Vitest transform; fall back to nextLoad for non-TS
// (Vitest overrides this hook in its own worker setup)
}
return nextLoad(url, context);
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed — added a |
||
|
|
||
| export async function load(url, context, nextLoad) { | ||
| if (!url.endsWith('.ts')) return nextLoad(url, context); | ||
|
|
||
| // On Node >= 22.6 with --experimental-strip-types, Node handles .ts natively | ||
| try { | ||
| return await nextLoad(url, context); | ||
| } catch (err) { | ||
| if (err.code !== 'ERR_UNKNOWN_FILE_EXTENSION') throw err; | ||
| } | ||
|
|
||
| // Fallback: read the file and return as module source | ||
| // This path is reached on Node < 22.6 where --experimental-strip-types | ||
| // is unavailable. TypeScript-only syntax will cause a parse error — callers | ||
| // should ensure .ts files contain only erasable type annotations. | ||
| const source = await readFile(fileURLToPath(url), 'utf-8'); | ||
| return { format: 'module', source, shortCircuit: true }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The fallback path at line 41-42 reads the In practice, within Vitest's forked worker processes Vite's transform loader is registered after our hook (via The existing comment documents the limitation well — consider making the fallback an explicit error instead to surface the problem clearly if this path is ever hit unexpectedly: // Instead of silently returning unparseable source:
throw new Error(
`[ts-resolver-loader] Cannot load ${url}: Node < 22.6 lacks --experimental-strip-types and no other loader transformed this .ts file.`
);
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 4586d2b — load hook now throws ERR_TS_UNSUPPORTED on Node < 22.6 instead of silently returning unparseable source. Also added a jsToTsResolver Vite plugin to vitest.config.js so in-process Vite resolution works regardless of Node version. Child-process tests skip on Node < 22.6. |
||
| } | ||
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.
--experimental-strip-typesis deprecated on Node >= 23--experimental-strip-typeswas renamed to--strip-typesin Node 23.0.0. Using the old flag on Node 23+ still works (it's kept as an alias for backward compatibility) but emits aDEP0XXXdeprecation warning to stderr on every test run, cluttering output and potentially failing CI pipelines that treat warnings as errors.The fix is to select the correct flag based on Node version:
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.
Fixed in 4586d2b — scripts/test.js and vitest.config.js now use --strip-types on Node >= 23 and --experimental-strip-types on Node 22.6-22.x, avoiding the deprecation warning.