Promise from macros never resolve

[josephrexme]

Following up on #43 I have a macro returning a promise and it never resolves. With the following code:

const reply = rs.reply(user, message)
console.log(reply, typeof reply) //=> [object Promise] , string

I always get a string. Then I tried with the replyAsync

const reply = rs.replyAsync(user, message)
console.log(reply, typeof reply) //=> Promise { ... }, object
reply.then(replyStr => console.log(replyStr, typeof replyStr)) //=> [object Promise], string

It always return the promise value as a string.

[josephrexme]

When I use this fork https://github.com/genericallyloud/rivescript-js

const reply = rs.reply(user, message)
console.log(reply, typeof reply) //=> Promise { <pending }, object
if(reply.then){
  reply.then(replyStr => {
    console.log(replyStr) //=> hello
    bot.sendMessage(replyStr, channel)
  })
}else{
  bot.sendMessage(reply, channel)
}

The only problem is the fork is way behind and I'd like to also stay updated with changes on this repo. I guess it solves most of the problem and is backward compatible

[kirsle]

This one stumped me for a little bit. Here's an example that works with the latest version: https://play.rivescript.com/s/T7AQtaUpn3 (enable "Async Replies" at the bottom)

It seems that the native Promise object to JavaScript isn't supported by RiveScript right now. When this feature was first implemented, an rs.Promise attribute was added which was an rsvp.js object that implemented a Promise polyfill, and so you would return one of those from object macros. This seems to still work.

When I tested the above code with the native Promise object, the reply would come back as "Testing: [object Promise]" whether replyAsync() or reply() were used. The reply itself was being resolved, but the inner promises for the object macros were not.

When I switched to the rs.Promise polyfill, it started working as expected: replyAsync() resolved all the promises correctly (the reply was delayed 1 second due to the setTimeout()), and if I tried it with the normal reply(), it would return a reply of "Testing: [ERR: Using async routine with reply: use replyAsync instead]" which is what I expect.

My hunch is there is an API difference between rsvp.js and the native Promise, and so internally when it tries to resolve the object macros, it fails to.

[dcsan]

as a user of the library, is this something we need to be aware of?
ie doesn't replyAsync() construct and return the promise? there's nothing we can do outside to control which promise library is used?

is this related to rs.Promise ?

I also haven't been able to get values back to Rivescript from simple javascript calls:
#234 (comment)

it seems that a next tick is required before we can do things like conditionals.

[josephrexme]

I honestly think we wouldn't need a replyAsync at all and if RSVP has an inconsistent behavior that differs from native promises, we can simply modify to fit whatever Promise library is being used like in the fork I referenced. That way .reply would reply normally for synchronous macro returns and would have a .then for the ones returning a promise.

I'd try to add changes from that fork into this but I'm afraid we may have to just ditch replyAsync completely. If this is something agreed on I could go ahead and make a PR

[kirsle]

@josephrexme: That way .reply would reply normally for synchronous macro returns and would have a .then for the ones returning a promise.

.reply was there first and returned a string, and .replyAsync returns a promise. This was to avoid breaking backward compatibility with code that was expecting a string to come back from .reply().

The other way to avoid breaking compatibility would be to add an additional parameter to reply() indicating that you wanted a promise out of it, but .reply() already has a long list of parameters, username, message, scope

@dcsan: it seems that a next tick is required before we can do things like conditionals.

Async-Await (like #220) will let us "pause" and wait for macros to resolve for things like conditionals and will make all of this a lot nicer. We could also use it in the reply() function, so that the caller doesn't have to worry about the promise -- they get a string back anyway, because we can wait for promises from object macros.

---

What we should do for this ticket: fix the promise system, get rid of rsvp.js and try to use native promises, or find a better polyfill library when native promises aren't available.

[josephrexme]

Yeah that fork works in a similar way. A string always get returned when you don't have a macro that returns a promise. I guess what I don't understand is how to know when to use replyAsync. Imagine having some macros that return a promise and static string returns from other matchers. If my code was to be handling replies to them this is what it'd look like:

const reply = rs.reply(user, message) // At this point I don't know if it'd return a string or promise so
// how do I decide if reply or replyAsync
//
// if I know I've added some macros returning promise then I can check for them
if(reply.then){
    reply.then(value => bot.reply(value))
}else{
  bot.reply(reply)
}
// if I don't need to work with promises at all
bot.reply(reply) // Reply is a string

Now the question remains why would I be using replyAsync when I have strings coming through as replies too? It'd mean to use replyAsync I'd have to do the same thing to check if what was returned is a string or a promise.

Currently using it like that on https://github.com/josephrexme/rivescript-promises and I could work on this if you get my point. In the long run we may decide to keep replyAsync for backward compatibility but it would do basically the same thing as reply

[kirsle]

I guess what I don't understand is how to know when to use replyAsync.

Short answer: just use replyAsync() every time in new code. It's backwards compatible with reply() and has the added benefit of supporting async object macros.

Longer answer: reply() was there in the beginning, when RiveScript had no support for async code at all, and it returned a string. When async macro support was added, replyAsync() was added too.

It was a different function because reply() was already documented to return a string. I agree with you that it would be dumb to force the end user to type check what they got from reply(), so I'd rather it always be a string, or always be a promise, not sometimes one or the other.

Naive code that pre-dated the async macro support might sometimes tell the user "Bot: [object Promise]" if they didn't know to type check the reply. Instead, what happens is that if an async macro was called, the bot will say "[ERR: async routine called from reply: use replyAsync instead]" in place of the macro's output. This would tell the bot maker they should update their code to use replyAsync() if they want to use async macros.

[josephrexme]

Thanks for #248 @kirsle looks like it addresses my problem here. I'll pull it in to test.