voxa icon indicating copy to clipboard operation
voxa copied to clipboard

They way voxa handles different types of request is really confusing!

Open rmberrios opened this issue 5 years ago • 6 comments

Currently we handle different types of AlexaRequest.

const AlexaRequests = [
  "AudioPlayer.PlaybackStarted",
  "AudioPlayer.PlaybackFinished",
  "AudioPlayer.PlaybackNearlyFinished",
  "AudioPlayer.PlaybackStopped",
  "AudioPlayer.PlaybackFailed",
  "System.ExceptionEncountered",
  "PlaybackController.NextCommandIssued",
  "PlaybackController.PauseCommandIssued",
  "PlaybackController.PlayCommandIssued",
  "PlaybackController.PreviousCommandIssued",
  "AlexaSkillEvent.ProactiveSubscriptionChanged",
  "AlexaSkillEvent.SkillAccountLinked",
  "AlexaSkillEvent.SkillEnabled",
  "AlexaSkillEvent.SkillDisabled",
  "AlexaSkillEvent.SkillPermissionAccepted",
  "AlexaSkillEvent.SkillPermissionChanged",
  "AlexaHouseholdListEvent.ItemsCreated",
  "AlexaHouseholdListEvent.ItemsUpdated",
  "AlexaHouseholdListEvent.ItemsDeleted",
  "Connections.Response",
  "Display.ElementSelected",
  "CanFulfillIntentRequest",
  "GameEngine.InputHandlerEvent",
  "Alexa.Presentation.APL.UserEvent",
  "Messaging.MessageReceived",
];

but in order to use it, you need to know which are marked as on${whateverRequestType} or be treat as an intent like CanFulFillIntentRequest.

My proposal to fix this issue is to treat all the other request as onStates except(OnrequestStarted, OnsessionEvents, etc. - lifecycle events). This change will also fixes #242 Because there won't be a need to manually include new event request types. It doesn't matter the event, Voxa will just work!.

Thoughts? If you like my proposal give a thumbs up or down! You can also propose a new approach!

rmberrios avatar Jul 05 '19 17:07 rmberrios

I don't like it, that would include state machine concepts (States) in request types that in reality are not part of the state machine execution, we never transition between states in AudioPlayer events or similar, in fact we couldn't even if we wanted cause those request types are out of session. If CanFulFillIntentRequest can be executed with onState then that seems like an error to me

armonge avatar Jul 06 '19 16:07 armonge

But i do like the idea of having something like "onRequest" which just handles whatever request type we throw at it

armonge avatar Jul 06 '19 16:07 armonge

Is the state machine concept strictly bound to being in a session?

I recently worked on a skill using the audio player, and I wanted to handle the PlaybackController request when a user resume the audio using the touch screen controls on a echo show/spot. I couldn't reuse the code I user for the ResumeIntent and had to duplicate code and I needed the build the AudioPlayer directive manually because the AudioPlayer class attach the directive to the reply when we return it in a state. I would be great to be able to reuse code in other states when having a request type, handling the request either with onState or onRequest, whichever we decide.

wuelcas avatar Jul 08 '19 14:07 wuelcas

I think is more related to the way voxa handles a state machine intent request. it could be troublesome to manage those intents that don't return a speech or session attributes. At least we agreed on something we should have a handler perhaps named onRequest. Could we come up with another way to reuse code out of another state?.

rmberrios avatar Jul 08 '19 15:07 rmberrios

you could do something like

function genericHandler(event, reply) {
// do something
}

app.onAudioPlayerSomething(genericHandler)
app.onAudioPlayerSomethingElse(genericHandler)

I do think it's a great point that the directives should work on this handlers, at least the ones that make sense

armonge avatar Jul 08 '19 15:07 armonge

I like the idea of the onRequest method, like:

voxaApp.onRequest('PlaybackController.NextCommandIssued', (voxaEvent) => {
// CODE
});

Though we should think more about the way to return the object, as most of them are not part of the state machine cycle

omenocal avatar Jul 08 '19 16:07 omenocal