Router

[fremail]

Hello!

I implemented a router requested in #262. Actually I brought most code of the router from the incoming request routing based on request type.

In two words, I did two things:

1) Added ability to route request to intent/AudioPlayer event handler/PlaybackController event handler/custom request handler and other request types. request.getRouter().launch(); request.getRouter().sessionEnded(); request.getRouter().intent("oneIntent"); request.getRouter().custom("sayHello"); request.getRouter().audioPlayer("PlaybackNearlyFinished"); request.getRouter().playbackController("NextCommandIssued"); request.getRouter().displayElementSelected();

2) Simplify incoming request routing by using the new router.

resolves #262, #102

[kobim]

Hi @fremail, thanks for this addition.

As the implementation seems generally solid, I wonder why did you choose to put the router as an attribute on the Request object rather pass it as a new last parameter to the handler functions:

app.intent("MyIntent", function (request, response, router) {
  // ...
});

I'm just afraid for some memory issues, as there's a circular reference between Request and Router.

Any thoughts/insights about this @lazerwalker ?

[fremail]

@kobim AFAIK Request will refer to Router (not have a copy of it), so there shouldn't be memory issues.

I did similar to Session object which is in Request. And the Router is in Request because it sounds good: "Route request to ...".

There is no problem to separate it from Request if you want.

---

Also during last several days I'm thinking about some flag for stopping next routing. For example in pre you decide to switch to myIntent intent though the original request was to originalIntent. And you do not want to execute originalIntent. My example in code:

// incoming request is to originalIntent
app.pre = function (request, response) {
    return request.getRouter().intent('myIntent').end();
};
app.intent('originalIntent', function () {
    // won't be executed in this example
});
app.intent('myIntent', function (request, response) {
    response.say('Hi there!');
});

Though maybe it's worth to be a separate PR.

[kobim]

@dblock what do you think?

[dblock]

This looks good to me. I say @lazerwalker makes the final call :)

[lazerwalker]

Sorry, somehow missed the earlier ping for this! This all LGTM 👍.

I'm not too concerned about memory leaks — I've never seen circular references like that cause any practical JS issues, and some Googling suggests modern JS engine GCs are generally smart enough to handle situations like that without trouble.

It felt weird at first glance, but thinking about it, I think having router be an attribute on request makes more sense than alternatives. I like how it mirrors how we handle session.

[dblock]

I made a nitpick on changelog, otherwise anyone hit merge pls.

[fremail]

Updated Changelog.md.

Squashed and rebased my commits.