updated to fix "Error" on ISP requests

alexa-js / alexa-app

A framework for Alexa (Amazon Echo) apps using Node.js

https://www.youtube.com/watch?v=pzM4jv7k7Rg

MIT License

1.03k stars 213 forks source link

updated to fix "Error" on ISP requests #368

Closed user1m closed 6 years ago

user1m commented 6 years ago

Fix issue where ISP requests added "'Error: not a valid request" to ssml output

user1m commented 6 years ago

@dblock please review :). I think a long term fix for handling when AZ introduces new requests types might be needed but for the time being this solves my problem

user1m commented 6 years ago

Ping @dblock

dblock commented 6 years ago

Ok, I read the docs. It's not much :( They don't say anything about these "connections" other than "to implement a purchase request put Connections.Response here" or something like that.

I understand we're fixing an error for these requests, but it feels to me we want to do more than just resolve the call here. Two thoughts:

It's only a matter of time till Amazon adds some new type of request/response and maybe our code needs to handle the future better instead of always handing a special case like this one. If that's true, then maybe there's a different/better fix for this specific problem?
Do we want to handle purchase flow natively? I think the answer is yes. It doesn't mean we don't want a fix for this specific problem, but if we're going to be checking for a specific request type we should be doing more than a Promise.resolve, similar to audio requests.

Thoughts?

Also cc: @lazerwalker who was doing a lot with this and maybe has an opinion

dblock commented 6 years ago

And an even more basic question, we have support to handle these via on.

app.on('Connections.SendRequest', (request, response, request_json) => {
});

So why do we need this code?

matt-kruse commented 6 years ago

Yes, these new types of requests will keep coming faster than we can directly support the new features. That's exactly why I added .on() so devs could handle new request types.

Ideally, each new type of functionality that sends new request types will get full support with helpful utilities and built in handling. Eventually.

There is a lot of new stuff coming. Trust me. :)

user1m commented 6 years ago

Ok maybe I was just using the sdk wrong. But the issue I was trying to solve was that the SDK kept adding "Error: not a valid request" (INVALID_REQUEST_TYPE) to requests with Connections.SendRequest.

@matt-kruse can you confirm if I move to app.on('Connections.SendRequest', ... I will avoid this error?

lazerwalker commented 6 years ago

Yep, switching to app.on will clear up that exact error; the result should be functionally identical to your proposed patch. If you're curious about the code, I'd poke at https://github.com/alexa-js/alexa-app/blob/master/index.js#L619-L623 and https://github.com/alexa-js/alexa-app/blob/master/index.js#L466-L468.

Just otherwise wanted to +1 @dblock and @matt-kruse's points here. Eventually, having our own consciously-designed API methods and helpers for the payment flow would rock. But it's probably fairly unstable on Amazon's end, and fairly unclear on our end what the correct abstraction is. app.on feels like the right tool for right now.