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 merged 16 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 SummaryThis is Phase 6 of the JS→TS migration, renaming 114 test files, 16 scripts, and 2 config files from Key findings:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Node.js process] --> B{Node version?}
B -->|">= 23"| C["--strip-types"]
B -->|"22.6 – 22.x"| D["--experimental-strip-types"]
subgraph "vitest test runner"
E[vitest.config.ts] -->|"injects strip flag into NODE_OPTIONS"| F[Child test processes]
F --> G{needs .js→.ts resolver?}
G -->|"batch / cli / registry tests"| H["--import ts-resolve-loader.ts\n(explicit in test code)"]
G -->|"all other tests"| I[No loader needed\nall imports already .ts]
end
subgraph "CI / npm scripts"
J["package.json scripts\n(--experimental-strip-types hardcoded)"] --> K[build:wasm / verify-imports\ngen-deps / sync-native-versions]
L["benchmark.yml\n(dynamic STRIP_FLAG)"] -->|"+ --import ts-resolve-loader.js"| M[benchmark scripts\nimport from lib/*.js]
N["ci.yml / publish.yml\n(dynamic STRIP_FLAG)"] --> O[verify-imports\nsync-native-versions]
end
subgraph "Bootstrap .js files (stay as .js)"
P[ts-resolve-loader.js]
Q[ts-resolve-hooks.js]
R[ts-resolver-*.js]
end
H --> P
P -->|"register()"| Q
|
![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. |
.github/workflows/benchmark.yml
Outdated
| 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 |
There was a problem hiding this comment.
Fixed — added --import ./scripts/ts-resolve-loader.js to the benchmark.ts invocation so .js specifiers resolve to their .ts counterparts on Node 22.
.github/workflows/benchmark.yml
Outdated
| 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 |
There was a problem hiding this comment.
Fixed — same --import ./scripts/ts-resolve-loader.js flag added to the embedding-benchmark.ts invocation.
.github/workflows/benchmark.yml
Outdated
| 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 |
There was a problem hiding this comment.
Fixed — same --import ./scripts/ts-resolve-loader.js flag added to the query-benchmark.ts invocation.
.github/workflows/benchmark.yml
Outdated
| 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 |
There was a problem hiding this comment.
Fixed — same --import ./scripts/ts-resolve-loader.js flag added to the incremental-benchmark.ts invocation.
…ports (#588) The file was renamed from .js to .ts but retained CommonJS require() calls. Since the repo has "type": "module", Node loads .ts files as ESM under --strip-types, where require is not defined. This caused a silent ReferenceError swallowed by 2>/dev/null || true in pre-commit.sh, making all pre-commit checks (cycles, dead exports, diff-impact) non-functional. Convert to ESM imports with createRequire for dynamic runtime requires. Also update the file header comment to reference .ts instead of .js.
…ibility (#588) Benchmark scripts import ./lib/bench-config.js and ./lib/fork-engine.js but only .ts files exist on disk. Node 22's --experimental-strip-types does not auto-resolve .js to .ts (that was added in Node 23+). Add --import ./scripts/ts-resolve-loader.js to all four benchmark invocations so the ESM loader resolves .js specifiers to their .ts counterparts.
|
Addressed the two remaining Greptile findings from the third review:
All 2098 tests pass, lint is clean. |
| /** | ||
| * Registers the .js → .ts ESM resolve hook. | ||
| * | ||
| * Usage: node --experimental-strip-types --import ./scripts/ts-resolve-loader.ts src/cli.ts | ||
| */ | ||
|
|
||
| import { register } from 'node:module'; | ||
|
|
||
| register(new URL('./ts-resolve-hooks.ts', import.meta.url)); |
There was a problem hiding this comment.
.ts loader duplicates existing .js loader but is unused
scripts/ts-resolve-loader.ts and scripts/ts-resolve-hooks.ts are newly added files, but neither is referenced anywhere in CI workflows, package.json scripts, or vitest.config.ts. Every callsite that needs the resolver (benchmark.yml lines 96, 247, 384, 521) still explicitly uses --import ./scripts/ts-resolve-loader.js.
The PR description notes that ESM loader hooks must stay .js for bootstrap reasons, which is consistent — but it's unclear what purpose these new .ts duplicates serve. If they're intended for documentation or future use, a comment explaining that would help. If they're not needed, they introduce confusion about which version to use and create an extra maintenance burden.
There was a problem hiding this comment.
Fixed — removed both scripts/ts-resolve-loader.ts and scripts/ts-resolve-hooks.ts. All callsites correctly use the .js versions (ESM loader hooks must stay .js for bootstrap reasons), so the .ts duplicates were dead code adding maintenance confusion.
There was a problem hiding this comment.
Correction on my earlier reply: I initially removed these files, but CI revealed they ARE used by the test suite. Three test files (batch.test.ts, cli.test.ts, registry.test.ts) reference scripts/ts-resolve-loader.ts via new URL('../../scripts/ts-resolve-loader.ts', import.meta.url) when spawning child node processes that need the loader.
The .ts versions serve a different purpose than the .js versions: the .js files are used by CI workflows and package.json scripts (where Node runs the loader directly), while the .ts files are used by the test suite's child process spawning (where the test resolver handles .ts imports).
Reverted the deletion. These files are correctly part of the PR.
.github/workflows/benchmark.yml
Outdated
| - name: Update build report | ||
| if: steps.existing.outputs.skip != 'true' | ||
| run: node scripts/update-benchmark-report.js benchmark-result.json | ||
| run: node --experimental-strip-types scripts/update-benchmark-report.ts benchmark-result.json |
There was a problem hiding this comment.
--experimental-strip-types in update-report invocations
The four update-*-report.ts invocations (benchmark, embedding, query, incremental) use node --experimental-strip-types without --strip-types on Node 23+. The benchmark.yml workflow isn't pinned to Node 22 — if it's run on a Node 23+ runner (or when the matrix is eventually updated), these will emit deprecation warnings about the obsolete flag.
The main benchmark runners (lines 96, 247, 384, 521) received the same fix in this PR and correctly keep --experimental-strip-types, but the equivalent vitest.config.ts dynamically selects between --experimental-strip-types and --strip-types based on the major version. Consider applying the same dynamic selection here, or at minimum noting this as a forward-compatibility concern.
There was a problem hiding this comment.
Fixed — all eight node invocations in benchmark.yml (four benchmark runners + four update-report scripts) now use a dynamic STRIP_FLAG variable that selects --strip-types on Node 23+ and --experimental-strip-types on Node 22.x, matching the approach already used in vitest.config.ts and pre-commit.sh.
- Use dynamic STRIP_FLAG selection in all benchmark.yml node invocations for forward compatibility with Node 23+ (--strip-types vs --experimental-strip-types) - Update token-benchmark.ts usage docs to include --import loader flag - Update CLAUDE.md Node version from >= 20 to >= 22.6
ESM loader hooks must stay as .js files for bootstrap reasons. The .ts duplicates were never referenced by any CI workflow or package script and only created confusion about which version to use.
|
Addressed the remaining findings from the latest Greptile review:
All tests pass, lint is clean. |
This reverts commit 966ec3e.
| "build:wasm": "node --experimental-strip-types scripts/build-wasm.ts", | ||
| "typecheck": "tsc --noEmit", | ||
| "verify-imports": "node scripts/verify-imports.js", | ||
| "test": "node scripts/test.js run", | ||
| "test:watch": "node scripts/test.js", | ||
| "test:coverage": "node scripts/test.js run --coverage", | ||
| "verify-imports": "node --experimental-strip-types scripts/verify-imports.ts", | ||
| "test": "vitest run", | ||
| "test:watch": "vitest", | ||
| "test:coverage": "vitest run --coverage", | ||
| "lint": "biome check src/ tests/", | ||
| "lint:fix": "biome check --write src/ tests/", | ||
| "format": "biome format --write src/ tests/", | ||
| "prepack": "npm run build", | ||
| "clean": "node -e \"require('fs').rmSync('dist',{recursive:true,force:true});require('fs').rmSync('.tsbuildinfo',{force:true})\"", | ||
| "prepare": "npm run build:wasm && npm run build && husky && npm run deps:tree", | ||
| "deps:tree": "node scripts/gen-deps.cjs", | ||
| "deps:tree": "node --experimental-strip-types scripts/gen-deps.ts", | ||
| "release": "commit-and-tag-version", | ||
| "release:dry-run": "commit-and-tag-version --dry-run", | ||
| "version": "node scripts/sync-native-versions.js && git add package.json crates/codegraph-core/Cargo.toml" | ||
| "version": "node --experimental-strip-types scripts/sync-native-versions.ts && git add package.json crates/codegraph-core/Cargo.toml" |
There was a problem hiding this comment.
--experimental-strip-types across all npm scripts
Every user-facing npm script that invokes a .ts file still hardcodes --experimental-strip-types, which is deprecated on Node 23+. The same pattern was flagged and fixed in benchmark.yml, pre-commit.sh, and vitest.config.ts, but package.json was left behind.
Affected scripts:
build:wasm(line 33) — runs viaprepare, i.e. on everynpm installverify-imports(line 35)deps:tree(line 45)version(line 48)
Users on Node 23+ will see a deprecation warning for every invocation. Because build:wasm is part of the prepare lifecycle, the warning appears on npm install, which is the noisiest possible place.
package.json scripts can't use shell conditionals inline, but a tiny wrapper (similar to the now-unused scripts/test.js, which already did version-aware flag selection) or an env-var trick works:
| "build:wasm": "node --experimental-strip-types scripts/build-wasm.ts", | |
| "typecheck": "tsc --noEmit", | |
| "verify-imports": "node scripts/verify-imports.js", | |
| "test": "node scripts/test.js run", | |
| "test:watch": "node scripts/test.js", | |
| "test:coverage": "node scripts/test.js run --coverage", | |
| "verify-imports": "node --experimental-strip-types scripts/verify-imports.ts", | |
| "test": "vitest run", | |
| "test:watch": "vitest", | |
| "test:coverage": "vitest run --coverage", | |
| "lint": "biome check src/ tests/", | |
| "lint:fix": "biome check --write src/ tests/", | |
| "format": "biome format --write src/ tests/", | |
| "prepack": "npm run build", | |
| "clean": "node -e \"require('fs').rmSync('dist',{recursive:true,force:true});require('fs').rmSync('.tsbuildinfo',{force:true})\"", | |
| "prepare": "npm run build:wasm && npm run build && husky && npm run deps:tree", | |
| "deps:tree": "node scripts/gen-deps.cjs", | |
| "deps:tree": "node --experimental-strip-types scripts/gen-deps.ts", | |
| "release": "commit-and-tag-version", | |
| "release:dry-run": "commit-and-tag-version --dry-run", | |
| "version": "node scripts/sync-native-versions.js && git add package.json crates/codegraph-core/Cargo.toml" | |
| "version": "node --experimental-strip-types scripts/sync-native-versions.ts && git add package.json crates/codegraph-core/Cargo.toml" | |
| "build:wasm": "node -e \"const v=process.versions.node.split('.').map(Number);const f=v[0]>=23?'--strip-types':'--experimental-strip-types';require('child_process').execFileSync(process.execPath,[f,'scripts/build-wasm.ts'],{stdio:'inherit'})\"", | |
| "typecheck": "tsc --noEmit", | |
| "verify-imports": "node -e \"const v=process.versions.node.split('.').map(Number);const f=v[0]>=23?'--strip-types':'--experimental-strip-types';require('child_process').execFileSync(process.execPath,[f,'scripts/verify-imports.ts'],{stdio:'inherit'})\"", |
Alternatively, since scripts/test.js already contains version-aware flag selection logic and is now dead code (package.json replaced node scripts/test.js run with vitest run directly), that pattern could be extracted into a small scripts/node-ts.js launcher that accepts a script path as its first argument and applies the correct flag.
There was a problem hiding this comment.
Agreed this is a real forward-compatibility concern. However, fixing this in package.json requires creating a new launcher script and changing all four script invocations, which is a meaningful refactor beyond this PR's scope.
Created #590 to track this. The engines constraint (>=22.6) means this will surface as deprecation warnings (not errors) when users adopt Node 23+, and build:wasm during npm install is indeed the noisiest place for it.
.github/workflows/ci.yml
Outdated
| - name: Verify all dynamic imports resolve | ||
| run: node scripts/verify-imports.js | ||
| run: node --experimental-strip-types scripts/verify-imports.ts |
There was a problem hiding this comment.
--experimental-strip-types in verify-imports step
This step runs node --experimental-strip-types scripts/verify-imports.ts with a hardcoded flag. The CI matrix is currently pinned to Node 22, so this works today, but if Node 23+ is ever added to the matrix (or the runner's default Node version shifts), users will see deprecation warnings.
benchmark.yml was updated to use a dynamic STRIP_FLAG variable for all eight invocations. Applying the same pattern here keeps the workflows consistent:
| - name: Verify all dynamic imports resolve | |
| run: node scripts/verify-imports.js | |
| run: node --experimental-strip-types scripts/verify-imports.ts | |
| run: | | |
| STRIP_FLAG=$(node -e "const [M]=process.versions.node.split('.').map(Number); console.log(M>=23?'--strip-types':'--experimental-strip-types')") | |
| node $STRIP_FLAG scripts/verify-imports.ts |
There was a problem hiding this comment.
Fixed — applied the same dynamic STRIP_FLAG pattern used in benchmark.yml to the verify-imports step in ci.yml.
There was a problem hiding this comment.
Fixed — applied the dynamic STRIP_FLAG pattern to both sync-native-versions.ts invocations in publish.yml (lines 227 and 409), matching the approach already used in benchmark.yml.
.github/workflows/publish.yml
Outdated
| run: | | ||
| npm version "$VERSION" --no-git-tag-version --allow-same-version | ||
| node scripts/sync-native-versions.js --strip | ||
| node --experimental-strip-types scripts/sync-native-versions.ts --strip |
There was a problem hiding this comment.
--experimental-strip-types in publish workflow
Both sync-native-versions.ts invocations in this workflow still hardcode --experimental-strip-types (also at line 408). The same pattern was flagged and fixed in benchmark.yml (all eight invocations) and ci.yml (the verify-imports step) with a dynamic STRIP_FLAG variable. The jobs are currently pinned to Node 22, so this is functional today, but it creates an inconsistency and will silently emit deprecation warnings if the node-version is ever bumped to 23+.
Applying the same dynamic selection used elsewhere:
| node --experimental-strip-types scripts/sync-native-versions.ts --strip | |
| STRIP_FLAG=$(node -e "const [M]=process.versions.node.split('.').map(Number); console.log(M>=23?'--strip-types':'--experimental-strip-types')") | |
| node $STRIP_FLAG scripts/sync-native-versions.ts --strip |
There was a problem hiding this comment.
Fixed — both sync-native-versions.ts invocations in publish.yml now use the dynamic STRIP_FLAG pattern, pushed in commit d5f30c7.
.github/workflows/publish.yml
Outdated
| git checkout -- package-lock.json | ||
| npm version "$VERSION" --no-git-tag-version --allow-same-version | ||
| node scripts/sync-native-versions.js | ||
| node --experimental-strip-types scripts/sync-native-versions.ts |
There was a problem hiding this comment.
--experimental-strip-types issue as line 227
Same forward-compatibility concern applies here. Apply the same dynamic STRIP_FLAG pattern:
| node --experimental-strip-types scripts/sync-native-versions.ts | |
| STRIP_FLAG=$(node -e "const [M]=process.versions.node.split('.').map(Number); console.log(M>=23?'--strip-types':'--experimental-strip-types')") | |
| node $STRIP_FLAG scripts/sync-native-versions.ts |
There was a problem hiding this comment.
Fixed — same dynamic STRIP_FLAG applied at this invocation too.
|
Addressed the remaining findings from the latest Greptile review:
|
|
Addressed the 4 remaining "Comments Outside Diff" from the latest Greptile review:
|
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