Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to Promise-based async implementation #1776

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

smoores-dev
Copy link

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
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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
Copy link

huco95 commented May 27, 2024

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

@daniellockyer
Copy link
Member

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

@felixnogal
Copy link

Hi @daniellockyer, any updates related to this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

streaming support / generator functions
4 participants