-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
lib,src,test,doc: add node:sqlite module #53752
Conversation
Review requested:
|
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.
I can't review the C++ code, but API and tests LGTM for a MVP.
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.
Yay, happy to see this come to fruition. Let's land as experimental and iterate 🎉
|
||
Constructs a new `SQLiteDatabaseSync` instance. | ||
|
||
### `database.close()` |
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.
It would be nifty to also support Symbol.dispose here but that can be in a follow up PR.
This is a lot more feature complete and ready than I was expecting, good job Colin! |
This is adding a new feature, so shouldn't be semver-minor? (and with this kind of change, |
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.
Love it! Amazing job @cjihrig
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.
lgtm
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.
Should we be concerned at all about the performance impact of people using these sync APIs within requests and blocking the event loop? With sync being the only option here I feel like the risk is increased.
If the concern is simplicity, I feel like only async would be better than only sync. Especially given top-level await. What reason is there to favour sync over async here?
I won't block on that concern, and in fact I think the work done so far is great. I would be happy landing what's here now. I just wanted to share my concern with going for sync first rather than async first.
SQLite runs in process and is often CPU bound, we do have async APIs for some CPU intensive stuff (like in crypto) but I suspect for most people this API is the better one performance wise (that said, in some cases an async API is preferable and we should expose that as well) |
If anyone wants to try out CodeSandbox demo: https://codesandbox.io/p/devbox/node-sqlite-with-node-js-nightly-vygw6p?file=%2Findex.js |
I just tested this and it's very slow and appears to ship with SQLite 3.46. Is this expected in the Nightly build? |
@karimfromjordan I think that's a limitation of codesandbox. I was unable to run the sandbox on my machine, and I was also unable to get NVM_NODEJS_ORG_MIRROR=https://nodejs.org/download/nightly/ nvm i node Running against an |
@bholmesdev I tested it locally. I ran 1000 inserts, with |
This PR is about the addition of SQLite into Node.js. If you have concerns about its usage, please visit the help repo. If you have concerns specific to the sandbox environment, please reach out to @karlhorky privately. |
@karimfromjordan thanks for sharing. I can confirm a slowdown, but I'm not seeing anything near 20x - more like 4x. Please note this is not optimized at all yet. When I start working on this again, I'll look into improving the perf if no one beats me to it. |
This is exciting. I always loved the PHP+MySql combo. And this one is one more reason for me to love nodeJS more. |
From a quick test, compiling SQLite in Node with the same settings used by better-sqlite3 yielded roughly the same performance. There may be other things to optimize as well, but it's good to know we aren't doing anything too awful 😄. |
PR-URL: #53752 Fixes: #53264 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 test_runner: * support glob matching coverage files (Aviv Keller) #53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682 PR-URL: #53826
Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 test_runner: * support glob matching coverage files (Aviv Keller) #53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682 PR-URL: #53826
Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 test_runner: * support glob matching coverage files (Aviv Keller) #53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682 PR-URL: #53826
Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 test_runner: * support glob matching coverage files (Aviv Keller) #53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682 PR-URL: #53826
Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462 test_runner: * support glob matching coverage files (Aviv Keller) #53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682 PR-URL: #53826
PR-URL: nodejs#53752 Fixes: nodejs#53264 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
Notable changes: http: * (SEMVER-MINOR) expose websockets (Natalia Venditto) nodejs#53721 lib: * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) nodejs#53752 module: * add `__esModule` to `require()`'d ESM (Joyee Cheung) nodejs#52166 path: * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) nodejs#52881 process: * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) nodejs#53239 stream: * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) nodejs#53462 test_runner: * support glob matching coverage files (Aviv Keller) nodejs#53553 worker: * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) nodejs#53682 PR-URL: nodejs#53826
I'm really grateful that Sqlite is natively supported in NodeJS and want to thank @cjihrig and everyone involved for working on this feature. Thanks for choosing Sqlite - it's fast and much more reliable than most alternatives. This feature would be super useful for Electron users - currently most Sqlite bindings don't work well for Electron apps, and having native support for Sqlite would make it easier to interact with the database. |
This is awesome! Is support for official SQLite extensions something that is being considered? In particular I am interested in the Session Extension. |
@cjihrig Actually the Session Extension is not a runtime loadable extension. It needs to be compiled into SQLite. To be able to use it with Node.js you would need to wrap its C API: https://www.sqlite.org/session/intro.html |
+1 for this. Supporting all of the sqlite3 amalgamation extensions should be a strong goal for Node.js (through wrapping the C APIs of each). Maybe a new issue concerning this topic specifically should be opened? |
@cjihrig @TheOneTheOnlyJJ I opened a PR so we can have some discussion about the API (among other things): #54181 |
t.assert.throws(() => { | ||
const stmt = db.prepare('INSERT INTO types (key, val) VALUES ($k, $v)'); | ||
stmt.run({ $k: 1, $unknown: 1 }); | ||
}, { | ||
code: 'ERR_INVALID_STATE', | ||
message: /Unknown named parameter '\$unknown'/, | ||
}); |
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.
@cjihrig Thanks for your work!
Regarding this case, I am a bit skeptical because better-sqlite3 allows for more named parameters to be passed than those actually used in the SQL. This can be useful when you build your SQL with conditions in a literal string, for example.
#53264 has been open for over a month with no objections, so I am opening this PR with an initial
node:sqlite
module. There is other functionality that could potentially be exposed in the future, but I believe this is enough for an experimental MVP.Fixes: #53264
Summary: Node.js now includes a built-in sqlite module (
require('node:sqlite')
) that becomes available when using the--experimental-sqlite
flagThe following example shows the basic usage of the
node:sqlite
module to openan in-memory database, write data to the database, and then read the data back.