-
Notifications
You must be signed in to change notification settings - Fork 133
Use Node.js native sqlite instead of better-sqlite3 #1357
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
base: master
Are you sure you want to change the base?
Conversation
|
Maybe it is better to move to nodejs build-in sql support https://nodejs.org/api/sqlite.html (available from Node 22) Spago next is a new tool and still not widely used so I don't think that there need to worry about old node version and ignore node platform development. |
|
@wclr sure, let's get some buy-in first from the maintainers and then we can also go that route. Main motivation is to avoid having to rebuild spago for different node versions. |
bin/index.dev.js
Outdated
| @@ -1,4 +1,4 @@ | |||
| #!/usr/bin/env node | |||
| #!/usr/bin/env -S node --no-warnings | |||
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.
Removing all the warnings is a terrible idea for a set of reasons 😄
Let's just exclude the warnings for experimental modules: --no-warnings=ExperimentalWarning
| FS.mkdirp Paths.globalCachePath | ||
| logDebug $ "Global cache: " <> Path.quote Paths.globalCachePath | ||
|
|
||
| -- Make sure we have git and purs |
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.
Node will fail with a horrible error if it doesn't find the module, so I think we should catch it at the startup and error out then.
Implementation sketch:
import Data.Int as Int
import Data.String as String
import Effect.Console as Console
import Node.Process as Process
-- first thing inside main:
main = do
-- Check that we're running on Node.js >= 22.5.0 (required for node:sqlite)
let
-- version is like "v22.5.0" or "22.5.0"
versionStr = String.stripPrefix (String.Pattern "v") Process.version
# fromMaybe Process.version
parts = String.split (String.Pattern ".") versionStr
case Array.take 2 parts >>= traverse Int.fromString of
Just [major, minor]
| major > 22 -> pure unit
| major == 22 && minor >= 5 -> pure unit
| otherwise -> do
Console.error $ "Error: spago requires Node.js v22.5.0 or later (found " <> Process.version <> ")"
Process.exit' 1
_ -> do
-- couldn't parse, issue a warning and let it crash later
..together with this we should also add the engines field to the package.json. Node won't enforce it on install, but will complain about it:
{
"engines": {
"node": ">=22.5.0"
}
}
src/Spago/Db.js
Outdated
| fs.mkdirSync(dir, { recursive: true }); | ||
| } | ||
| } catch (err) { | ||
| // ignore - will fail later if needed |
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.
why are we ignoring the error here?
src/Spago/Db.js
Outdated
| .prepare("SELECT * FROM package_sets WHERE compiler = ? ORDER BY date DESC LIMIT 1") | ||
| .get(compiler); |
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.
we use positional arguments in a few places, we should use them everywhere, e.g.:
| .prepare("SELECT * FROM package_sets WHERE compiler = ? ORDER BY date DESC LIMIT 1") | |
| .get(compiler); | |
| .prepare("SELECT * FROM package_sets WHERE compiler = @compiler ORDER BY date DESC LIMIT 1") | |
| .get({ compiler }); |
src/Spago/Db.js
Outdated
| .all(); | ||
| return row; | ||
| } | ||
| return Promise.resolve().then(() => { |
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.
The existing code is in Effect, and this database code is sync as well, why turn everything into Promises?
4bc08d1 to
73f07cc
Compare
- Use Node.js native node:sqlite module (available from Node 22.5.0) - Add Node.js version check at startup - Add engines field to package.json - Use named parameters consistently in SQL queries - Suppress ExperimentalWarning for node:sqlite in shebang and CI Benefits: - Zero native compilation dependencies (no node-gyp) - Built-in Node.js module - will stabilize over time - Same performance - local file-based SQLite database
73f07cc to
3a032f3
Compare
🐰 Generated with Crush Assisted-by: Claude Opus 4.5 via Crush <crush@charm.land>
Replace
better-sqlite3with Node.js nativenode:sqlitemodule.@paramwith object argument)ExperimentalWarningin CLI and tests