using callback pattern for asynchronous code about alexa-app HOT 14 CLOSED

I personally wholeheartedly agree with this. It's the single biggest thing that bugs me about using this module personally: returning false on every single Intent handler is very non-standard and I have to comment every one to explain why it's there.

I think handling promises as return values would be ideal, and an alternate mocha-style done parameter would be nice as a second option .

from alexa-app.

I'm fine with only supporting promises, it makes things cleaner in the long run.

However since Bluebird as being used a the promise implementation we could use asCallback to easily support both.

In terms of the return false usages we should at the very least print a deprecation notice and suggest users to move to the newer and more standard api

@OverloadUT @chearmstrong any thoughts?

from alexa-app.

I agree, this is a bit annoying, but I put it in there originally for backwards compatibility. In the previous releases, handlers were required to be sync. Allowing for the return of an explicit Promise (as opposed to false) was in my plans but I never got to it.

I also wanted to keep it VERY simple to implement the most basic use cases, which are sync handlers. In that case, the developer never needs to worry about creating or fulfilling Promises or remembering to call a 'done' function. Just write some simple code and it works. My thought was that only more advanced functionality will be async in handlers, and at that point the developer can learn the additional requirements for that scenario.

I didn't want to use a callback, because Promises are supposed to avoid that ugly syntax. And forcing a callback parameter in method signatures makes it harder to add parameters later if necessary. Promises are the way to go, IMO.

from alexa-app.

Thanks. I'm just helping out, I do have committer rights and such. I know @matt-kruse is considering making an org and expanding the circle of friends, so looking forward!

from alexa-app.

Mocha uses some magic to make the done() callback only necessary if the implementing function signature actually sets the done parameter.

That way you get the best of both worlds: by default it's all sync, unless you define the done parameter in which case it's async. And alternatively you can leave done out of it and just return a promise!

However, that whole done thing is not terribly standard as far as I know.

from alexa-app.

Agreed, I'm all for promises. In my use of the library I used promises as well. Supporting the return of a promise shouldn't be too hard no? It can be treated similar to the return false check. Check if the return object is a promise and if so attach a then which either calls res.send() or accomplishes that same behavior some other way.

In terms of the done pattern we could investigate how mocha does that and replicate the behavior. Other wise the the return false pattern can be kept for non-promise based asynchronous code. That would be your decision in terms of how you want you library to present its interface. Either way I'd be happy to help with the changes.

from alexa-app.

any updates on this? I can submit a PR if you would like

from alexa-app.

@gurpreetatwal Would love to see a PR. You got my attention now.

from alexa-app.

I'll start working on it in about a week. Are you taking over the maintenance of this repo now? I'd love to help with that if you like

from alexa-app.

@dblock I now have time to start working on this task, however before I begin I wanted to plan the task out a bit more.

What exactly are we planning to achieve?

• Do we want to only support promises for asynchronous tasks or should we support the usage of a done callback as well?
• Should we still support the return false usage or just drop it completely and make a semver major change?

from alexa-app.

This is just my 2c, but I think we don't have to support done callback if we don't want to, however we should support false for backward compatibility if it's not too difficult.

from alexa-app.

Closing via #133.

from alexa-app.

Give @ajcrites work in #133 a try, lets see if there's anything missing.

from alexa-app.

Just seen this (sorry) but #133 seems like a good solution to this.

from alexa-app.