Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
rmberrios opened this issue Jul 5, 2019 · 6 comments
Open
Assignees

Comments

@rmberrios
Copy link
Contributor

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
Copy link
Contributor

armonge commented Jul 6, 2019

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
Copy link
Contributor

armonge commented Jul 6, 2019

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

@wuelcas
Copy link
Contributor

wuelcas commented Jul 8, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

armonge commented Jul 8, 2019

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
Copy link
Contributor

omenocal commented Jul 8, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants