Use is-promise to check if objects are Promises

[ahnolds]

This adds support for alternative Promise implementations.

In particular, we had seen several cases where our command Inhibitors were producing ugly error messages about invalid return types, even though we are returning what should be a valid value (an object with a string reason and a Promise response generated by message.reply). For example:

        client.dispatcher.addInhibitor(function(message){
            if (message.command && message.command.name === 'set-name' && !TrainManager.isTrainChannel(message.channel.id)) {
                return {reason: 'invalid-channel', response: message.reply('use this command from a train channel.')};
            }
            return false;
        });

Switching to use is-promise resolves the issue by correctly handling all thenables as Promises, including alternative Promise implementations like Bluebird.

[HarmoGlace]

Why do you need to use a package who have only 2 lines to check if it is a promise ? The problem with this package is that any object with a then function will be detected as a Promise, e.g.

const isPromise = require('is-promise');
const object = {
     then: () => null;
};

isPromise(object) // will return true

object instanceof Promise // will return false

[ahnolds]

As for why you need a package, I guess you don't - if you'd rather copy that code locally and cite the source instead of adding a lightweight dependency, that's your call. Personally in my node development experience, the only times I've seen people averse to adding dependency packages for functionality are projects that are dependency-free to start with, which Commando clearly isn't. That said, whatever the maintainers prefer.

As for any object with a then method being treated as a Promise, that's kind of the point. There are many Promise implementations (jQuery, Bluebird, Q, ES6 native, ...), and they will not always be instanceof Promise, hence my code snippet above not working in my project even though it pretty clearly should. Per my understanding of the spec (see Promises/A+), an object/function with a then method is a thenable (admittedly yes, a thenable must confirm to the rest of the spec to be a real promise) and can reasonably be awaited, which is really what matters here. Take a look at this StackOverflow post for more.

[ahnolds]

@HarmoGlace (or anyone else who looks at these PRs) any more thoughts on this? It's been two weeks since I submitted this. Just to give a bit more justification, here is a minimal example of code that causes this problem:

bot.js:

// Use Bluebird promise implementation
global.Promise=require("bluebird");

const path = require('path');
const Commando = require('discord.js-commando');

const client = new Commando.Client();
client.registry.registerGroup('test', 'Test commands');
client.registry.registerCommandsIn(path.join(__dirname, 'commands'));
client.on('error', console.error);
client.login('<TOKEN>');

commands/test/test.js:

const Commando = require('discord.js-commando');

class TestCommand extends Commando.Command {
    constructor(client) {
        super(client, { name: 'test', group: 'test', memberName: 'test', description: 'Test the inhibitor'});

        client.dispatcher.addInhibitor(message => {
            return {reason: 'unauthorized', response: message.reply('this command can never be run')};
        });
    }

    async run(message, args) {
        return await message.reply("this should never happen");
    }
}

module.exports = TestCommand;

Error output when the !test command is run:

TypeError: Inhibitor "" had an invalid result; must be a string or an Inhibition object.
    at CommandDispatcher.inhibit (/home/alec/my_test/node_modules/discord.js-commando/src/dispatcher.js:206:12)
    at CommandDispatcher.handleMessage (/home/alec/my_test/node_modules/discord.js-commando/src/dispatcher.js:126:27)
    at CommandoClient.<anonymous> (/home/alec/my_test/node_modules/discord.js-commando/src/client.js:64:51)
    at CommandoClient.emit (events.js:315:20)
    at MessageCreateAction.handle (/home/alec/my_test/node_modules/discord.js/src/client/actions/MessageCreate.js:31:14)
    at Object.module.exports [as MESSAGE_CREATE] (/home/alec/my_test/node_modules/discord.js/src/client/websocket/handlers/MESSAGE_CREATE.js:4:32)
    at WebSocketManager.handlePacket (/home/alec/my_test/node_modules/discord.js/src/client/websocket/WebSocketManager.js:384:31)
    at WebSocketShard.onPacket (/home/alec/my_test/node_modules/discord.js/src/client/websocket/WebSocketShard.js:444:22)
    at WebSocketShard.onMessage (/home/alec/my_test/node_modules/discord.js/src/client/websocket/WebSocketShard.js:301:10)
    at WebSocket.onMessage (/home/alec/my_test/node_modules/ws/lib/event-target.js:125:16)
    at WebSocket.emit (events.js:315:20)
    at Receiver.receiverOnMessage (/home/alec/my_test/node_modules/ws/lib/websocket.js:797:20)
    at Receiver.emit (events.js:315:20)
    at Receiver.dataMessage (/home/alec/my_test/node_modules/ws/lib/receiver.js:437:14)
    at Receiver.getData (/home/alec/my_test/node_modules/ws/lib/receiver.js:367:17)
    at Receiver.startLoop (/home/alec/my_test/node_modules/ws/lib/receiver.js:143:22)