search

TryGhost / node-sqlite3

SQLite3 bindings for Node.js
BSD 3-Clause "New" or "Revised" License
6.23k stars 817 forks source link

Migrate to Promise-based async implementation #1776

Open smoores-dev opened 7 months ago

smoores-dev commented 7 months ago

Fixes #1715 and #834

Hi there!

Hopefully this is a welcome request. I've seen @daniellockyer mention a few times that you all would be open to a pull request that moves this library to using a Promise-based async implementation. I'm personally interested in using this library for Storyteller, so I spent some time this past week working on just that.

What does this PR do?

This PR replaces the callback property on the Baton structs with a deferred property, of type Napi::Promise::Deferred. Everywhere that was previously calling the callback now either rejects or resolves the deferred instead.

The one exception to this is Statement::Each, which now returns an AsyncIterable. This allows consumers to use the for await (...) construct to loop through result rows.

Constructors have been made "private" (in Typescript, at least, and this should be changed in the docs as well). Javascript constructors can't return Promises/be async, so instead, the async initialization code for Database, Statement, and Backup has been split out into their own methods, and new create static methods have been added that construct the object and then asynchronously initialize it.

All of the tests have been updated and are passing. The only one that I could use some explanation on is the async_calls test; I'm not sure what it was meant to be testing before!

My C++ is rusty at best; I did my best to follow best practices and the patterns laid out in this library, but I may have stumbled. I would absolutely appreciate any and all feedback on my C++ code here!

smoores-dev commented 7 months ago

It looks like node-addon-api somewhat recently added an engines spec to their package.json, and it sets a minimum of ^16, so I had to bump the semver check script to 16. We should probably add our own engines spec to this library, given that (and probably even update that semver check script to use that, rather than a hard-coded value). This isn't an actual change in the node versions that this library supports, just making the script more accurately reflect requirements that already existed!

smoores-dev commented 7 months ago

Howdy! Just checking in: it looks like the GHA workflow needs to be approved before it can be run. Is this PR something that you all are interested in, generally? No rush; I'm happy to be able to use my own fork while we work on getting it merged, but I wanted to make sure that I'm not pestering you all if you don't want to go in this direction!

smoores-dev commented 6 months ago

Hello! @daniellockyer, is this still something that you think you might be interested in? Would it make this change more digestible if it maintained an optional callback interface, in addition to the new Promise one?

huco95 commented 5 months ago

This would be fantastic to have! Any plans to merge this?

daniellockyer commented 5 months ago

Sorry! I'm interested but currently struggling to find time to check it properly and review. Hope to find time soon 🙏🏻

felixnogal commented 4 months ago

Hi @daniellockyer, any updates related to this PR?