-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Expose SQLite? #53264
Comments
I imagine this idea will get a lot of support and a lot of opposition. If someone else handles the consensus building aspect, I'd be happy to work on the implementation side. |
Shall we use the same interface as https://github.com/TryGhost/node-sqlite3? |
@gabrielschulhof On the rare occasion that I voluntarily write JavaScript code, I personally prefer the That being said, the non-blocking behavior of Perhaps Node.js could allow native addons to link against the bundled SQLite dependency instead and leave the API to third-party packages. |
I think at the very least let's land the |
FWIW, SQLite has followed semver since version 3.9.0 in 2015, and the current major version is still 3:
|
I would like to bring attention to esqlite by @mscdex, which outperform(s/ed) better-sqlite3 in a number of cases while being asynchronous. https://github.com/mscdex/esqlite |
It looks like we have consensus around experimenting with this (i.e. prototyping a sqlite binding in core), just for further visibility I'll also ping the TSC in chat. As for the API I personally really prefer a synchronous one over an asynchronous one since SQLite is built to run in-process. I think in the common use case this is significantly simpler and likely faster. That said, an asynchronous promise based API should also be fine. Funnily enough 2 people asked me about this feature today - a good luck omen it's something users find interesting. |
We already see different opinions on the API. If there is no "one size fits all" API, should we have one in core? If we make it possible for native addons to link against the SQLite dependency that's embedded in Node.js, then that should let third-party packages define whatever API they want without having to ship SQLite. |
Currently the different opinions are basically sync vs async. The basic API could be something like this class Database {
// path can be :memory: or the file path
constructor(path: string, options?: { readonly?: boolean, create?: boolean }) {}
query(statement: string): Statement; // works like prepare, like bun is doing right know
open(): void; // maybe we can just open automatically? and add a option to disable auto-open
close(): void;
}
type AllResult = Array<any>;
type RunResult = void; // maybe boolean?
type GetResult = any;
class Statement {
all(...params: any[]): Promise<AllResult>;
run(...params: any[]): Promise<RunResult>;
get(...params: any[]): Promise<GetResult>;
// we can also introduce sync methods, like fs
allSync(...params: any[]): AllResult;
runSync(...params: any[]): RunResult;
getSync(...params: any[]): GetResult;
} Sync has advantages, and async too, so ship both and be happy. In the first iteration, we can stick with the easiest/simplest implementation, which is probably the sync version. EDIT: I didn't talk about transactions because it didn't even matter in this first iteration, is better to discuss this method when we have at least the basic methods, then we will have more knowledge on what could be the best API for exposing transactions.
The point is that you don't need to use third-party packages to be able to use sqlite, but you can still do that even if we have our own API. |
We've had very little api discussion so far, I think it's fine to talk about stuff and see if we can reach consensus? |
@tniessen looping back to your comment here: #53264 (comment) I tend to think the better-sqlite3 sync API is better for most cases and given SQLite's execution model and the motivation of users asking to add it to Node.js (not as a DB server replacement but for local configuration, at least that's what I've been hearing). I see your point though. There is interesting discussion here: https://sqlite.org/forum/forumpost/7c90893579 as well. I think it may be a good idea to reach out to the SQLite team and ask, so I went ahead and opened https://sqlite.org/forum/forumpost/96f9f78fc1 |
I think it makes the most sense to do something similar to the EDIT: Very early, but here is the branch. |
Another thing I think is worth discussing is whether or not it makes sense to expose this as its own module ( |
One note: if you do decide to include SQLite in Node.js, please statically compile an up-to-date SQLite library instead of relying on the host's SQLite library! This is a problem with Python's SQLite library and Bun's SQLite library: By default they use whatever SQLite version the OS has installed, which leads to headaches. Some default SQLite builds omit features like SQLite extensions, JSON, or math functions. Or they are stuck on old version of SQLite which lack window functions, So if you statically compile a recent SQLite version, say |
@asg017 Node bundles 3.46.0 for web storage support already so presumably that's the one that will be used in this case. |
Also, regarding sync vs async: the
I've never benchmarked it myself, but this is the sole reason why I always use I know a lot of Node.js developers would expect an async API (like |
Just to add my 2-cents, sync makes sense for simple queries. But analytical queries, things like computeing averages and sum can take a fair amount of time. Given that some users will want to use this for things like dashboards, that can issue a bunch of aggregation queries, an async API would make more sense. |
I agree with that point from better-sqlite3. A counterpoint is that sqlite might not be the only thing the application is doing. But if a sqlite call is blocking, then nothing else can happen. This is why both sync and async APIs should be provided. |
This is also my conclusion based on https://sqlite.org/forum/forumpost/96f9f78fc1 |
I'll also escalate the questions to the TSC chat to make sure we have consensus. |
Any reason to put it in |
FWIW, that discussion also raised this point:
This reinforces my opinion that we should make it easy for third-party packages to link against the bundled version of SQLite, assuming we are committing to shipping SQLite forever anyway. |
With asynchronous APIs, I think it's important to very carefully design a good transaction interface. |
Yes and that sounds like a good idea. That does not preclude us from shipping our own wrapper as well. I think a major theme in recent Node features (the test runner, task runner, watch mode, util.parseArgs etc) isn't to be the best or have the richest functionality in those spaces - it's to provide sensible and safe defaults for most projects. I think no matter what API we ship, userland will still provide alternatives and projects using sqlite heavily for more than configuration are likely to still use those wrappers. (Similarly to projects needing browser testing, advanced task runner features, rich argument parsing, more fs convenience methods etc). So I am not that concerned about our API not being "feature complete"
Assuming you had no messy language constraints whatsoever - what API would you pick? I personally think we might want to stick with disposers (Symbol.dispose) and not the disposer pattern - but honestly even if we have very rudimentary transaction support and leave richer APIs to userland I'm fine with that. |
Not particularly. A new core module is a bigger undertaking - particularly since the decision to require the |
I personally recommend using most of the amazing API of https://github.com/WiseLibs/better-sqlite3, i.e. use synchronous APIs. They generically offer better performance with SQLite on modern HW. I'm not opposed to have sync and async version of those. cc @JoshuaWise @mceachen for visiblity. |
Proper SQLite support within Node.js would be splendid! A couple thoughts:
One suggestion from the peanut gallery: I’d suggest this API live in a |
This seems to be the consensus so far. |
I'm not trying to knock the better-sqlite3 API at all, but my understanding is that this is already not the case. The Bun docs mention that their API was inspired by better-sqlite3, but they already seem to have diverged from it some (and probably will continue to do so). That approach also would not offer any compatibility with other sqlite modules like Deno's or node-sqlite3 (which has roughly twice the weekly downloads according to npm). |
Another question regarding the API - if we were to start off following the better-sqlite3 API and wanted to add async support in the future, what would that look like? Node has generally had async APIs, and appended |
@cjihrig honestly since naming is very bikesheddy in this case I think you should just pick one. Here is an example color for our bike shed:
But I think that's not one of the things that will prevent a PR from landing so I wouldn't worry too much about it. |
I have a few thoughts on this. Thoughts on an appropriate APII designed However, it should be noted that many asynchronous APIs would simply break or result in undefined behavior or unsafe memory accesses when combined with much of the behavior provided by SQLite. For example, SQLite allows you to register user-defined SQL functions in the host language (i.e., JavaScript in this case). These function would be invoked during certain SQL queries, but if those SQL queries were executed in a thread pool, it will result in unsafe cross-thread usage of JavaScript contexts (which is very, very bad). Similarly, transactions become prone to error and terrible performance when performed in an asynchronous manner, since transactions in SQLite lock the entire database. Therefore, any asynchronous API for SQLite in Node.js would need to be highly limited, exposing only the most basic functionality that SQLite offers. To safely separate the synchronous and asynchronous worlds, I recommend exposing two different classes, I believe an approach like this would align with the existing patterns/philosophy of Node.js's core libraries, and would satisfy the memory safety that is expected of the Node.js runtime. Sprinkle in a few high-level convenience methods, and let user-land do the rest. Thoughts on SQLite versionsPerhaps most importantly (much more important than the API, in my opinion), is how to deal with the issue of SQLite versions. Besides the fact that the SQLite developers release new versions many times per year, SQLite is designed to be customized at compile-time, giving the user the power to enable certain advanced features, disable deprecated features that may harm performance or be unsafe, or even change the behavior of basic SQL expressions (like case-sensitivity, unicode, etc.). In |
I second this assertion of @JoshuaWise and think it is very important to build this capability in the node's integrated version of SQLite. I want the same (possibly customized) version of SQLite powering my node application as well as my CLI so there are no surprises when I am building my application or analyzing my data. For this reason itself, it is very important that not only can I customize my SQLite with the extensions I need, but do it in multiple locations, and as easily as possible because I am not a C programmer. Even the most obvious steps for a C programmer can be very confusing for me. |
I have a slightly different view:
If someone wanted something outside of that, they should use a 3rd party package like
This is kindof how Python works: the builtin That means that the SQLite library offered in |
FWIW, the SQLite developers strongly recommend against having more than a single copy of SQLite per process, see How To Corrupt An SQLite Database File. |
That seems like it would raise a more immediate question about WebStorage and its impact on code already using a userland sqlite module. How do Deno/Bun/Python etc. deal with that? |
@tniessen ooh thanks for sharing, hadn't seen that before! But in this case, if Node.js statically links a "golden" SQLite library for My hunch is that there could be only 1 statically linked copy of SQLite, but other dynamically linked SQLite copies would be fine? Especially since this pattern is used in Python/Deno (which uses SQLite for localStorage + KV), but I'm no expert here |
I would think that Node’s internal SQLite would use different files on disk for storing the database than the files on disk used by these various packages? And if that’s the case, then I don’t see why they would interfere with each other. |
right, as long as I continue to use the SQLite driver with which I created my db, I would not expect any problem. So, if I use |
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]>
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]>
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]>
We agreed (the TSC) to allow SQLite into the project for the
localStorage
API at #52435It would be neat if we exposed that to userland similraly to how Bun does, there is a lot of prior art of languages and runtimes embracing SQLite for a local configuration database like python, php, Tcl and others.
This issue is to discuss it and raise concerns about whether or not it makes sense to expose SQLite bindings given we already bundle it.
The text was updated successfully, but these errors were encountered: