Switch to bluebird

[myndzi]

I thought I might have submitted this same conversion some long time ago, but I didn't seem to see the pull request or any reason for its rejection, so here goes, if you want it.

When is pretty decent these days, but Bluebird is still noticeably better on the memory utilization front, which can matter quite a bit if you're doing a lot of messaging. It also provides a slightly more useful API. Looking at When's current docs, I can see that they aren't that different in functionality other than naming, but Bluebird still wins in convenience.

Anyway, I had some code that was using a super old amqplib that I had patched for Bluebird, back when it mattered to me more, and I wanted to update it to a recent copy of amqplib. That left me with either changing my code or changing my fork, and I went with the latter, since it turned out to be easier. (Some of the bluebird promise methods didn't port readily to when, though the inverse is usually true).

Also got rid of the deferred "anti-pattern" while I made the conversion, except in the tests where it's 1) used a lot and 2) really doesn't matter as long as it gets the job done.

It should be noted that using Bluebird doesn't really do all that much for the amqplib codebase itself, since it hardly uses promises much, but it DOES provide useful tools for consumers of the library... though there are some incompatibilities, too, so this might require some sort of shim to not be a breaking change.

[squaremo]

Hi Kris, thank you for putting all this effort in (again!). I like the success/failure continuation style of bluebird's Promise constructor, and I understand it's quicker than when. Is it also more stable?

Introducing incompatibility with existing code is a bit of a stop light. Is there a page that details what the differences are, so we could fix them or at least provide advice?

[myndzi]

I think I goofed up those commits when I tried to correct the single file thing, crap. I'll fix at first opportunity. I only wanted the new commits and had picked them out.

re: details on differences, not really anything I can think of other than comparing the API docs. For example, bluebird provides ".return" while when provides ".yield"

[myndzi]

Yes, Bluebird has been very stable and the developer is a performance beast. I've never looked back :)

[squaremo]

re: details on differences, not really anything I can think of other than comparing the API docs.

Ah, so do you mean the incompatibility will be for those people that call a when-specific method on a returned promise? It would be a shame to break such things, although doing that is relying on an implementation detail in a way isn't it.

[myndzi]

This is true, but in practice, it's kind of what makes it useful (besides performance characteristics) to select a useful promise library. Tricky gray area...

[uschen]

@myndzi https://github.com/CapitalGene/amqp.node

[myndzi]

Fork !== pull request, but he's certainly welcome to merge yours :)

[OlivierCuyp]

👍 For bluebird, if it create a compatibility issue, just change the major version.
Bluebird is the fastest promise library available with the nicest features.

[uschen]

@myndzi I might had merged your previous ones into that fork. the coding style is very different than @squaremo 's , so it won't be easy to merge.

[knpwrs]

I was actually just using this and was going to suggest using Bluebird to avoid the callback_api module. For example, the connect function as modified by this PR can be updated to the following:

function connect(url, connOptions, callback) {
  if (typeof connOptions === 'function') {
    callback = connOptions;
    connOptions = null
  }
  return new Promise(function (resolve, reject) {
    raw_connect(url, connOptions, function(err, conn) {
      if (err === null) resolve(new ChannelModel(conn));
      else reject(err);
    });
  }).nodeify(callback);
};

Then all of the following are valid invocations:

// Promise, no options
connect(url).then(function (res) {
  //
}).catch(function (err) {
  //
});

// Promise, options
connect(url, ops).then(function (res) {
  //
}).catch(function (err) {
  //
});

// Callback, no options
connect(url, function (err, res) {
  //
});

// Callback, options
connect(url, ops, function (err, res) {
  //
});

.nodeify returns a Promise, simply attaching its argument as appropriate callbacks to .then and .catch unless the argument is falsy in which case .nodeify is basically a noop. This means all clients can just require the module and use it however they want without having to explicitly require a specific API.

Args.js can be used to assist with argument overloading / shifting if you don't want to do it manually.

[analytik]

+1 for Bluebird.

Is there anything left to do with this PR, besides merging conflicts and fixing the tests? Can I help somehow?

[nfantone]

Same over here. This is really nice. Offering help if needed.

CI is failing due to when still being used in tests. Maybe declare it as a devDependency?

[squaremo]

Closing this, since #295 has been merged. Thanks for everyone's comments here, and sorry it took a couple of go-rounds :-}