WiseLibs / better-sqlite3

The fastest and simplest library for SQLite3 in Node.js.
MIT License
5.23k stars 390 forks source link

Support sql`` JS template strings that create cached prepared statements #1157

Closed benbucksch closed 3 months ago

benbucksch commented 3 months ago

However, it's possible to combine all the advantages, by letting the SQL template string function (JS template strings are just normal JS functions and can do anything they want) create the prepared statement first, cache it internally, and then use the cached prepared statement to do the variable replacement.

SQL template strings that create cached prepared statements gives

There are implementations of that idea. @leafac/sqlite/, now replaced by radically-straightforward, implements exactly that. Demo, direct link to the relevant time (13:10): https://youtu.be/3PCpXOPcVlM?t=793 However, I am uncertain about the stability of this project, given the short notice deprecation, complete lack of documentation, etc.

This is a request to add this feature directly to better-sqlite3 .

mceachen commented 3 months ago

I'm -1.

@JoshuaWise has done a great job in keeping this library focused and simple, and yet there are still a ton of unresolved issues on this project.

This would be a nice feature, but string templating, by design, doesn't have the same SQL-aware expressiveness, composability, and vendor-specific syntax translation as something like knex. In other words, not all users of this library will want to (or need to) switch to this new API.

If this feature could not be added as an additional layer, but had to be incorporated in this library, I might change my mind, but IMHO, what @leafac has done with a layer on top of this library is the correct way to do it. Whether that code is maintained or documented sufficiently is a wholly different matter.

benbucksch commented 3 months ago

not all users of this library will want to (or need to) switch to this new API.

That's fine. You're not required to use it.

But many dev like me absolutely need it. A library that doesn't offer this is a no-go from the get-go for me. Just because you don't need it, or it could theoretically be done elsewhere, is not a good reason to reject the feature. For me, this is an essential, fundamental feature of any SQL API.

what @leafac has done with a layer on top of this library is the correct way to do it.

As you can see, that library is no longer maintained. If I base my app on it, I need to have confidence that the lib and feature is going to be there in the future.

Also, there's the issue of discoverability. If I find better-sqlite3, I won't know that there is a better way than the "better sqlite3" (forgive the pun, but that's my point).

Prinzhorn commented 3 months ago

However, I am uncertain about the stability of this project, given the short notice deprecation, complete lack of documentation, etc. As you can see, that library is no longer maintained. If I base my app on it, I need to have confidence that the lib and feature is going to be there in the future.

Instead you'd put the burden of maintaining it on better-sqlite3?

But many dev like me absolutely need it.

Then maintain it?

I don't mean to sound rude, but better-sqlite3 has essentially been in maintenance mode for quite some time. I don't remember the last real bug report and most activity is around support for newer Node.js/Electron versions.

This is not about "dev like me absolutely need it" or "discoverability" or "this is a no-go" it's simply that there is no capacity here to implement and support this feature. And since it can easily be implemented in user-land (and better-sqlite3 was always meant as a very low level wrapper anyway) then I recommend doing that.

We'd rather have low level things like N-API or being able to cooperatively kill a worker mid transaction then a feature that can be implemented outside of better-sqlite3. Because these are at the core of it.

benbucksch commented 3 months ago

there is no capacity here to implement and support this feature

That's fair enough. Thanks for letting me know.

leafac commented 3 months ago

The library that adds tagged template support to better-sqlite3 is here: https://github.com/radically-straightforward/radically-straightforward/tree/main/sqlite

It used be called @leafac/sqlite and now it’s called @radically-straightforward/sqlite because it’s becoming part of a set of tools that I’m developing called Radically Straightforward. I’m still working on some parts of Radically Straightforward, but the SQLite part is ready for use and documented.

I hope you have a good time using it.