7

Suppose I created or have a node.js library lib.js

export class C {
    constructor(value, callback) {
        callback(false, `Hello ${value}`);
    }

    task(value, callback) {
        callback(false, "returned " + value);
    }
}

The important part is that the classes' constructor needs to accept a callback as it does database connections and file I/O. If I now import and use the library callback-style, everything is fine (see c1 below).

I would really like to promisify the library where I use it to make object construction more convenient (in reality it's a whole bunch of classes and methods).

However, I can't find a way to new the class properly in a promise-safe nicely. 

import Promise from 'bluebird';
import * as lib from './lib';


Promise.promisifyAll(lib);


// old style -- works as expected
const c1 = new lib.C("c1", (e, v) => {
    console.log(c1, e, v); 
});


// assuming c1 got initialized, .task() also works
c1.task("t1", console.log);
c1.taskAsync("t2").then(() => console.log("also works"));


// But how to do this properly with promises?
const c2 = new lib.C("c2"); c2.then(console.log); // clearly doesn't work, lack of callback
const c3 = new lib.CAsync("c3"); c3.then(console.log); // "cannot read property apply of undefined"
const c4 = ???

How would I do this best? Changing library signature isn't a good option, creating factory methods also seems to be ugly.

Bergi
  • 630,263
  • 148
  • 957
  • 1,375
left4bread
  • 1,514
  • 2
  • 15
  • 25
  • Don't do IO in constructors it's a bad idea to bind io and construction together. – Benjamin Gruenbaum Jul 13 '15 at 19:50
  • Style-wise you're probably right. But isn't that what the node Redis libraries and many other libs do as well (implicitly)? Also, it's not that I'm blocking or anything, merely triggering actions. – left4bread Jul 13 '15 at 19:53
  • 1
    @left4bread Doing IO (even just establishing connections) in your constructor tends to lead you to initializer loops in short order. Just splitting the IO work into a method (`r = new Database(); r.openPool();`) can make your code much simpler and more testable. – ssube Jul 13 '15 at 19:56
  • 2
    See also [Is it bad practice to have a constructor function return a Promise?](http://stackoverflow.com/q/24398699/1048572) – Bergi Jul 13 '15 at 19:58

2 Answers2

6

I feel strongly about this so I'll start with it: Don't do IO in constructors it's a bad idea to bind io and construction together.

That said, if you must do this because the library is out of your control and are OK with losing the ability to build objects in a sync way, you can:

export class C {
    constructor(value, callback) {
        callback(false, `Hello ${value}`);
    }

    task(value, callback) {
        callback(false, "returned " + value);
    }
}

And when promisifying:

import Promise from 'bluebird';
import * as lib from './lib';


Promise.promisifyAll(lib);

var old = lib.C; // reference the constructor
lib.C = function(value){ // override it
  o; // object we'll later return, populate in promise constructor
  var p = new Promise(function(resolve, reject){ 
    // the promise constructor is always sync, so the following works
    o = new old(value, function(err, data) {
      if(err) return reject(err);
      resolve(data);   
    });
  }); 
  // THIS IS THE IMPORTANT PART
  o.then = p.then.bind(p); // make the object a thenable, 
  return o
};

Which would let you both use the return value and the promise, the promise will only have a then so you might want to Promise.resolve it to get a "real" promise rather than an object with properties and a promise.

var o = new lib.C(); // get object
o.then(function(data){
    // access data
});

This can be extracted to a pattern:

 function promisifyConstructor(cons){
   return function(...args) => { // new constructor function
     let o;
     let p = new Promise((resolve, reject) => {
         // delegate arguments
        o = new cons(...args, (err, data) => err ? reject(err) : resolve(data));
     });
     o.then = p.then.bind(p);
     return o;
   }
 }
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
Benjamin Gruenbaum
  • 270,886
  • 87
  • 504
  • 504
  • In this particular example the lib is under my control, however, losing sync initialization would be ugly. However, I'm accepting it since it convinced me (and Bergi's link above) that I should just add an `.init(callback)` method. – left4bread Jul 13 '15 at 20:00
  • 1
    @left4bread just to be clear: the solution here is _exactly_ what you're asking for, losing sync initialisation is a byproduct of conflating io and creation - I recommend against it, it's the exact same problem when using callbacks. – Benjamin Gruenbaum Jul 13 '15 at 20:01
  • @BenjaminGruenbaum You're missing a closing paren in the `new old` part. – neverfox Feb 06 '16 at 23:32
  • Given [this question](http://stackoverflow.com/q/37014920/1048572), it seems you've missed to `.bind(p)` the `p.then` method. – Bergi May 03 '16 at 21:56
  • Sure, I just wasn't certain that `bind` is the best or most performant way to mitigate the problem. Or maybe there's even some trick from the Bluebird internals :-) – Bergi May 05 '16 at 00:34
1

You can't directly promisify a constructor (that I'm aware of), but you can trivially work around that with a factory method:

function createC(value) {
  return new Promise(function (res, rej) {
    var c = new C(value, function (err, val) {
      if (err) {
        rej(err);
      } else {
        res(val); // or res(c) if you prefer
      }
    });
  });
}

I don't think there's a prettier way, and a well-constructed factory shouldn't be too ugly. You could generalize the factory to take any constructor of that form, but then you're approaching full DI and it may be worth finding a promise-friendly DI library.

ssube
  • 47,010
  • 7
  • 103
  • 140