Remove promisify-any dependency

[jwerre]

Just use util.promisify

[jwerre]

Just realized Bluebird is a dependency of promisify-any. Removing this should probably come before removing Bluebird.

[jankapunkt]

Also here let's check if tests already cover these cases / functionality enough in order to be on the safe side. I think coverage is very high but there are a few lines missed and you know how Mr. Murphy thinks about this :-)

[jankapunkt]

I think we can start on this after #30 has been merged

[jankapunkt]

#30 is merged, we can safely start with this one :-)

[jwerre]

I'm looking into this now and noticed some oddities I thought maybe you guys could chime in on. First off, a lot of the tests are returning the wrong value, here generateAccessToken should return a string. For example:

generateAccessToken: sinon.stub().returns({ client: {}, expiresAt: new Date(), user: {} })

should be:

generateAccessToken: sinon.stub().returns('my-access-token')

I believe these were left over from version 2 and never updated.

Second, I'm not sure util.promisify is going to work since it requires a callback function and all the of the methods I looked at don't even have a callback as an argument. We could wrap everything in a new Promise but that could get pretty messy.

You guys have any thoughts on this? How to promisify this:

AbstractGrantType.prototype.generateAccessToken = function(client, user, scope) {
  if (this.model.generateAccessToken) {
    return promisify(this.model.generateAccessToken, 3).call(this.model, client, user, scope)
      .then(function(accessToken) {
        return accessToken || tokenUtil.generateRandomToken();
      });
  }

  return tokenUtil.generateRandomToken();
};

I initinally thought we could do this but it won't work since there is no callback:

AbstractGrantType.prototype.generateAccessToken = function(client, user, scope) {
  if (this.model.generateAccessToken) {
    return require('util').promisify(this.model.generateAccessToken)(client, user, scope)
      .then(function(accessToken) {
        return accessToken || tokenUtil.generateRandomToken();
      });
  }

  return tokenUtil.generateRandomToken();
};

[jankapunkt]

Yes I ran the tests right now with util.promisify and they all fail due to what you wrote @jwerre

If the functions are simple one-dimensional we could write our own wrapper like so:

const promisify = fn => function (...args) {
  const env = this;
  return new Promise((resolve, reject) => {
    try {
      resolve(fn.call(env, ...args));
    } catch (e) {
      reject(e);
    }
  });
};

I checked a few tests and it works so far but I am not sure if this works for all.

[jwerre]

Yes, that's kind of where I ended up. We could just write our own promisify function. Let me whip up a pseudo function and see what you think.

[jankapunkt]

@jwerre did you come up with anything? Maybe you open a draft pull request so we can work out the details using the code review?

[jwerre]

@jankapunkt I did but I don't love it. I'm not sure it's better than what is there. I'm going to test it out this morning and see if it works. Will post later today.

[jwerre]

Here... I'll just post what I have so far:

module.exports = (fn, fnArgs) => {

  // I was hoping for something like this but I can't get arguments from the
  // function that is passed in.
  //
  // let lastArg = fn.prototype.arguments.length - 1;
  // let isPromise = typeof fn.prototype.arguments[lastArg] !== 'function';

  // if there is no callback in the arguments, return a promise
  if (!fnArgs.length || typeof fnArgs[fnArgs.length - 1] !== 'function') {
    return fn;
  }

  // if there is a callback warp in promise
  return (...args) => {

    return new Promise((resolve, reject) => {

      fn(...args, (err, result) => {

        if (err) {
          return reject(err);
        }

        resolve(result);

      });

    });

  };

};

So instead of:

return promisify(this.model.generateAccessToken, 3)

...it would be:

return promisify(this.model.generateAccessToken, arguments)

I was hoping to get the arguments from the pass function but I don't think is possible so we'll have to just pass them in with the function.

[jwerre]

@jankapunkt I pushed a promisify branch which you can take a look at. I didn't create a pull request because it's far from done and failing a bunch of test. The only files I really worked on are:

• lib/utils/promisify.js
• test/unit/utils/promisify_test.js
• lib/grant-types/abstract-grant-type.js
• test/unit/grant-types/abstract-grant-type_test.js

Running either of these test should work fine. Take a look at the code and tell me what you think.

[jwerre]

@jankapunkt @HappyZombies, what would you guys say to removing support for callbacks all together?

[HappyZombies]

@jwerre I am ok with that, but I believe that'll be out of scope for removing this dependency.

Going to a Promise based approach is definitely the way to go, but would require a lot of work and we need to be careful too. I think this can be something we tackle in a next major version.

We can get started on that, but I really wanna focus on getting out important fixes, administrator work NOW. If bluebird has to stay for a while then that's ok for now.

Once we get our next release out (which I am guessing will be 4.1.0), we can talk about what we want in a version 5.

If removing promisify-any is being too much of a pain, let's put a hold on it for now and get back to it later.

[jankapunkt]

I agree with @HappyZombies but I will still take a look into it. In the meantime we can further discuss release management

[jwerre]

I agree as well. When we deprecate callbacks we can remove Bluebird and promise-any in one fell swoop so I don't think there is any reason to spend any more time on this.

[jwerre]

@jankapunkt Are you going to take a look at this, or is this safe to close? #48

[jankapunkt]

@jwerre let's keep it open until we have a clear plan for #48 but once we have we can safely close this as superceded by #48

[Uzlopak]

I also had a look into promisify-any. promisify-any is capable of wrapping also generator functions.

Is there any necessity to support generator functions?