VoxaAI / voxa

Voxa is a framework that uses state machines to create elegant cross platform conversational experiences.
http://voxa.readthedocs.io/
MIT License
73 stars 26 forks source link

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

Open rmberrios opened 5 years ago

rmberrios commented 5 years ago

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!

armonge commented 5 years ago

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 commented 5 years ago

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

wuelcas commented 5 years ago

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.

rmberrios commented 5 years ago

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?.

armonge commented 5 years ago

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

omenocal commented 5 years ago

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