Closed ahnolds closed 3 years ago
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
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 await
ed, which is really what matters here. Take a look at this StackOverflow post for more.
@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)
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 Promiseresponse
generated by message.reply). For example:Switching to use is-promise resolves the issue by correctly handling all
then
ables as Promises, including alternative Promise implementations like Bluebird.