sqlite: enable common flags#57621
Conversation
|
Review requested:
|
|
Not sure whether this will be a problem, but this increases the binary size by 600KB. |
e4b93b9 to
9590dee
Compare
cjihrig
left a comment
There was a problem hiding this comment.
Not saying whether or not we should land this, but https://github.com/nodejs/node/blob/main/deps/sqlite/unofficial.gni should be updated, and there should be at least one test for each new API to prevent regressions.
Good point. Also, I think it would be good to have some benchmarks on sqlite implementations. |
9590dee to
0c1ec86
Compare
@cjihrig , what's the difference between .gyp and .gni file? |
0c1ec86 to
31c5aa5
Compare
They are for two different build systems. The .gyp file is used by the official build. The .gni file is used by the unofficial GN build. |
Thank you! |
Why not? And why not the Geopoly extension? |
For |
On the other hand, the issue mentions the SQLite Amalgamation. I think we could have RBU enabled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57621 +/- ##
==========================================
+ Coverage 90.22% 90.23% +0.01%
==========================================
Files 630 630
Lines 185054 185055 +1
Branches 36254 36219 -35
==========================================
+ Hits 166963 166985 +22
- Misses 11032 11037 +5
+ Partials 7059 7033 -26 🚀 New features to boost your workflow:
|
31c5aa5 to
3f64dfa
Compare
cjihrig
left a comment
There was a problem hiding this comment.
One comment, but changes LGTM.
Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
| ); | ||
| }); | ||
|
|
||
| test('fts3 parenthesis', (t) => { |
There was a problem hiding this comment.
This is inconsistent with the rest, missing is enabled in its name.
cjihrig
left a comment
There was a problem hiding this comment.
Still unsure whether or not we want all of these enabled, but giving this a LGTM so it's not stalled.
I really hope they land. Not having them enabled limits the usefulness of the library a lot. |
|
I personally don't care about the other ones but I'm really hoping FTS5 specifically lands. |
This is why, for all the various use cases various projects have, they should all be included. 😊 |
|
Just noting that this is ready to land. It just needs one person who believes in this change strongly enough to put the |
|
Let's do this! |
|
Landed in 9aa57bf |
PR-URL: nodejs#57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: #57621 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This PR enables flags that are common to other sqlite players in Node.js ecosystem:
This is related to #56476, even though it does not enable the RBU extension.
I see this as a good step toward the stabilization since it will make it easier for people from other dependencies.