fix: add minimum Node.js version check at CLI startup - #83
Conversation
- Add runtime version check at the top of src/cli.ts that exits with
a clear error message if Node.js major version is below 18
- Add "engines": { "node": ">=18" } to package.json for install-time
warnings via npm/yarn
- Add cli-version-check.test.ts with 8 test cases covering the
version-check decision logic
Closes pavanvamsi3#63
| "version": "1.2.0", | ||
| "description": "A local dashboard to visualize and analyze your GitHub Copilot CLI sessions", | ||
| "engines": { | ||
| "node": ">=18" |
There was a problem hiding this comment.
The engines field sets the minimum at Node 18, but commit d635f46 on main already dropped Node 18 from CI because vitest 4, vite 7, and better-sqlite3 12 all require Node 20+. This should be "node": ">=20" to match reality.
|
|
||
| // Node.js version gate — must run before any modern syntax/APIs. | ||
| const [major] = process.versions.node.split('.').map(Number); | ||
| if (major < 18) { |
There was a problem hiding this comment.
Same issue — the threshold should be 20, not 18, and the error message should read "Node.js 20 or later" to match the project's actual CI requirement.
| * The actual cli.ts uses `process.versions.node` and `process.exit` directly, | ||
| * so here we test the *decision logic* (should it reject?) in isolation. | ||
| */ | ||
| function checkNodeVersion(versionString: string): { ok: boolean; major: number } { |
There was a problem hiding this comment.
The test file re-implements the version check as a local checkNodeVersion function rather than importing or exercising the actual cli.ts code. If someone bumps the threshold in cli.ts from 18 to 20 and forgets to update this copy, all 8 tests will still pass while the production check is wrong. Consider extracting the pure decision logic from cli.ts as a named export (e.g. export function meetsNodeRequirement(version: string): boolean) so this test can import and verify the real implementation.
kannupriyakalra
left a comment
kannupriyakalra
left a comment
There was a problem hiding this comment.
Review summary
Verdict: Request changes — 2 errors, 1 warning.
What this PR does well
- The approach is solid: gate at the top of
cli.tsusing only pre-ES2015 features, so the check itself can never crash on old Node. Good instinct. enginesfield inpackage.jsonis the right companion piece.- 8 test cases with clear names and correct assertions.
Errors (must fix)
Wrong minimum version (×2)
Both package.json ("node": ">=18") and the if (major < 18) guard in cli.ts set the bar at Node 18, but commit d635f46 on main already bumped the actual minimum to Node 20 because vitest 4, vite 7, and better-sqlite3 12 all require it. Setting the minimum to 18 means a user on Node 18 or 19 gets past the gate, installs the package, and then hits cryptic build/runtime errors — exactly the problem this PR is trying to prevent.
Fix: change both occurrences to >=20 / < 20 and update the error message and test cases to say "Node.js 20 or later".
Warning
Tests verify a copy of the logic, not the real code
cli-version-check.test.ts duplicates the threshold in a local checkNodeVersion function. If someone later changes the guard in cli.ts without touching the test file, all tests stay green while the production code is wrong. See the inline comment for a suggested refactor (export the pure check from cli.ts so the test imports and exercises the real implementation).
|
Hey @vansh7nvc — left a review with two blockers to resolve before this can merge:
Great idea for the feature overall — just needs those two fixes. Let me know if you have questions! |
Address reviewer feedback: 1. Bumped Node version threshold in package.json and cli.ts from >=18 to >=20, as vitest 4, vite 7, and better-sqlite3 12 require Node 20+. 2. Extracted pure `meetsNodeRequirement` function from cli.ts and exported it, allowing tests to import and exercise the actual production logic rather than duplicating it. Wrap the CLI side-effects in a guard so they don't run when imported in vitest.
…check # Conflicts: # src/cli.ts
2 participants
Summary
Fixes #63 — adds a minimum Node.js version check at CLI startup.
The project requires Node.js 18+ (for
fetch,fs/promises, ES2020 syntax, etc.) but previously had no runtime check. If a user ran the CLI with an older Node.js version, they'd get a cryptic syntax error instead of a helpful message.Changes
1. Runtime version check in
src/cli.tsAdded a version gate at the very top of the CLI entry point (after the shebang, before any imports or modern syntax). If
process.versions.nodemajor version is below 18, it prints a clear error message and exits with code 1:The check only uses features available in all Node.js versions (
process.versions.node,String.split,console.error,process.exit), so it will never itself crash on older Node.2.
enginesfield inpackage.jsonAdded
"engines": { "node": ">=18" }so npm/yarn can warn users at install time.3. Tests
Added
src/__tests__/cli-version-check.test.tswith 8 test cases verifying:Testing
claude-code-sessions.test.tsare unrelated)