Conditionally promisify all model functions

[mjsalinger]

The current library does not run well with node-style callback functions, which require promisification. Bluebird's native promisify will only promisify node-style callback functions, but will break existing promises.

• Added promisify-any and invoked it on all model functions. This library promisifies node-style callback functions, synchronous functions, generator functions, and keeps Promises intact.
• Added tests for ensuring that raw callbacks work
• Added saveToken to its own .then.

[maxtruxa]

This should be return Promise.try(promisify(this.model.getClient, 2), [clientId, null]), as the signature of getClient is (clientId, clientSecret[, callback]). Currently promisify's callback is passed as clientSecret.

model.getClient = function getClient(clientId, clientSecret, callback) {
    let client = /* get client using clientId */;
    if (clientSecret) {
        // oops..
    }
    // ...
}

This could be prevented in the model by performing a check like typeof clientSecret === 'function' but callback support should really be optional. If I decide to use promises (or throw) I shouldn't have to be aware of this edge case.

[maxtruxa]

This problem persists.

[mjsalinger]

Good point, will fix.

[nunofgs]

@mjsalinger can you provide a concrete example of how this is not working with promises and callbacks?

Closing for now, but I'll surely give it a second look if something is broken.

[mjsalinger]

This is critical for my app. Reopening. Will update with an example shortly.

[mjsalinger]

@nunofgs For example, prior to this pull request, if I create a model function as follows:

model.validateScope(user, client, scope callback) { 
  // do some work
  callback(null, scope);
}

and the code in the library before this PR is

return Promise.try(this.model.validateScope, [user, client, scope]);

it would not work. Because Promise.try in the example above is expecting the function this.model.validateScope to return a Promise, and the model uses a callback, bluebird won't process it and the then will never execute.

So to make this model function work with the Promise chain, it needs to be Promisified, becoming

return Promise.try(promisify(this.model.validateScope, 3), [user, client, scope])

[maxtruxa]

This PR still breaks getClient on models using promises (see this comment).

[maxtruxa]

Previously model#validateScope was a required model function. (Promise.try throws a TypeError if the first argument isn't a function. ) This change makes validateScope optional.
If this is the desired effect there should probably be an else accepting any scope. Something like this should work:

} else {
  return scope;
}

Without this addition all scopes are lost, resulting in a call to model#saveToken with token.scope === undefined (see for example ClientCredentialsGrantType#saveToken).

[maxtruxa]

Adding additional thens and Promise.try is unnecessary.

Simply replacing the old

return this.model.saveToken(token, client, user);

with

return promisify(this.model.saveToken, 3)(token, client, user)

should be fine.

Promise.try should just be used to start a new promise chain, because it catches synchronous exceptions and rejects the returned promise. Down here we're already inside a promise handler, so synchronous exceptions are caught and passed down the chain anyways.

[maxtruxa]

Adding additional thens and Promise.try is unnecessary. See previous comments.

[maxtruxa]

Adding additional thens and Promise.try is unnecessary. See previous comments.

[maxtruxa]

Adding additional thens and Promise.try is unnecessary. See previous comments.

[maxtruxa]

Changing getClient to check that the callback is always passed as third argument would catch the problem described in this comment.

getClient: function(clientId, clientSecret, callback) {
  should(clientSecret).equal(null);
  callback(null, { grants: ['authorization_code'], redirectUris: ['http://example.com/cb'] });
}

[maxtruxa]

I was reading the documentation of promisify-any again and noticed two things:

• When using promisify-any with a non-native promise library, it's cleaner to tell promisify-any to actually use it. Without the call to use() promisify-any defaults to using it's own copy of bluebird (currently bluebird@2.10.0).

var Promise = require('bluebird');
var promisify = require('promisify-any').use(Promise);

This shouldn't be a huge issue since bluebird accepts any thenable, but this brings me to the second point.

• promisify() already catches synchronous exceptions, which makes the use of bluebird's Promise.try redundant.

function fn() {
  throw new Error('test');
}

Using promisify-any:

promisify(fn)().catch(console.log.bind(console));

Using bluebird:

Promise.try(fn).catch(console.log.bind(console));

Using promisify-any without bluebird's Promise.try comes with the added benefit that we don't rely on passing arguments to Promise.try, which is deprecated as of bluebird@3.0.0.

So

return Promise.try(this.model.getUserFromClient, client);

becomes

return promisify(this.model.getUserFromClient, 1)(client);

instead of

return Promise.try(promisify(this.model.getUserFromClient, 1), client);

[mjsalinger]

I like this approach, I'll make the necessary changes.

[mjsalinger]

Added fixes, @maxtruxa.

[maxtruxa]

Looking great. I don't see any problems with merging this.

[mjsalinger]

Thanks!