User-defined error handler should automatically resolve unresolved … #183

rickwargo · 2017-02-16T17:31:20Z

…responses, update README to reflect behavior.

Add tests for error handler resolution and error messages.

…sponses, update README to reflect behavior. Add tests for error handler resolution and error messages.

ajcrites · 2017-02-16T17:41:01Z

README.md

@@ -500,7 +503,8 @@ response.card({
 ## Error Handling

 Handler functions should not throw exceptions. Ideally, you should catch errors in your handlers using try/catch and respond with an appropriate output to the user. If exceptions do leak out of handlers, they will be thrown by default. Any exceptions can be handled by a generic error handler which you can define for your app. Error handlers cannot be asynchronous.
-
+The generic error handler, below, will automatically resolve the promise (request) with `response.send()` if it does not occur in the error handler.
+`response.fail(message, exception)` should only be called from a controlled calling environment, e.g. your own server, not AWS Lambda.


Why would this be the case? You should be able to use response.fail to force a failure response in any environment. Particularly this will trigger .error and .post.

Maybe it needs a better description. Calling response.fail() in a Lambda environment produces an undesired message from Alexa.

I agree, maybe something explaining what happens on Lambda. Can be edited later.

ajcrites · 2017-02-16T17:41:39Z

index.js

@@ -357,6 +357,8 @@ alexa.app = function(name) {
      var handleError = function(e) {
        if (typeof self.error == "function") {
          self.error(e, request, response);
+          if (!response.resolved)


Include braces

ajcrites · 2017-02-16T17:41:59Z

index.js

-          } else {
-            response.fail("Unhandled exception.", e);
-          }
+            if (e.message) {


Indent this block by two lines instead of four.

I'm fighting with my editor and my default of 4 - missed this one.

dblock · 2017-02-17T02:12:15Z

test/test_alexa_app_unknown_type_request.js

        });

        it("fails with not a valid request", function() {
+          testApp.error = undefined; // may be defined in other tests, must unset


The test above sets it, wrap that into a context and have a beforeEach and afterEach, where you set the error before and unset it after. This way you don't need to rely on the order of tests.

Now creating a new testApp instance and setting the error handler before each test.

dblock · 2017-02-17T02:12:38Z

test/test_alexa_app_unknown_type_request.js

@@ -21,25 +21,74 @@ describe("Alexa", function() {

        var mockRequest = mockHelper.load("unknown_type_request.json");

-        it("invokes a globally defined error function", function() {
+        it("invokes a globally defined error function that throws an error", function() {
          testApp.error = function(e, request, response) {


Similar to below, wrap error reset or create a new testApp instance every time.

Now creating a new testApp instance and setting the error handler before each test.

…ngle testApp.error

…m/rickwargo/alexa-app into test-to-handle-user-error-handler

rickwargo

Now creating a new testApp before each request and adding the appropriate error handler.

dblock · 2017-02-17T03:35:07Z

CHANGELOG.md

@@ -5,6 +5,11 @@
 * [#174](https://github.com/alexa-js/alexa-app/pull/174): Always enfore strict header checking - [@tejashah88](https://github.com/tejashah88).
 * Your contribution here.

+### 3.1.3 (February 16, 2017)


This should really happen when you do an actual release, but I'll let it slide ;)

dblock · 2017-02-17T03:35:32Z

CHANGELOG.md

@@ -5,6 +5,11 @@
 * [#174](https://github.com/alexa-js/alexa-app/pull/174): Always enfore strict header checking - [@tejashah88](https://github.com/tejashah88).
 * Your contribution here.

+### 3.1.3 (February 16, 2017)
+
+* [#182](https://github.com/alexa-js/alexa-app/issues/182): Fix: user-defined error handler should automatically resolve unresolved responses, update README to reflect behavior - [@rickwargo](https://github.com/rickwargo).


No need to mention things like "update README to reflect behavior"

rickwargo added 2 commits February 16, 2017 12:28

User-defined error handler should automatically resolve unresolved re…

0897a26

…sponses, update README to reflect behavior. Add tests for error handler resolution and error messages.

Changed pull request number in Changelog.

ac861c7

rickwargo force-pushed the test-to-handle-user-error-handler branch from 5d973c9 to ac861c7 Compare February 16, 2017 17:32

This was referenced Feb 16, 2017

Should app.error() resolve? #182

Closed

Updating library for asynchronous support through promises #171

Merged

ajcrites reviewed Feb 16, 2017

View reviewed changes

rickwargo force-pushed the test-to-handle-user-error-handler branch 2 times, most recently from 9a06347 to c85c071 Compare February 16, 2017 17:49

More formatting changes (indent 4 --> 2).

3c1b17d

rickwargo force-pushed the test-to-handle-user-error-handler branch from c85c071 to 3c1b17d Compare February 16, 2017 17:50

simple changes

331ba92

dblock requested changes Feb 17, 2017

View reviewed changes

rickwargo and others added 4 commits February 16, 2017 21:29

Create a new testApp prior to each request instead of manipulating si…

ae21607

…ngle testApp.error

Create a new testApp prior to each request instead of manipulating si…

130333b

…ngle testApp.error

Merge branch 'test-to-handle-user-error-handler' of https://github.co…

d1b24e7

…m/rickwargo/alexa-app into test-to-handle-user-error-handler

Merge branch 'master' into test-to-handle-user-error-handler

89925c1

rickwargo commented Feb 17, 2017

View reviewed changes

dblock reviewed Feb 17, 2017

View reviewed changes

dblock merged commit 1b2c3cf into alexa-js:master Feb 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User-defined error handler should automatically resolve unresolved … #183

User-defined error handler should automatically resolve unresolved … #183

rickwargo commented Feb 16, 2017

ajcrites Feb 16, 2017

rickwargo Feb 16, 2017

dblock Feb 17, 2017

ajcrites Feb 16, 2017

ajcrites Feb 16, 2017

rickwargo Feb 16, 2017

dblock Feb 17, 2017

rickwargo Feb 17, 2017 •

edited

Loading

dblock Feb 17, 2017

rickwargo Feb 17, 2017 •

edited

Loading

rickwargo left a comment

dblock Feb 17, 2017

dblock Feb 17, 2017

User-defined error handler should automatically resolve unresolved … #183

User-defined error handler should automatically resolve unresolved … #183

Conversation

rickwargo commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickwargo Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickwargo Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

rickwargo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickwargo Feb 17, 2017 •

edited

Loading

rickwargo Feb 17, 2017 •

edited

Loading