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

Adding async operation support. #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martinheidegger
Copy link
Contributor

This PR adds the option to pass-in open, destroy, read, write, writev, and final as async function instead of a callback.

@martinheidegger martinheidegger force-pushed the async-options branch 5 times, most recently from f28abf7 to e7b48f2 Compare November 26, 2020 12:54
@phated
Copy link

phated commented Dec 7, 2020

This gets really tricky if someone makes a function that takes a callback and returns a promise. We've struggled with this same issue in async-done and I don't think the complexity of the API is worth it. It almost seems like it should just be a wrapper of some sort.

@mafintosh
Copy link
Owner

I’m leaning towards just making a major that impls the internal _read/write etc with promises instead of cbs. Luckily we don’t have any public apis with cbs exposes on the stream itself so interop is trivial

@phated
Copy link

phated commented Dec 7, 2020

@mafintosh have you done any profiling on creating so many intermediate promises? I've not, so I'm just curious.

@martinheidegger
Copy link
Contributor Author

This gets really tricky if someone makes a function that takes a callback and returns a promise.

It needs to be mutually exclusive. In the implementation as-is I relied on the skill of the programmer (either pass-in a regular fn or a fn returning a promise). I see that this might be a bit naive, so how about adding a guard for regular function to exit with an error or show a warning if a regular function returns a promise (wrong API use).

@martinheidegger
Copy link
Contributor Author

martinheidegger commented Jan 11, 2022

Rebased to match recent changes. In order to address the recent comment: if an async function is detected but no promise is returned, an error will be thrown.

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.

3 participants