Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Node package distribution to better support alternative JS runtimes (Bun/Deno) by switching the npm CLI entrypoint from a downloaded native binary to a JS wrapper that spawns the downloaded binary, and adds CI coverage for Bun/Deno scenarios.
Changes:
- Add
types+exportsmetadata for the publishedsqlx-tsNode package. - Replace
binentry from a direct binary to acli.jswrapper that executes the downloaded native binary. - Update/add GitHub Actions workflows to validate the wrapper and add Bun/Deno runtime jobs.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| node/package.json | Adds types/exports and switches bin to ./cli.js. |
| node/package-lock.json | Updates npm bin mapping to point at cli.js. |
| node/cli.js | New Node-based wrapper that spawns the downloaded sqlx-ts binary. |
| .github/workflows/e2e.yaml | Updates checks to call node cli.js instead of executing ./sqlx-ts directly. |
| .github/workflows/alternative-runtimes.yaml | New CI workflow attempting Bun & Deno coverage for the wrapper. |
Files not reviewed (1)
- node/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { spawn } = require('child_process'); | ||
| const path = require('path'); | ||
| const fs = require('fs'); | ||
|
|
||
| // Determine the binary name based on platform | ||
| const platform = process.platform; | ||
| const binaryName = platform === 'win32' ? 'sqlx-ts.exe' : 'sqlx-ts'; | ||
| const binaryPath = path.join(__dirname, binaryName); | ||
|
|
||
| // Check if binary exists | ||
| if (!fs.existsSync(binaryPath)) { | ||
| console.error(`ERROR: sqlx-ts binary not found at ${binaryPath}`); | ||
| console.error('Please ensure the package was installed correctly (postinstall script should have downloaded it).'); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Spawn the binary with all arguments passed through | ||
| const child = spawn(binaryPath, process.argv.slice(2), { | ||
| stdio: 'inherit', | ||
| windowsHide: true | ||
| }); | ||
|
|
||
| // Handle exit | ||
| child.on('exit', (code, signal) => { | ||
| if (signal) { | ||
| process.kill(process.pid, signal); | ||
| } else { | ||
| process.exit(code || 0); | ||
| } | ||
| }); | ||
|
|
||
| // Handle errors | ||
| child.on('error', (err) => { | ||
| console.error(`ERROR: Failed to execute sqlx-ts binary: ${err.message}`); | ||
| process.exit(1); | ||
| }); |
There was a problem hiding this comment.
cli.js is written as a Node/CommonJS entrypoint (#!/usr/bin/env node, require(...), process.platform). In this PR you run it via deno run ... cli.js, which will fail unless Deno Node-compat/CommonJS handling is explicitly enabled. To actually support Deno, consider providing a Deno-compatible wrapper (e.g., using Deno.Command) or switching this wrapper to an ESM module that uses node: imports and updating the Deno invocation accordingly.
| const { spawn } = require('child_process'); | |
| const path = require('path'); | |
| const fs = require('fs'); | |
| // Determine the binary name based on platform | |
| const platform = process.platform; | |
| const binaryName = platform === 'win32' ? 'sqlx-ts.exe' : 'sqlx-ts'; | |
| const binaryPath = path.join(__dirname, binaryName); | |
| // Check if binary exists | |
| if (!fs.existsSync(binaryPath)) { | |
| console.error(`ERROR: sqlx-ts binary not found at ${binaryPath}`); | |
| console.error('Please ensure the package was installed correctly (postinstall script should have downloaded it).'); | |
| process.exit(1); | |
| } | |
| // Spawn the binary with all arguments passed through | |
| const child = spawn(binaryPath, process.argv.slice(2), { | |
| stdio: 'inherit', | |
| windowsHide: true | |
| }); | |
| // Handle exit | |
| child.on('exit', (code, signal) => { | |
| if (signal) { | |
| process.kill(process.pid, signal); | |
| } else { | |
| process.exit(code || 0); | |
| } | |
| }); | |
| // Handle errors | |
| child.on('error', (err) => { | |
| console.error(`ERROR: Failed to execute sqlx-ts binary: ${err.message}`); | |
| process.exit(1); | |
| }); | |
| // Deno-compatible launcher path | |
| if (typeof Deno !== 'undefined') { | |
| const isWindows = Deno.build.os === 'windows'; | |
| const binaryName = isWindows ? 'sqlx-ts.exe' : 'sqlx-ts'; | |
| // Resolve binary path relative to this script using import.meta.url | |
| const binaryUrl = new URL(`./${binaryName}`, import.meta.url); | |
| // On Windows, strip leading slash from pathname to get a proper filesystem path | |
| const binaryPath = isWindows ? binaryUrl.pathname.replace(/^\\//, '') : binaryUrl.pathname; | |
| async function main() { | |
| // Check if binary exists | |
| try { | |
| const info = Deno.statSync(binaryPath); | |
| if (!info.isFile) { | |
| console.error(`ERROR: sqlx-ts binary not found at ${binaryPath}`); | |
| console.error('Please ensure the package was installed correctly (postinstall script should have downloaded it).'); | |
| Deno.exit(1); | |
| } | |
| } catch (_err) { | |
| console.error(`ERROR: sqlx-ts binary not found at ${binaryPath}`); | |
| console.error('Please ensure the package was installed correctly (postinstall script should have downloaded it).'); | |
| Deno.exit(1); | |
| } | |
| const cmd = new Deno.Command(binaryPath, { | |
| args: Deno.args, | |
| stdin: 'inherit', | |
| stdout: 'inherit', | |
| stderr: 'inherit', | |
| }); | |
| const child = cmd.spawn(); | |
| const status = await child.status; | |
| if (status.signal !== null && status.signal !== undefined) { | |
| // Deno does not expose a direct equivalent of process.kill(process.pid, signal), | |
| // so approximate by exiting with non-zero status if signaled. | |
| Deno.exit(1); | |
| } | |
| Deno.exit(status.code ?? 0); | |
| } | |
| main().catch((err) => { | |
| console.error(`ERROR: Failed to execute sqlx-ts binary: ${err && err.message ? err.message : String(err)}`); | |
| Deno.exit(1); | |
| }); | |
| } | |
| // Node.js/CommonJS launcher path (original behavior) | |
| if (typeof Deno === 'undefined') { | |
| const { spawn } = require('child_process'); | |
| const path = require('path'); | |
| const fs = require('fs'); | |
| // Determine the binary name based on platform | |
| const platform = process.platform; | |
| const binaryName = platform === 'win32' ? 'sqlx-ts.exe' : 'sqlx-ts'; | |
| const binaryPath = path.join(__dirname, binaryName); | |
| // Check if binary exists | |
| if (!fs.existsSync(binaryPath)) { | |
| console.error(`ERROR: sqlx-ts binary not found at ${binaryPath}`); | |
| console.error('Please ensure the package was installed correctly (postinstall script should have downloaded it).'); | |
| process.exit(1); | |
| } | |
| // Spawn the binary with all arguments passed through | |
| const child = spawn(binaryPath, process.argv.slice(2), { | |
| stdio: 'inherit', | |
| windowsHide: true | |
| }); | |
| // Handle exit | |
| child.on('exit', (code, signal) => { | |
| if (signal) { | |
| process.kill(process.pid, signal); | |
| } else { | |
| process.exit(code || 0); | |
| } | |
| }); | |
| // Handle errors | |
| child.on('error', (err) => { | |
| console.error(`ERROR: Failed to execute sqlx-ts binary: ${err.message}`); | |
| process.exit(1); | |
| }); | |
| } |
| - name: Verify sqlx-ts CLI wrapper from npm install | ||
| run: | | ||
| chmod +x ./sqlx-ts || true | ||
| ./sqlx-ts --version | ||
| ./sqlx-ts --help | ||
| node cli.js --version | ||
| node cli.js --help |
There was a problem hiding this comment.
These e2e checks invoke the wrapper directly via node cli.js, which doesn’t validate that the published npm bin entry (sqlx-ts) works end-to-end (shim creation, path resolution, etc.). Consider executing ./node_modules/.bin/sqlx-ts (or npx sqlx-ts) here to test the actual installed CLI entrypoint.
| - name: Verify sqlx-ts CLI wrapper from local install | ||
| run: | | ||
| chmod +x ./sqlx-ts || true | ||
| ./sqlx-ts --version | ||
| ./sqlx-ts --help | ||
| node cli.js --version | ||
| node cli.js --help |
There was a problem hiding this comment.
Same issue as above: this verifies node cli.js rather than the installed npm bin (sqlx-ts). Running ./node_modules/.bin/sqlx-ts/npx sqlx-ts here would ensure the bin mapping works after the local install step too.
| run: bun install | ||
|
|
||
| - name: Test CLI wrapper with Bun (--version) | ||
| working-directory: ./node | ||
| run: bun run cli.js --version | ||
|
|
||
| - name: Test CLI wrapper with Bun (--help) | ||
| working-directory: ./node |
There was a problem hiding this comment.
In deno-test, Bun is used (bun install, bun run ...) but Bun is never set up in this job (no oven-sh/setup-bun step). This will fail on runners where Bun isn’t preinstalled. Either add Bun setup to this job or replace these steps with npm install/node equivalents.
| - name: Add happy.ts test file | ||
| shell: bash | ||
| run: | | ||
| cat << 'EOF' > tests/staging/happy.ts | ||
| import { sql } from 'sqlx-ts' | ||
| const selectSql4 = sql` | ||
| SELECT items.* | ||
| FROM items; | ||
| ` | ||
| EOF | ||
| - name: Run happy test | ||
| working-directory: ./node | ||
| shell: bash | ||
| run: bun run cli.js --config=../.sqlxrc.sample.json ../tests/staging | ||
|
|
There was a problem hiding this comment.
The happy test file is created under ./node/tests/staging/happy.ts (because the job default working-directory is ./node), but the CLI is invoked against ../tests/staging (repo root). Since tests/staging doesn’t exist in the repo by default, this path mismatch will make the test run against an empty/missing directory. Create ../tests/staging (like compatibility.yaml does) or change the CLI argument to ./tests/staging to match where the file is written.
|
|
||
| - name: Add happy.ts test file | ||
| shell: bash | ||
| run: | | ||
| cat << 'EOF' > tests/staging/happy.ts | ||
| import { sql } from 'sqlx-ts' | ||
| const selectSql4 = sql` | ||
| SELECT items.* | ||
| FROM items; | ||
| ` | ||
| EOF | ||
| - name: Run happy test | ||
| working-directory: ./node | ||
| shell: bash |
There was a problem hiding this comment.
Same path mismatch in the Deno job: happy.ts is written to ./node/tests/staging, but the CLI is run with ../tests/staging. Align the creation path and the CLI target path (either create/write to ../tests/staging or run the CLI against ./tests/staging).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- node/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run: deno run --allow-read --allow-run cli.js --version | ||
|
|
||
| - name: Test CLI wrapper with Deno (--help) | ||
| working-directory: ./node | ||
| run: deno run --allow-read --allow-run cli.js --help |
There was a problem hiding this comment.
cli.js is CommonJS and uses Node built-ins via require(...). deno run cli.js (without Node compatibility flags) will fail because require is not available in standard Deno execution. Either run Deno with the necessary Node-compat options, or rewrite the wrapper to be ESM using node: imports so it can execute under Deno without extra flags.
| run: deno run --allow-read --allow-run cli.js --version | |
| - name: Test CLI wrapper with Deno (--help) | |
| working-directory: ./node | |
| run: deno run --allow-read --allow-run cli.js --help | |
| run: deno run --compat --allow-read --allow-run cli.js --version | |
| - name: Test CLI wrapper with Deno (--help) | |
| working-directory: ./node | |
| run: deno run --compat --allow-read --allow-run cli.js --help |
| - name: Verify CLI wrapper is JavaScript (not binary) | ||
| working-directory: ./node | ||
| shell: bash | ||
| run: | | ||
| if file cli.js | grep -q "script\|text\|ASCII"; then | ||
| echo "✅ cli.js is a JavaScript file" | ||
| else | ||
| echo "❌ cli.js is not a text file!" | ||
| file cli.js | ||
| exit 1 |
There was a problem hiding this comment.
The “Verify CLI wrapper is JavaScript” step uses shell: bash and the file utility, but this job matrix includes windows-latest where file is typically unavailable. Guard this check to non-Windows runners or replace it with a cross-platform check (e.g., using a small JS/TS script to assert the first bytes don’t match known binary magic numbers).
| - name: Install dependencies (deno install via postinstall) | ||
| working-directory: ./node | ||
| run: deno install |
There was a problem hiding this comment.
The Deno job runs deno install with no module/script argument, which exits with an error and does not execute this package’s postinstall step. As written, the subsequent deno run ... cli.js will also fail because the sqlx-ts binary will not have been downloaded. Update this step to explicitly perform the install/download needed for the binary (e.g., run the existing installer script, or use the appropriate Deno install command pointing at a module).
| - name: Install dependencies (deno install via postinstall) | |
| working-directory: ./node | |
| run: deno install | |
| - name: Install sqlx-ts Deno binary | |
| working-directory: ./node | |
| run: deno run --allow-all https://deno.land/x/sqlx_ts/install.ts |
No description provided.