TryGhost
Migrate to Promise-based async implementation
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!
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!
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!
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?
Sorry! I'm interested but currently struggling to find time to check it properly and review. Hope to find time soon 🙏🏻
