feat(types): migrate test suite and config files to TypeScript (Phase 6)#588
feat(types): migrate test suite and config files to TypeScript (Phase 6)#588carlos-alm wants to merge 8 commits intomainfrom
Conversation
Rename all 114 test files and 2 helpers from .js to .ts, plus vitest.config and commitlint.config. Add type annotations to fix noImplicitAnyLet lint errors. Remove jsToTsResolver vite plugin (no longer needed). Drop allowJs/checkJs from tsconfig.
|
Claude finished @carlos-alm's task —— View job PR Review: TypeScript Migration Phase 6 ✅Review Todo List:
Phase 1: Root Cause Analysis ✅Problem Being Solved: Final phase of TypeScript migration - converting test suite (.js → .ts) to complete the codebase migration and enable full TypeScript strict mode checking. Why This Approach: Logical completion of the migration phases. The approach is sound:
Risk Assessment: Low risk - this is cleanup/completion work, not new functionality. Phase 2: Configuration Change Detection ✅Changes Reviewed:
Assessment: All configuration changes are justified cleanup after migration completion. No rules were relaxed or weakened. Phase 3: Test Integrity Verification ✅File Renames: All 114 test files + 2 helpers renamed from Type Annotations Added: Systematic addition of explicit types to
Assessment: Test logic is preserved. Type annotations are conservative (using Phase 4: Code Quality Assessment ✅Approach Quality: Conservative and methodical - only adds types where required by the linter, uses Consistency: All changes follow the same pattern - minimal annotations to satisfy TypeScript without over-engineering test types. Maintainability: Removes temporary scaffolding that was supporting the migration, leaving cleaner configuration. Phase 5: Backlog Compliance Evaluation ✅Root Cause AnalysisProblem Being Solved: Completing TypeScript migration by converting the test suite to enable full strict mode checking and removing temporary migration scaffolding. Backlog Compliance
Critical ConcernsNone identified. This is excellent, methodical completion work. Final Recommendation
|
Rename 16 scripts and 1 docs example from .js to .ts. Update all invocations in package.json, CI workflows, and shell scripts to use node --experimental-strip-types. Remaining .js files (must stay JS): - scripts/test.js, scripts/build-wasm.js (Node 20 bootstrap) - 4 ESM loader hooks (bootstrap .ts support) - scripts/gen-deps.cjs, src/index.cjs (CJS by design)
- Bump engines.node from >=20 to >=22
- Remove Node 20 from CI test matrix
- Bump native build Node from 20 to 22
- Replace old JS ESM loader hooks with 2 TypeScript hooks
(ts-resolve-loader.ts + ts-resolve-hooks.ts) that resolve .js→.ts
import specifiers at runtime
- Remove canStripTypes helper and version guards from tests
- Update src/index.cjs to import('./index.ts') for dev; build step
produces dist/index.cjs with ./index.js for published package
- Simplify vitest.config.ts to use --experimental-strip-types only
- Update child-process test invocations to use --import loader flag
- Rewrite scripts/gen-deps.cjs as ESM TypeScript
- Rename scripts/build-wasm.js → .ts
Impact: 1 functions changed, 1 affected
…ile (#588) The test file was renamed from .js to .ts in the Phase 6 migration but the CI workflow was not updated, causing test discovery to fail.
# Conflicts: # vitest.config.js
Greptile SummaryPhase 6 of the JS→TS migration renames 114 test files, 16 scripts, and both config files to
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Node invocation\n(node --experimental-strip-types)"] --> B{Node version}
B -->|">=23"| C["--strip-types\n.js→.ts auto-resolved"]
B -->|"22.6–22.x"| D["--experimental-strip-types\n.js→.ts NOT auto-resolved"]
D --> E{Import loader present?}
E -->|"--import ts-resolve-loader.js"| F["✅ .js specifiers resolved to .ts\nBenchmark scripts work"]
E -->|"missing (current benchmark.yml)"| G["❌ ERR_MODULE_NOT_FOUND\nbench-config.js / fork-engine.js"]
C --> F
H["pre-commit-checks.ts\nrequire() calls"] --> I{Module type}
I -->|"type: module (this repo)"| J["❌ ReferenceError: require is not defined\nHook silently exits 0"]
I -->|"type: commonjs"| K["✅ Works as before"]
F --> L["vitest.config.ts\nChild processes inherit NODE_OPTIONS"]
L --> M["✅ Test suite passes\n(src/ still .js, no .js→.ts needed)"]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "engines": { | ||
| "node": ">=20" | ||
| "node": ">=22" | ||
| }, |
There was a problem hiding this comment.
engines constraint too broad for --experimental-strip-types
--experimental-strip-types was only introduced in Node 22.6.0. Declaring >=22 here means users on Node 22.0–22.5 will hit errors when running any script that uses node --experimental-strip-types (build:wasm, deps:tree, verify-imports, version, etc.) — even though the engines field technically allows those versions.
The same issue affects vitest.config.ts, which injects --experimental-strip-types into NODE_OPTIONS unconditionally for any Node 22.x (see below).
| "engines": { | |
| "node": ">=20" | |
| "node": ">=22" | |
| }, | |
| "engines": { | |
| "node": ">=22.6" | |
| }, |
There was a problem hiding this comment.
Fixed — tightened engines constraint to >=22.6 since --experimental-strip-types requires Node 22.6+.
vitest.config.ts
Outdated
| const [major] = process.versions.node.split('.').map(Number); | ||
| const existing = process.env.NODE_OPTIONS || ''; | ||
| const stripFlag = major >= 23 ? '--strip-types' : '--experimental-strip-types'; |
There was a problem hiding this comment.
--experimental-strip-types
The old vitest.config.js explicitly gated on major > 22 || (major === 22 && minor >= 6) before injecting the flag into NODE_OPTIONS. The new config only destructures major and unconditionally assigns --experimental-strip-types for any Node 22.x. Child processes spawned by tests (e.g. watcher tests, CLI integration tests) inherit NODE_OPTIONS; on Node 22.0–22.5 that invalid flag will crash them immediately.
| const [major] = process.versions.node.split('.').map(Number); | |
| const existing = process.env.NODE_OPTIONS || ''; | |
| const stripFlag = major >= 23 ? '--strip-types' : '--experimental-strip-types'; | |
| const [major, minor] = process.versions.node.split('.').map(Number); | |
| const existing = process.env.NODE_OPTIONS || ''; | |
| const supportsStripTypes = major > 22 || (major === 22 && minor >= 6); | |
| const stripFlag = major >= 23 ? '--strip-types' : '--experimental-strip-types'; |
And in the env block, guard the injection:
!supportsStripTypes || (existing.includes('--experimental-strip-types') || existing.includes('--strip-types'))
? ''
: stripFlag,There was a problem hiding this comment.
Fixed — restored the minor-version guard (supportsStripTypes = major > 22 || (major === 22 && minor >= 6)) and gated the strip-types injection on it, matching the old config's behavior.
package.json
Outdated
| "scripts": { | ||
| "build": "tsc", | ||
| "build:wasm": "node scripts/build-wasm.js", | ||
| "build": "tsc && node -e \"require('fs').writeFileSync('dist/index.cjs',require('fs').readFileSync('src/index.cjs','utf8').replace('./index.ts','./index.js'))\"", |
There was a problem hiding this comment.
String.replace() only replaces first occurrence
String.prototype.replace() with a string pattern replaces only the first match. If src/index.cjs ever gains a second ./index.ts reference, later occurrences will be silently left behind in dist/index.cjs, causing a runtime failure for CJS consumers. Prefer replaceAll:
| "build": "tsc && node -e \"require('fs').writeFileSync('dist/index.cjs',require('fs').readFileSync('src/index.cjs','utf8').replace('./index.ts','./index.js'))\"", | |
| "build": "tsc && node -e \"require('fs').writeFileSync('dist/index.cjs',require('fs').readFileSync('src/index.cjs','utf8').replaceAll('./index.ts','./index.js'))\"", |
There was a problem hiding this comment.
Fixed — switched to replaceAll for the ./index.ts to ./index.js replacement in the build script.
| specifier: string, | ||
| context: { parentURL?: string; conditions: string[] }, | ||
| nextResolve: Function, | ||
| ): Promise<{ url: string; shortCircuit?: boolean }> { |
There was a problem hiding this comment.
Function type — use a specific callback signature
Function is explicitly banned by most TypeScript style guides (equivalent to any for callables). The Node ESM nextResolve hook has a known shape:
| specifier: string, | |
| context: { parentURL?: string; conditions: string[] }, | |
| nextResolve: Function, | |
| ): Promise<{ url: string; shortCircuit?: boolean }> { | |
| export async function resolve( | |
| specifier: string, | |
| context: { parentURL?: string; conditions: string[] }, | |
| nextResolve: (specifier: string, context?: { parentURL?: string; conditions: string[] }) => Promise<{ url: string; shortCircuit?: boolean }>, | |
| ): Promise<{ url: string; shortCircuit?: boolean }> { |
There was a problem hiding this comment.
Fixed — replaced bare Function type with a specific callback signature: (specifier: string, context?: { parentURL?: string; conditions: string[] }) => Promise<{ url: string; shortCircuit?: boolean }>.
…eplaceAll (#588) Impact: 1 functions changed, 0 affected
| # Run all checks in a single Node.js process | ||
| HOOK_DIR="$(cd "$(dirname "$0")" && pwd)" | ||
| RESULT=$(node "$HOOK_DIR/pre-commit-checks.js" "$WORK_ROOT" "$EDITED_FILES" "$STAGED" 2>/dev/null) || true | ||
| RESULT=$(node --experimental-strip-types "$HOOK_DIR/pre-commit-checks.ts" "$WORK_ROOT" "$EDITED_FILES" "$STAGED" 2>/dev/null) || true |
There was a problem hiding this comment.
--experimental-strip-types is deprecated on Node 23+
This shell script hardcodes --experimental-strip-types, but vitest.config.ts correctly selects between --experimental-strip-types (Node 22.x) and --strip-types (Node >=23) at runtime. Users running Node 23+ who follow this documentation example will get deprecation warnings, even though they're suppressed here via 2>/dev/null.
Since the package now requires Node >=22.6, consider a version-aware invocation:
| RESULT=$(node --experimental-strip-types "$HOOK_DIR/pre-commit-checks.ts" "$WORK_ROOT" "$EDITED_FILES" "$STAGED" 2>/dev/null) || true | |
| STRIP_FLAG=$(node -e "const [M,m]=process.versions.node.split('.').map(Number); console.log(M>=23?'--strip-types':'--experimental-strip-types')") | |
| RESULT=$(node $STRIP_FLAG "$HOOK_DIR/pre-commit-checks.ts" "$WORK_ROOT" "$EDITED_FILES" "$STAGED" 2>/dev/null) || true |
There was a problem hiding this comment.
Fixed — the pre-commit.sh example now dynamically selects between --strip-types (Node >=23) and --experimental-strip-types (Node 22.x) at runtime, matching the approach already used in vitest.config.ts.
…588) Detect Node version at runtime and select --strip-types (>=23) vs --experimental-strip-types (22.x), matching the approach in vitest.config.ts.
Replace JSDoc-only annotations in fixtures.ts with proper TypeScript interfaces and typed parameters. Add interfaces and typed parameters to resolution-benchmark.test.ts helper functions. Eliminates all noExplicitAny warnings in both files. Impact: 22 functions changed, 0 affected
|
Addressed the remaining review feedback from Greptile's second review:
All tests pass (546/546), TypeScript type check passes, and lint is clean. |
| ARGS="$ARGS --npm" | ||
| fi | ||
| node scripts/benchmark.js $ARGS 2>/dev/null > benchmark-result.json | ||
| node --experimental-strip-types scripts/benchmark.ts $ARGS 2>/dev/null > benchmark-result.json |
There was a problem hiding this comment.
--import loader — benchmark scripts will fail on Node 22
scripts/benchmark.ts (and embedding-benchmark.ts, query-benchmark.ts, incremental-benchmark.ts) contain cross-script imports using .js specifiers:
import { resolveBenchmarkSource, srcImport } from './lib/bench-config.js';
import { isWorker, workerEngine, workerTargets, forkEngines } from './lib/fork-engine.js';Both scripts/lib/bench-config.js and scripts/lib/fork-engine.js do not exist on disk — only their .ts counterparts do. Node 22's --experimental-strip-types strips type annotations but does not rewrite .js specifiers to .ts (that resolution was only added in Node 23+). Without the --import ESM loader hook, these scripts will fail immediately with ERR_MODULE_NOT_FOUND.
The fix is to add --import ./scripts/ts-resolve-loader.js (the .js version is still present and fully functional) before the script path. The same fix applies at lines 247, 384, and 521:
| node --experimental-strip-types scripts/benchmark.ts $ARGS 2>/dev/null > benchmark-result.json | |
| node --experimental-strip-types --import ./scripts/ts-resolve-loader.js scripts/benchmark.ts $ARGS 2>/dev/null > benchmark-result.json |
| ARGS="$ARGS --npm" | ||
| fi | ||
| node scripts/embedding-benchmark.js $ARGS 2>/dev/null > embedding-benchmark-result.json | ||
| node --experimental-strip-types scripts/embedding-benchmark.ts $ARGS 2>/dev/null > embedding-benchmark-result.json |
There was a problem hiding this comment.
--import loader — same resolution failure as line 96
scripts/embedding-benchmark.ts also imports ./lib/bench-config.js and ./lib/fork-engine.js (which exist only as .ts files). Same fix applies:
| node --experimental-strip-types scripts/embedding-benchmark.ts $ARGS 2>/dev/null > embedding-benchmark-result.json | |
| node --experimental-strip-types --import ./scripts/ts-resolve-loader.js scripts/embedding-benchmark.ts $ARGS 2>/dev/null > embedding-benchmark-result.json |
| ARGS="$ARGS --npm" | ||
| fi | ||
| node scripts/query-benchmark.js $ARGS 2>/dev/null > query-benchmark-result.json | ||
| node --experimental-strip-types scripts/query-benchmark.ts $ARGS 2>/dev/null > query-benchmark-result.json |
There was a problem hiding this comment.
--import loader — same resolution failure as line 96
scripts/query-benchmark.ts also imports ./lib/bench-config.js and ./lib/fork-engine.js (which exist only as .ts files).
| node --experimental-strip-types scripts/query-benchmark.ts $ARGS 2>/dev/null > query-benchmark-result.json | |
| node --experimental-strip-types --import ./scripts/ts-resolve-loader.js scripts/query-benchmark.ts $ARGS 2>/dev/null > query-benchmark-result.json |
| ARGS="$ARGS --npm" | ||
| fi | ||
| node scripts/incremental-benchmark.js $ARGS 2>/dev/null > incremental-benchmark-result.json | ||
| node --experimental-strip-types scripts/incremental-benchmark.ts $ARGS 2>/dev/null > incremental-benchmark-result.json |
There was a problem hiding this comment.
--import loader — same resolution failure as line 96
scripts/incremental-benchmark.ts also imports ./lib/bench-config.js and ./lib/fork-engine.js (which exist only as .ts files).
| node --experimental-strip-types scripts/incremental-benchmark.ts $ARGS 2>/dev/null > incremental-benchmark-result.json | |
| node --experimental-strip-types --import ./scripts/ts-resolve-loader.js scripts/incremental-benchmark.ts $ARGS 2>/dev/null > incremental-benchmark-result.json |
1 participant
Summary
.jsto.tsvitest.config.jsandcommitlint.config.jsto.ts.jsto.tsnode scripts/X.jsinvocations in package.json, CI workflows, and shell scripts tonode --experimental-strip-types scripts/X.tsletdeclarations to fixnoImplicitAnyLetlint errors (0 lint errors)jsToTsResolvervite plugin from vitest config (no longer needed)allowJs/checkJsmigration flags fromtsconfig.jsonRemaining JS files (must stay JS for Node 20 / bootstrap reasons):
scripts/test.js+scripts/build-wasm.js— Node 20 bootstrap (no strip-types)ts-resolve-*.js,ts-resolver-*.js) — bootstrap.tssupportscripts/gen-deps.cjs+src/index.cjs— CommonJS by designTest plan
tsc --noEmitcleanbiome check— 0 errorsnode --experimental-strip-types scripts/verify-imports.tspasses