nodejs / CTC

Node.js Core Technical Committee & Collaborators
80 stars 27 forks source link

`util.callbackify` for async functions #109

Closed benjamingr closed 7 years ago

benjamingr commented 7 years ago

Hey,

From https://github.com/nodejs/promises/issues/6 originally, but seeing that util.promisify is finally in sight, I think this might be worth proposing too.

I propose that for compatibility reasons, and so that callback users using Node can keep working with their tooling to add a util.callbackify function that converts an async function to a callback taking function.

For example:

async function foo() {
  let data1 = await promiseApi(..);
  let data2 = await promiseApiOther(data1);
  return result;
}
const foo2 = util.callbackify(foo);
foo2(function(err, data) {
  // error here if foo threw
  // use data here
});

This is likely faster in core, and would enable seamless integration of async functions into callback APIs and code bases. Seeing as we're not going to make a promisified core any time soon - this is beneficial for interoping between code bases using callbacks and async functions together.

How do @nodejs/collaborators feel about this?

mcollina commented 7 years ago

I think we need this if we want to ship promisify.

Qard commented 7 years ago

:+1: from me. Regardless of how core feels about promises and async/await, userland will use them, so we need to ensure interop works well.

cjihrig commented 7 years ago

I think it makes sense if promisify makes it in.

vkurchatkin commented 7 years ago

-1 unless we have something to offer in terms of performance

bnoordhuis commented 7 years ago

Someone has to say it: s/toCallback/callbackify/

evanlucas commented 7 years ago

Someone has to say it: s/toCallback/callbackify/

Yea, can we be consistent?

benjamingr commented 7 years ago

@bnoordhuis @evanlucas this issue was mostly to see if there is interest - I've amended to callbackify.

refack commented 7 years ago

At the moment I see 20 ๐Ÿ‘ so ๐Ÿ˜€me imagegauntlet ๐Ÿ‹๏ธโ€Picking up

(ouch! my back ๐Ÿ˜–)

refack commented 7 years ago

@not-an-aardvark I'm working on a PR, finishing some tests and will upload tomorrow for you'll to review...

eljefedelrodeodeljefe commented 7 years ago

I'd be happy if we could push https://github.com/nodejs/node/pull/12712 forward.

refack commented 7 years ago

I believe this could be closed now that https://github.com/nodejs/node/pull/12712/commits/af3aa682ac534bb55765f5fef2755a88e5ff2580 landed

benjamingr commented 7 years ago

Sweet! Thanks a lot @refack :)