OPENSRC-105 Make sure all the promises will reject in case of internal throw

[orlandoc01]

Just wanted to offer an alternative strategy for this PR that may be worth considering. @imolfar and I had discussed how using Promise.promisify may be used as a means of promisifying some of the Registry callback functions but opted against it because it looked like we would have to change a lot of the methods that were constructed and we wanted to keep the code styling coherent. However, this PR looks like you're doing that already so it may be worthwhile to reconsider this tactic. I'll briefly describe an example implementation and the reasoning behind it and leave it up you guys whether or not to adopt this strategy.

The issue with the way Promises are currently constructed in this library is that it is an anti-pattern because the registry method callback API automatically support Promisification out-of-the-box, so doing it manually using a Promise constructor with resolve and reject leads to more code and is more likely to lead to uncaught errors. (See http://bluebirdjs.com/docs/anti-patterns.html example 2 for more info). So, as an alternative, you could create a function which wraps the function in a Promise.promisify call and then immediately invokes it and use this function instead of the catcher function you implemented above.

For example, say you have a helper function like: const P = (context, func, ...args) => Promise.promisify(func.bind(context, ...args))();

You could then take code like this:

Consumer.prototype.getRegistrant = function(address) {
    var self = this;
    return catcher(function(fulfill, reject) {
      self.registrar.registrantIndex.call(address, {from: self.address}, function(err, index) {
        if (err) {
         console.error(err);
         reject(err);
         return;
       }
       fulfill(self.getRegistrantbyIndex(index));
     });
   });
 };

and turn it into something like this:

Consumer.prototype.getRegistrant = function(address) {
  return P(this, this.registrar.registrantIndex.call, address, {from: this.address})
  .then(index => this.getRegistrantbyIndex(index));
};

Like I said though, just a suggestion for an alternative way of handling this issue. I wouldn't suggest it if it was only one function that needed this wrapping but since it looks like you're changing most of them, it may be worthwhile to then just invest the time and promisify them now instead. I've tested this code locally on some of the methods just out of curiosity and it works fine. If you opted for this, you would then just add that helper function or something like that and replace each catcher call with that helper function instead and get rid of the callbacks.

[lastperson]

@orlandoc01 @imolfar please review new diff.

[lastperson]

I agree, though refactoring can be done over and over again and it is out of scope of this task. You can commit to this branch too ;).

[orlandoc01]

Great job addressing this anti-pattern using Promisify.all. It's a pretty clean solution and even better then the one I originally suggested in my post. Nice work! :+1:. Just made a few minor styling suggestions that deal with new features in ES6 that could save us even a few more lines of code and make it more consise and understandable. This mainly deals with taking advantage of the new lexical this binding in arrow functions. Feel free to ignore these style suggestions if you prefer, but there is one issue that should be addressed with the misspelling of lenght in lib/consumer.js on line 159. Otherwise, you're good to go!

[lastperson]

@orlandoc01

one issue that should be addressed with the misspelling of lenght in lib/consumer.js on line 159

It should be fixed inside the Registry contract first, please create separate task.