-
Notifications
You must be signed in to change notification settings - Fork 17
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 13 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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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. |
||
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-typesin verify-imports stepThis step runs
node --experimental-strip-types scripts/verify-imports.tswith 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.ymlwas updated to use a dynamicSTRIP_FLAGvariable for all eight invocations. Applying the same pattern here keeps the workflows consistent: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 — applied the same dynamic
STRIP_FLAGpattern used inbenchmark.ymlto theverify-importsstep inci.yml.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 — 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.