sqlite: mark as release candidate#61262
Conversation
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61262 +/- ##
==========================================
- Coverage 88.54% 88.53% -0.01%
==========================================
Files 704 704
Lines 208734 208756 +22
Branches 40271 40277 +6
==========================================
+ Hits 184823 184830 +7
- Misses 15932 15936 +4
- Partials 7979 7990 +11
🚀 New features to boost your workflow:
|
cjihrig
left a comment
There was a problem hiding this comment.
LGTM. I still think there should be a definitive decision made about an async API before stabilizing it though because the presence or lack of an async API could influence the sync API.
PR-URL: REPLACEME
25e14fa to
c02731d
Compare
tpoisseau
left a comment
There was a problem hiding this comment.
The API looks good for a release candidate.
@cjihrig Is there an issue to track this? I personally don't think we need an Async API for the generic case. However, something like https://github.com/mcollina/sqlite-pool to provide concurrent read transactions might be interesting to support. https://www.sqlite.org/lang_transaction.html Having said that, the API to support this would need to be significantly different. |
@louwers yes, breaking changes would still be possible but we'd try to avoid them unless there are major usability issues. |
louwers
left a comment
There was a problem hiding this comment.
I don't know how much weight my vote counts, but I would definitely wait until the async API (#59109) has landed before marking it as a RC.
I personally think DatabaseSync should be renamed to Database, because synchronous should be the default since 9 out of 10 times it will be more performant than using the (future) asynchronous API.
|
Moving to stabilizing what we have does not prevent new features to be added. Keeping api experimental forever is not really good for the project. |
|
The tracking issue is at #54307.
This is the type of thing I meant. The reason the name |
The issue is: #54307. I took longer than I expected, but I've been working on it. I love the reference to the sqlite-pool, it's something I added due to the nature of sqlite with concurrent operations. I should, finally, have a PR for review by the end of this week |
I think trying to maintain consistency within core is a good thing. And, with all due respect, that is just your opinion. While I do agree with you, I have seen plenty of people complain about |
We could test it. I think for a lot of queries, the serialization overhead to and from a worker thread is larger than running the query in the main thread. Guiding people to use the recommended API (in my opinion) is more important than consistency. Is it really more consistent though? Is there another class with a I am not a fan of the API where you have separate Maybe we could rename const { Database } from 'node:sqlite';
const db = new Database('...', {
threadingMode: 'multiThread'
});
// will fail if threadingMode is 'singleThread'
const dbPool = db.pool; // DatabasePool instance
await db.pool.exec('...'); // async
db.exec('...'); // syncAnother advantage would be that is that it's easier to use both sync and async queries from the same DB handle. |
I'm not sure if there are any classes, but IMO it's definitely more consistent with, for example, the I'll stop commenting on this discussion though. |
|
Let's move the discussion to #54307 |
|
@Waldenesque I think you offer a valuable viewpoint. The async API not being ready should not be a blocker for stabilizing the SQLite API, but we should at least agree on what the extension point will be when and if an async API is added. For example if we should have a unified |
|
The stability index definition suggests:
So if the worry is that "there will be changes", 1.2 does not forbid that and already warns about it. Personally my understanding of the difference between 1.1. and 1.2 is "how certain you are about the likelihood of changes - >x% or < x%?" (different people might have different x, I'd say 20% is my personal threshold). If the worry is that it will encourage usage and make changes more difficult, perhaps it's worth gauging how many users are already using to make it difficult already. Note that breaking changes even after a feature reaches stability are still possible. Just that they will have to be semver-major (in experimental stages, that's not a hard requirement, but can still be done out of caution). |
Offense certainly wasn’t intended. My guess is that if you asked a normal node user, (especially if they’ve interacted with if MySQL or Postgres) if an async call could block another async call, I suspect that they would be surprised. Any async api will require careful verbiage to describe the footguns. |
louwers
left a comment
There was a problem hiding this comment.
@mcollina brought up a good point that an API that makes blocking the main thread hard to do by accident is a good thing.
I think we're still a way off from landing a good async implementation and API, but I think we can proceed with this.
SQLTagStore does not have a public constructor so it can be renamed later without much issues if needed.
|
I agree keeping Sync in the name is good to label things that can block. I would remove the redundant |
|
I removed my earlier comments, which I now think are distractions. Colin's answer to my question stands by itself, so there's no point keeping my chatter around. (I was never eager to stick my nose into this in the first place, I did it reluctantly.) After some ruminations I've come around to feeling a warm fuzzy about the sync API moving toward stabilization as it is. If anyone is still on the other side, it might be useful for them to know why, and I was writing something. However I just noticed @louwers seems to be coming around too, so for now I'll skip it. Maybe everybody is finding warm fuzzies. |
|
Would be nice to clean up the API and remove the 4 now redundant functions Can we get confirmation from reviewers you want to keep these before marking as release candidate. Would be nice to have everyone setting statement options the same way with database.prepare(sql[, options]) |
|
@mike-git374 why are they redundant? Is there a tracking issue for that removal? |
|
They are not completely redundant. The existing methods allow you to change the behavior of an existing prepared statement, which This seems ready to merge. Please land it. |
|
The
notable-change
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section. |
|
Landed in c9acf34 |
Is this going to break branch-diff? |
Although the C++ Prepared statements, as far as SQLite is concerned, are immutable constructs. If we adopt that as well it reduces both the API complexity and the potential for surprising behavior. These options are purely Node.js-side marshaling flags (not SQLite state), and since (I think |
|
FYI |
|
I agree that using the options on EDIT: It's also worth noting that chaining calls on the statement object seems to be more in line with how other SQLite APIs work. Neither better-sqlite3 nor bun:sqlite appear to have documented options passed to |
PR-URL: REPLACEME PR-URL: #61262 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
Notable changes: http2: * (SEMVER-MINOR) add http1Options for HTTP/1 fallback configuration (Amol Yadav) #61713 sea: * (SEMVER-MINOR) support ESM entry point in SEA (Joyee Cheung) #61813 sqlite: * mark as release candidate (Matteo Collina) #61262 stream: * (SEMVER-MINOR) rename `Duplex.toWeb()` type option to `readableType` (René) #61632 test_runner: * (SEMVER-MINOR) show interrupted test on SIGINT (Matteo Collina) #61676 PR-URL: #61922
Notable changes: http2: * (SEMVER-MINOR) add http1Options for HTTP/1 fallback configuration (Amol Yadav) #61713 sea: * (SEMVER-MINOR) support ESM entry point in SEA (Joyee Cheung) #61813 sqlite: * mark as release candidate (Matteo Collina) #61262 stream: * (SEMVER-MINOR) rename `Duplex.toWeb()` type option to `readableType` (René) #61632 test_runner: * (SEMVER-MINOR) show interrupted test on SIGINT (Matteo Collina) #61676 PR-URL: #61922
Notable changes: http2: * (SEMVER-MINOR) add http1Options for HTTP/1 fallback configuration (Amol Yadav) #61713 sea: * (SEMVER-MINOR) support ESM entry point in SEA (Joyee Cheung) #61813 sqlite: * mark as release candidate (Matteo Collina) #61262 stream: * (SEMVER-MINOR) rename `Duplex.toWeb()` type option to `readableType` (René) #61632 test_runner: * (SEMVER-MINOR) show interrupted test on SIGINT (Matteo Collina) #61676 PR-URL: #61922
Notable changes: http2: * (SEMVER-MINOR) add http1Options for HTTP/1 fallback configuration (Amol Yadav) nodejs#61713 sea: * (SEMVER-MINOR) support ESM entry point in SEA (Joyee Cheung) nodejs#61813 sqlite: * mark as release candidate (Matteo Collina) nodejs#61262 stream: * (SEMVER-MINOR) rename `Duplex.toWeb()` type option to `readableType` (René) nodejs#61632 test_runner: * (SEMVER-MINOR) show interrupted test on SIGINT (Matteo Collina) nodejs#61676 PR-URL: nodejs#61922
Notable changes: http2: * (SEMVER-MINOR) add http1Options for HTTP/1 fallback configuration (Amol Yadav) #61713 sea: * (SEMVER-MINOR) support ESM entry point in SEA (Joyee Cheung) #61813 sqlite: * mark as release candidate (Matteo Collina) #61262 stream: * (SEMVER-MINOR) rename `Duplex.toWeb()` type option to `readableType` (René) #61632 test_runner: * (SEMVER-MINOR) show interrupted test on SIGINT (Matteo Collina) #61676 PR-URL: #61922
Mark the SQLite module as release candidate and remove the experimental warning.