-
Notifications
You must be signed in to change notification settings - Fork 16
feat(types): migrate test suite and config files to TypeScript (Phase 6) #588
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 6 commits
8b2ad0a
7d77811
476e642
8cfb084
5c50b2a
a6b8f2e
04a2e50
57bcfaf
a21a419
563de54
9b8eef1
966ec3e
f19aec9
39d07f1
d5f30c7
83f2633
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -93,11 +93,11 @@ jobs: | |||||
| if [ "${{ steps.mode.outputs.source }}" = "npm" ]; then | ||||||
| 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 | ||||||
|
|
||||||
| - 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 | ||||||
|
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 four The main benchmark runners (lines 96, 247, 384, 521) received the same fix in this PR and correctly keep
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 — all eight |
||||||
|
|
||||||
| - name: Upload build result | ||||||
| if: steps.existing.outputs.skip != 'true' | ||||||
|
|
@@ -244,11 +244,11 @@ jobs: | |||||
| if [ "${{ steps.mode.outputs.source }}" = "npm" ]; then | ||||||
| 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 | ||||||
|
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.
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 — same |
||||||
|
|
||||||
| - name: Update embedding report | ||||||
| if: steps.existing.outputs.skip != 'true' | ||||||
| run: node scripts/update-embedding-report.js embedding-benchmark-result.json | ||||||
| run: node --experimental-strip-types scripts/update-embedding-report.ts embedding-benchmark-result.json | ||||||
|
|
||||||
| - name: Upload embedding result | ||||||
| if: steps.existing.outputs.skip != 'true' | ||||||
|
|
@@ -381,11 +381,11 @@ jobs: | |||||
| if [ "${{ steps.mode.outputs.source }}" = "npm" ]; then | ||||||
| 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 | ||||||
|
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.
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 — same |
||||||
|
|
||||||
| - name: Update query report | ||||||
| if: steps.existing.outputs.skip != 'true' | ||||||
| run: node scripts/update-query-report.js query-benchmark-result.json | ||||||
| run: node --experimental-strip-types scripts/update-query-report.ts query-benchmark-result.json | ||||||
|
|
||||||
| - name: Upload query result | ||||||
| if: steps.existing.outputs.skip != 'true' | ||||||
|
|
@@ -518,11 +518,11 @@ jobs: | |||||
| if [ "${{ steps.mode.outputs.source }}" = "npm" ]; then | ||||||
| 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 | ||||||
|
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.
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 — same |
||||||
|
|
||||||
| - name: Update incremental report | ||||||
| if: steps.existing.outputs.skip != 'true' | ||||||
| run: node scripts/update-incremental-report.js incremental-benchmark-result.json | ||||||
| run: node --experimental-strip-types scripts/update-incremental-report.ts incremental-benchmark-result.json | ||||||
|
|
||||||
| - name: Upload incremental result | ||||||
| if: steps.existing.outputs.skip != 'true' | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,7 +43,7 @@ jobs: | |||||||||||||
| fail-fast: false | ||||||||||||||
| matrix: | ||||||||||||||
| os: [ubuntu-latest, macos-latest, windows-latest] | ||||||||||||||
| node-version: [20, 22] | ||||||||||||||
| node-version: [22] | ||||||||||||||
|
|
||||||||||||||
| runs-on: ${{ matrix.os }} | ||||||||||||||
| name: Test Node ${{ matrix.node-version }} (${{ matrix.os }}) | ||||||||||||||
|
|
@@ -113,7 +113,7 @@ jobs: | |||||||||||||
| node-version: 22 | ||||||||||||||
|
|
||||||||||||||
| - name: Verify all dynamic imports resolve | ||||||||||||||
| run: node scripts/verify-imports.js | ||||||||||||||
| run: node --experimental-strip-types scripts/verify-imports.ts | ||||||||||||||
|
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.
This step runs
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 — applied the same dynamic
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 — 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. |
||||||||||||||
|
|
||||||||||||||
| rust-check: | ||||||||||||||
| runs-on: ubuntu-latest | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -142,7 +142,7 @@ jobs: | |||||||
| - name: Setup Node.js | ||||||||
| uses: actions/setup-node@v6 | ||||||||
| with: | ||||||||
| node-version: 20 | ||||||||
| node-version: 22 | ||||||||
|
|
||||||||
| - name: Setup Rust | ||||||||
| uses: dtolnay/rust-toolchain@stable | ||||||||
|
|
@@ -224,7 +224,7 @@ jobs: | |||||||
| VERSION: ${{ needs.compute-version.outputs.version }} | ||||||||
| 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 | ||||||||
|
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.
Both Applying the same dynamic selection used elsewhere:
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 — both sync-native-versions.ts invocations in publish.yml now use the dynamic STRIP_FLAG pattern, pushed in commit d5f30c7. |
||||||||
| echo "Packaging version $VERSION" | ||||||||
|
|
||||||||
| - name: Build TypeScript | ||||||||
|
|
@@ -405,7 +405,7 @@ jobs: | |||||||
| run: | | ||||||||
| 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 | ||||||||
|
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.
Same forward-compatibility concern applies here. Apply the same dynamic
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 — same dynamic STRIP_FLAG applied at this invocation too. |
||||||||
| echo "Publishing version $VERSION" | ||||||||
|
|
||||||||
| - name: Build TypeScript | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,5 +7,5 @@ docs/ | |
| *.db | ||
| .codegraphrc.json | ||
| .versionrc.json | ||
| commitlint.config.js | ||
| commitlint.config.ts | ||
| codegraph-improvements.md | ||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -48,7 +48,7 @@ fi | |||||||
|
|
||||||||
| # 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 | ||||||||
|
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.
This shell script hardcodes Since the package now requires Node >=22.6, consider a version-aware invocation:
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 — the pre-commit.sh example now dynamically selects between |
||||||||
|
|
||||||||
| if [ -z "$RESULT" ]; then | ||||||||
| exit 0 | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,26 +26,26 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||
| "README.md" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ], | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "engines": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "node": ">=20" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "node": ">=22.6" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
28
to
30
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 same issue affects
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 — tightened engines constraint to |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| "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').replaceAll('./index.ts','./index.js'))\"", | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "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" | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+33
to
+48
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.
Every user-facing npm script that invokes a Affected scripts:
Users on Node 23+ will see a deprecation warning for every invocation. Because
Suggested change
Alternatively, since
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. 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 ( |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "keywords": [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "codegraph", | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,28 @@ | ||||||||||||||||||||
| /** | ||||||||||||||||||||
| * ESM resolve hook — rewrites .js specifiers to .ts when the .js file | ||||||||||||||||||||
| * does not exist on disk. Needed because TypeScript's moduleResolution | ||||||||||||||||||||
| * "nodenext" convention uses .js extensions in imports, but at runtime | ||||||||||||||||||||
| * the source files are .ts. | ||||||||||||||||||||
| * | ||||||||||||||||||||
| * Loaded via module.register() from ts-resolve-loader.ts. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import { existsSync } from 'node:fs'; | ||||||||||||||||||||
| import { fileURLToPath } from 'node:url'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| 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 }> { | ||||||||||||||||||||
|
Comment on lines
+14
to
+17
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.
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 — replaced bare |
||||||||||||||||||||
| try { | ||||||||||||||||||||
| return await nextResolve(specifier, context); | ||||||||||||||||||||
| } catch (err: any) { | ||||||||||||||||||||
| // Only attempt .js → .ts fallback for file-relative specifiers | ||||||||||||||||||||
| if (err?.code === 'ERR_MODULE_NOT_FOUND' && specifier.endsWith('.js')) { | ||||||||||||||||||||
| const tsSpecifier = specifier.slice(0, -3) + '.ts'; | ||||||||||||||||||||
| return nextResolve(tsSpecifier, context); | ||||||||||||||||||||
| } | ||||||||||||||||||||
| throw err; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| } | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| /** | ||
| * 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)); | ||
|
Comment on lines
+1
to
+9
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 PR description notes that ESM loader hooks must stay
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 — removed both
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. 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 The Reverted the deletion. These files are correctly part of the PR. |
||
This file was deleted.
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.
--importloader — benchmark scripts will fail on Node 22scripts/benchmark.ts(andembedding-benchmark.ts,query-benchmark.ts,incremental-benchmark.ts) contain cross-script imports using.jsspecifiers:Both
scripts/lib/bench-config.jsandscripts/lib/fork-engine.jsdo not exist on disk — only their.tscounterparts do. Node 22's--experimental-strip-typesstrips type annotations but does not rewrite.jsspecifiers to.ts(that resolution was only added in Node 23+). Without the--importESM loader hook, these scripts will fail immediately withERR_MODULE_NOT_FOUND.The fix is to add
--import ./scripts/ts-resolve-loader.js(the.jsversion is still present and fully functional) before the script path. The same fix applies at lines 247, 384, and 521: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 — added
--import ./scripts/ts-resolve-loader.jsto the benchmark.ts invocation so .js specifiers resolve to their .ts counterparts on Node 22.