alexa-app
alexa-app copied to clipboard
alexa-js
Stop wrapping calls with global try-catch
I was banging my head against the wall trying to figure out why alexa-app wasn't seeming to call some of my Intent events correctly.
I finally figured out that alexa-app wraps the event calls in a try-catch, invisibly discarding all exceptions. This makes it hell to develop on!
Did you try defining app.error? That allows you to handle the exceptions however you wish. Otherwise, they are silently eaten to prevent the exception from bubbling up and potentially crashing the server.
Ultimately I feel that if I write code to crash my app, it's my responsibility to handle in my code. Capturing exceptions from code the user of your module is writing just doesn't seem to me like good practice.
If this is a common node pattern I'm unaware of then by all means, but this is the first time I've encountered it!
(I didn't know about app.error though. I'm using that now. I swear I looked in the readme but clearly missed that big section :) )
You can trap and throw an exception if you want:
// Log and throw intent exceptions app.error = function(e,request,response) { console.log(e); throw e; };
The reason I trap errors is because there needs to be some feedback back to Alexa. It should respond with something, even in the worst cases where an exception is thrown. So, by default, my library eats the exception and handles it for you so things don't blow up. If you want to handle it differently, you can. I just want the default to be "don't crash the server, and at least respond with something". Does that make sense?
I disagree with it philosophically, but it's your module! The app.error method is more than a workable solution so it's not a huge deal.
I think I would appreciate it more if I were running something like a web server where crashing would be extra bad, but I am using Lambda which already has a layer to prevent crashing from being catastrophic.
For that case, yes, I can see how you would prefer no default error handling. In my case, I'm imagining running a container for Alexa apps, and I never want an individual app to be able to ruin the party for others. Especially if the apps are maintained by different authors.
Though, I was just doing some testing right now and was confused why some code wasn't running. It was because it was throwing an exception I didn't handle, and the response was sent back anyway. So I see your point.
Maybe in my case, where I don't want applications to harm each other, I should protect against that in my own error method, and by default in the module, unhandled exceptions should be thrown. I think you've convinced me. :)
It's also worth noting that the general pattern you're using to throw errors that are caught locally is discouraged. So much so that WebStorm is flagging it!
I agree that it's discouraged if you catch your own errors. The difference here is that I am enabling developers to handle the caught exception. So it's not flow control, but real exception handling just done in a way that can be handled by the user. WebStorm would have no way of knowing that, so I don't blame it for flagging it.
I think I figured out what the overall issue is with this implementation. It's not the global try catch necessarily - promise libraries do the same thing.
It's that the default behavior is that if the user doesn't implement the error event, the errors silently disappear. I feel that if the error handler isn't defined, any errors (other than the ones you are specifically purposely throwing and handling) should be thrown up the chain to be handled like normal. That way you get some convenient error handling that protects from crashing, but doesn't redefine very basic assumptions such as "an unhandled exception should crash my app".
I'm still torn on this. In theory, I agree with you. I'd prefer to throw the exception by default, unless the user defines some special handling. But I think the Echo is a bit different. If an app crashes, there is no useful error message returned, nothing saying what is really going on. There is no error screen or console for people to see. So my goal was to always provide some useful response by default. When I was developing my first app, I was very frustrated that it wasn't working, and I couldn't figure out what the errors were. I had to put in special error handling just to get some useful response back while running. So from that frustration, I felt that a framework should, by default, always try to return some useful error messages in the event that there is a problem or exception. it makes it much easier to debug a problem. I'm going to leave this open and gather some more feedback from others.
FWIW, I'm pretty much 50/50 on my original request now that I've thought about it more, because of exactly what you said. My apps are using promises that always have a catch on the highest chain, so that I can always give a friendly response relevant to the Intent, which is the same thing you're trying to handle, just one step higher. As long as nothing makes it past my catches, I shouldn't need your error handler to kick in.
This issue is effecting me as Im trying to debug an app I'm writing and errors are getting caught and discarded before I see them. What about a DEBUG_MODE that turns on this functionality during development?
Personally I'd side with @OverloadUT around unnecessary trapping of errors, I ran into this problem a million times while developing a skill. Maybe @OverloadUT can propose a pull request that addresses it and we can talk around it? We would need to provide an UPGRADING document and a default error handler that doesn't swallow errors but maybe does something more intelligent?
Chiming into this thread, I'm also with @OverloadUT and @dblock on this one. But perhaps it is worthy of expanding the conversation a bit more broadly. What do you think about the following idea:
-
alexa-appshould (philosophically) move in a direction where defaults ease debugging and/or hosting in AWS Lambda. -
alexa-app-servershould override these these defaults when it loads an app to be appropriate to running inexpress. If there is a want from the community, these config setups could eventually be extracted to a separate module for use outside ofalexa-app-serverbut, we'll start small. (Because frameworks should be extracted, not built, IMHO)
This gives a good philosophical divide as to where particular pieces of code, be it for error handling or anything else, should reside. Thoughts?
Philosophically I think those should have the same level of support inside this library - what I mean is that if we have code that helps with lambda in alexa-app we shouldn't be putting code that helps with express into alexa-app-server. Then, "mounting" alexa-app inside one or the other should be a 1-liner. But I don't know what that all means, I struggled a bit with making it all work in https://github.com/artsy/elderfield so whatever makes that easier is a go.
I want to add that right now on HEAD we went almost the other direction. There's a bunch new code to make it work in the Express scenario and to integrate nicely with alexa-app-server. Check out #150 notably.