-
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
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. |
f32efe6 to
295c45b
Compare
6c5b095 to
ee35778
Compare
f-f
left a comment
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.
Looks great, thanks!
- 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
79dfca7 to
06d9e75
Compare
node:sqlite has locking issues on Windows where rapid sequential inserts fail with 'database is locked' even with timeout set. Using BEGIN IMMEDIATE acquires the lock once for all inserts.
4e624ef to
30d641a
Compare
Replace
better-sqlite3with Node.js nativenode:sqlitemodule.@paramwith object argument)ExperimentalWarningin CLI and tests