6

I have this code that is part of a small API that I am writing for an NPM module called Poolio. The question I have seems to be a common question for those supporting error-first callbacks as well as promises- how do we support both while maintaining consisent APIs and consistent return values from the API? For example, if I conditionally return a promise from my API, depending on whether the consumer of my lib provides a callback, that is a little awkward in my opinion.

The consumer of the lib can provide a callback or use the Promise then function, but not both.

Here is a function exported by my lib, that I would like to promisify:

Pool.prototype.any = function (msg, cb) {

    var workId = this.counter++;
    var self = this;

    return new Promise(function (resolve, reject) {

        if (typeof cb === 'function') {
            self.resolutions.push({
                workId: workId,
                cb: cb
            });
        }
        else {
            self.resolutions.push({
                workId: workId,
                resolve: resolve,
                reject: reject
            });
        }

        if (this.available.length > 0) {
            var cp = this.available.shift();
            cp.workId = workId;
            cp.send(msg);
        }
        else {
            self.msgQueue.push({
                workId: workId,
                msg: msg
            });
        }
    });

};

my question is - if the user provides a callback function in the original function arguments, how can I resolve the promise without calling 'then'? Sorry it's hard to explain but hopefully you can understand.

also there is this interesting question: Do never resolved promises cause memory leak?

Alexander Mills
  • 90,741
  • 139
  • 482
  • 817
  • 3
    resolving the promise has nothing to do with `then` ... a promise can be resolved independent of any `then` callbacks being invoked on it – Jaromanda X Jan 26 '16 at 07:22
  • 1
    *if* there is a then to be called on the promise, then calling resolve from inside the promise should call all attached then's, right? In other words, if there are no thens, then calling resolve should have no effect, but if there are thens, then those thens should be called. My question is how to avoid calling them. – Alexander Mills Jan 26 '16 at 07:23
  • yes, also any then's "attached" after resolution will be called "immediately" (in quotes because it's not that clear cut) – Jaromanda X Jan 26 '16 at 07:24
  • `My question is how to avoid calling them` - ahhh, I see now! Sorry, totally missed the point of your question – Jaromanda X Jan 26 '16 at 07:25
  • if the user provides a callback function, then you are not going to "push" resolve/reject functions into resolutions, so you'll never be able to call resolve anyway – Jaromanda X Jan 26 '16 at 07:29
  • If you want to only return a Promise if no callback is supplied, you could change your code to [this](https://jsfiddle.net/a1xk7s5e/) – Jaromanda X Jan 26 '16 at 07:37
  • thanks @JaromandaX that code looks good. I don't like the fact that there are two different return types, but I guess I can live with it. I think it's somehow better to always return a promise, but maybe not. Thanks for your idea, it should work fine. – Alexander Mills Jan 26 '16 at 07:39
  • I think your original code works fine too ... even if someone called .any with a callback and "used" the returned promise, the returned promise would never be able to be resolved with your code anyway (because the callback is present, the resolve/reject functions are never "saved" so how can they be called!) – Jaromanda X Jan 26 '16 at 07:41
  • yeah, it seems that Promise will get GC'ed even they aren't resolved. It looks like @slebetman has an answer, but I don't think it will work for my scenario. – Alexander Mills Jan 26 '16 at 07:57

3 Answers3

2

It's actually very straightforward. Only you may have missed it because it's hidden amongst that tangle of code.

Basically you do this:

var promise = new Promise(function (resolve, reject) { /*....*/});

if (typeof cb === 'function') {
    promise.then(cb);
} else {
    return promise;
}
slebetman
  • 109,858
  • 19
  • 140
  • 171
  • hmmm this is an error-first callback, if there is an error, will it appear in the right place? – Alexander Mills Jan 26 '16 at 07:58
  • 1
    that totally disregards whatever `self.resolutions.` does (not that I understand what that's all about) – Jaromanda X Jan 26 '16 at 08:01
  • yeah, this won't work for my situation and I am not sure if it's correct either. If you are curious, this is the library in question: https://github.com/ORESoftware/poolio – Alexander Mills Jan 26 '16 at 08:04
  • here is the original without promises: https://github.com/ORESoftware/poolio/blob/master/index.js – Alexander Mills Jan 26 '16 at 08:04
  • here is the new one with promises: https://github.com/ORESoftware/poolio/blob/master/promise.js – Alexander Mills Jan 26 '16 at 08:05
  • @AlexMills: The above is obviously extremely simplified for clarity. But the logic is correct. You of course need to handle all the corner cases as well like massaging the return value to the second argument of cb etc which can easily be done by wrapping it in another anonymous function. I thought it would be obvious to anyone who knows js. If it's not clear I can expand on the example – slebetman Jan 26 '16 at 08:17
  • @AlexMills: The key lesson I'm trying to teach is that promises are nothing special. They're just a kind of callback, just like regular callbacks. – slebetman Jan 26 '16 at 08:17
  • I think it's a `cb(err, data)` function so doing a `then(cb)` won't work, the cb will receive the data as an error. – Shanoor Jan 26 '16 at 08:31
  • @ShanShan: Read my comment above. Of course you should wrap it up in a function to pass the values correctly. I just want to illustrate the correct control-flow structure. I'll leave details for homework. – slebetman Jan 26 '16 at 08:36
1

Actually, it's a pretty common thing APIs do (mongodb-driver example). Basically, write a private function accepting a callback, write a public function checking for cb and writing it if necessary. Using the code from your github (_any might need a refactoring, you don't need to check if cb is a function for example and maybe other things too):

 // private function
var _any = function(msg, cb) {
  if (this.kill) {
    console.log('warning: pool.any called on pool of dead/dying workers');
    return;
  }

  debug('current available pool size for pool_id ' + this.pool_id + ' is: ' + this.available.length);
  var workId = this.counter++;

  if (typeof cb === 'function') {
    this.resolutions.push({
      workId: workId,
      cb: cb
    });
  } else {
    workId = -1;
  }

  if (this.available.length > 0) {
    var cp = this.available.shift();
    cp.workId = workId;
    cp.send(msg);
  } else {
    this.msgQueue.push({
      workId: workId,
      msg: msg
    });
  }
};

 // public exposed function
Pool.prototype.any = function(msg, cb) {
  if (typeof cb === 'function') {
    // cb is provided, no action is required here
    return _any(msg, cb);
  } 

  // no cb, wrap the call inside a Promise and provide a cb
  return new Promise(function(resolve, reject) {
    _any(msg, function(err, data) {
      if (err) reject(err);
      else resolve(data);
    });
  });
}
Shanoor
  • 13,344
  • 2
  • 29
  • 40
  • You're not returning a promise if there is no callback so calling `pool.any().then()` won't work. – slebetman Jan 26 '16 at 08:38
  • @slebetman `return new Promise()` seems to be returning a promise to me. OP can check if `msg` is present inside `_any()`, I'm outlining the idea not writing the whole code with full error checking. – Shanoor Jan 26 '16 at 08:54
0

I think you are trying to do something like this:

function Pool() {
  this.counter = 0
  this.resolutions = []
  this.available = []
  this.msgQueue = []
}

Pool.prototype.any = function(msg, cb) {
  const self = this;

  const regCB = (msg, resolve, reject) => {
    const workId = self.counter++;
    self.resolutions.push({
      workId,
      resolve,
      reject,
    })

    if (self.available.length > 0) {
      const cp = self.available.shift();
      cp.workId = workId;
      cp.send(msg);
    } else {
      self.msgQueue.push({
        workId,
        msg,
      });
    }
  }

  const promise = new Promise((resolve, reject) => {
    if (typeof cb === 'function') {
      resolve = data => cb(null, data)
      reject = err => cb(err, null)
    }

    regCB(msg, resolve, reject)
    resolve(msg)
  })


  return promise
}

const fnc = (err, data) => {
  console.log('fnc', {
    data
  })
}

const promise = Promise.resolve(3)
const pool = new Pool
pool.any('with_function', fnc)

const poolPromise = pool.any('with_promise')
poolPromise.then((data) => {
  console.log('poolPromise', {
    data
  })
})
Teocci
  • 7,189
  • 1
  • 50
  • 48
  • `resolve = cb` seems pointless when you're already doing `promise.then(cb)` later – Bergi Aug 11 '22 at 02:50
  • `pool.any('with_promise', promise)` looks wrong, the `promise` is not used for anything. It should rather be just `pool.any('returns_promise')` – Bergi Aug 11 '22 at 02:50