Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Promises/callbacks from request handlers (#71) #133

Merged
merged 3 commits into from
Jan 27, 2017

Conversation

ajcrites
Copy link
Collaborator

@ajcrites ajcrites commented Jan 27, 2017

For Issue #71

This allows you to return a Promise from a request handler (launch, intent handler, session ending, or audio player events). When the promise resolves, the response is sent. If the promise is rejected, it is treated like an error (e.g. if an error were thrown from the handler).

I have written the code so that it also now supports callbacks in an asynchronous pattern:

skill.intent("intentName", utterances, (req, res, cb) => {
  someAsync(cb);
  return false;
});

Currently you can either return a Promise or false from the handler in order to be asynchronous, but calling the callback makes calling res.send unnecessary.

This is backwards compatible, but I would be in favor of a major version change that supports only promises and just using:

Promise.resolve(handlerResult).then(res.send).catch(handleError);

We can also remove the return false functionality and make either returning a Promise or calling the callback required in order to send the response (or just call response.send() explicitly). I can either do this now as part of this PR or we can move forward with this and remove backwards compatibility later.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 04678a3 on ajcrites:promise/71 into ** on alexa-js:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling f7b5a1e on ajcrites:promise/71 into ** on alexa-js:master**.

@dblock
Copy link
Collaborator

dblock commented Jan 27, 2017

This is perfect, I'm merging it.

Take a look at code coverage there's one audioplayer scenario that should have a test, but that's not a blocker.

I don't know how I feel about forcing people into a backward incompatible way of doing something that quickly, but I opened #134, you should PR that on top and we can discuss it and see whether it really makes things so much better and whether paying the backwards compat price may be ok.

@dblock dblock merged commit 0129cb7 into alexa-js:master Jan 27, 2017
@dblock
Copy link
Collaborator

dblock commented Jan 27, 2017

@ajcrites very nice work, would love to have you on the maintenance team for alexa-app, especially given this change, interested?

@ajcrites ajcrites deleted the promise/71 branch January 27, 2017 17:22
@ajcrites
Copy link
Collaborator Author

@dblock I'll take a look at #134 as well. I'd love to be on the maintenance team :D

@chearmstrong
Copy link

Really nice solution to this @ajcrites - good work.

@xunxdd
Copy link

xunxdd commented Jul 18, 2017

From my test over and over again, without returning false, alexa would not wait for the callback and therefore deliver any response. It does work locally with the alexa-app-server.

Can you advise?

fb.getOverallData().then(function (data) {
res.say('xxx);
res.shouldEndSession(true);
res.send();
}).catch(function (err) {
res.say('xxx');
res.shouldEndSession(true);
res.send();
});
//return false;

@ajcrites
Copy link
Collaborator Author

ajcrites commented Jul 18, 2017 via email

@xunxdd
Copy link

xunxdd commented Jul 18, 2017

thanks! let me try it out.

@xunxdd
Copy link

xunxdd commented Jul 18, 2017

No, just tested it, it does not work. again it works locally with the alexa-app-server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants