callbackify or promise-nodeify

[achingbrain]

🚨 Bikeshed warning 🚨

In #2390 I reached for callbackify when converting the internals to promises (only partially because the name has a nice symmetry with promisify), but missed that we already had promise-nodeify in the codebase.

It seems silly to have both when they do similar things. Ordinarily I'd just swap out callbackify but I'm not a great fan of how promise-nodeify makes you add boilerplate to handle setting up the promise yourself (I think, I could be using it wrong) and prefer how callbackify lets you write an async function and forget callbacks are even a thing.

The caveat is that your callbackified function must return a promise - if you return a value instead everything explodes which leads to a few functions marked async superfluously, but I don't think this is a big deal as we've sort of agreed to mark promise returning functions async even if they just chain through to a different function that actually does some async work.

Given:

async function returnsPromise (options) {
  // do some async stuff
}

callbackify

const func = callbackify.variadic(returnsPromise)

promise-nodeify

const func = (options, callback) => {
  if (typeof options === 'function') {
    callback = options
    options = {}
  }

  return promiseNodeify(returnsPromise(options), callback)
}

@hugomrdias you started to have an objection to callbackify in the review of #2390 - could you explain a bit here?

[daviddias]

I believed that the goal with #1670 is to stop having a callbacks version of the API all together. We can then have an example that shows how to use one of those modules for users that still want callbacks, but we won't support duo API

[achingbrain]

+1 that, can't wait to get there, this is more about what to do in the interim.

[daviddias]

this is more about what to do in the interim.

Land #1670 🤯 🎊 😅

[hugomrdias]

mainly because we already started using it in other places, the boilerplate in most places was there already and i personally like it more because it forces us to be explicit about what happens. With callbackify we need to be careful about using variadic or not, promise-nodeify handles the caveat you talked about plus others https://github.com/kevinoid/promise-nodeify#features.

having said that i think we shouldn't spend time on this, hopefully it will not cause problems and we should focus on removing the callbacks all together.

[alanshaw]

having said that i think we shouldn't spend time on this, hopefully it will not cause problems and we should focus on removing the callbacks all together.

+1, they are both small and will only be in the bundle temporarily until #1670 lands. Lets close this and not tempt others to join in on the bikeshedding!