Add general support for upcoming request types

[matt-kruse]

Allows alexa-app to handle future unknown request types by assigning a general on() function to map request types to handlers.

[matt-kruse]

No tests for this PR because I am not familiar with this kind of test-writing.

Just to be clear on the intent of these changes - I have access to beta features not yet released, and the API includes request types not handled by alexa-app. I needed to be able to handle those new request types. This is a general way to handle upcoming API changes until explicit handlers can be added. I plan to add more API-specific handling code once the API is public.

[kobim]

Hi @matt-kruse, Before commenting on the changes, may I ask why not using your new mechanism for all request types? There are bunch of "standalone functions" who can be transformed to the new format (launchFunc, sessionEndedFunc, audioPlayerEventHandlers, etc.). It will allow to clean the this.request function a bit, and also standardize any future request type implementation.

[lazerwalker]

@matt-kruse: Awesome! This makes total sense to me.

@kobim: Would love to hear other folks chime in, but that seems like a totally reasonable future PR, and very much inline with the design philosophy of e.g. a lot of the Node core modules. The important thing would be providing backwards compatibility and a nice long deprecation path for all the existing standalone functions, as that would be a pretty huge breaking change we wouldn't want to just surprise people with.

[kobim]

@lazerwalker I'm not talking about replacing the interface, just the inner implementation. Replacing the individual variables with an on('...') will allow trimming the big "if-else" block of this.request:

  this.request = function(request_json) {
  ...
      requestType = request.type();
      if (!response.resolved) {
        if (typeof self.requestHandlers[requestType] === "function") {
          return Promise.resolve(self.requestHandlers[requestType](request, response));
        } else {
          throw "INVALID_REQUEST_TYPE";
        }
      }
  ...
  };

  ...
  this.on('IntentRequest', function(request, response) {
    var intent = request.data.request.intent.name;
    if (typeof self.intents[intent] === "undefined" || typeof self.intents[intent].handler !== "function") {
        throw "NO_INTENT_FOUND";
    }
    if (self.intents[intent].isDelegatedDialog() && !request.getDialog().isCompleted()) {
        return request.getDialog().handleDialogDelegation(request, response);
    } else {
        return self.intents[intent].handler(request, response);
    }
  });
  ...

then replacing the "simple" callbacks

  this.launchFunc = null;
  this.launch = function(func) {
    self.launchFunc = func;
  };

with:

  this.launch = function(func) {
    self.on('LaunchRequest', func);
  };

and the other "complex" handlers:

  this.audioPlayerEventHandlers = {};
  this.audioPlayer = function(eventName, func) {
    self.audioPlayerEventHandlers[eventName] = {
      "name": eventName,
      "function": func
    };
  };

with:

  this.audioPlayer = function(eventName, func) {
    self.on(`AudioPlayer.${eventName}`, func);
  };

[matt-kruse]

The advantage of type-specific handlers is that they can do a little pre-processing of the request json before passing it off to the handler. They might extract and normalize a data structure or do some conversions, so the handler has more convenient access to data. It also helps a developer fill in what they can do, like "oh, I need to handle a launch request" instead of knowing every request type. When I first created the API I didn't anticipate lots of new request types, so it was pretty hard-coded. Going to a more general approach seems appropriate now that there are a number of new request types on the horizon.

[matt-kruse]

Oops, I think I accidentally pushed more commits, but they should be the exact same changes.

The Gadgets API will be released pretty soon, and these changes are required in order to handle the API at all. There are other changes I want to make to support the API, but this is the bare minimum.

I would really like to see a version update asap so it's available when the Gadgets API is made public.

[dblock]

The changes look ok, but this really needs tests and a green build, please.

[matt-kruse]

Yeah, I don't really have time to do that stuff, nor do I understand the framework you have setup for all that stuff. Either someone else will need to do that or I'll have to fork and distribute separately. Sorry, I need to move quickly.

[dblock]

Hopefully someone can finish this PR by writing tests.

[matt-kruse]

I did a quick look at the structure of tests and I don't really understand enough to do it. I love unit testing, but I don't personally subscribe to such exhaustive testing requirements, so I'm a bit lost in all that test code. :)

I also don't know anything about TypeScript, so I am not sure how to correctly update those definitions.

[matt-kruse]

The Gadgets API has been officially released today, for creating skills that use Echo Buttons. These changes are required for Gadgets skills to be developed using alexa-app. If you can provide direction on what kinds of tests are expected and how to fix the build, I can take a stab at it. I just don't have time to learn it all from scratch. I'd really like to get this out there so people can build gadget skills using alexa-app.

[dblock]

Just needs tests to exercise the new code paths with on for example. Maybe exactly the code in the README?

The build being broken on some node versions here is something else, maybe @lazerwalker can help?

[matt-kruse]

Just took another look, I still can't figure out where the code would go in the testing structure. I don't know where/how to plug it in. I'll see if I can find anyone in my circle that can make sense of this.

[lazerwalker]

I'm taking a look at the TS CI build failures now.

In terms of tests, you'll probably want to make a new test file (e.g. test_alexa_app_request_handlers.js). You probably want to create an app in your test, set it up with a request type via on, load in a test fixture containing JSON payload data for the type of request you'd expect to handle it, and then assert that the response is what you'd expect it to be.

https://github.com/alexa-js/alexa-app/blob/master/test/test_alexa_app_launch_request.js might be a good example for a similar structure.

[lazerwalker]

I don't know what's up with the Node 1-4 errors, but I just pushed a fix to a small typo you had in the index.d.ts type definitions, that should clear up the latest node build error.

[matt-kruse]

@dblock the tests are there, the build is still failing. I don't know much about that, but it looks like npm install is failing due to typescript dependencies?

[lazerwalker]

As per #324, this appears to be an upstream issue in tslint: https://github.com/palantir/tslint/pull/3792.

It works for versions of npm >= 5 since that's the first version of npm that respects package-lock.json to properly resolve for us.

I'm not sure what to do here. This is 100% a spurious tooling issue rather than an actual code failure. Maybe we could muck with things to get earlier versions of npm to resolve correctly, but that's tricky since tslint is a sub-dependency rather than an explicitly declared one. It's possible that specifying a more exact version of dtslint might be capable of fixing it.

If that doesn't work out, it seems like our most likely options are (a) merge something in that turns the build red (I assume this is right out), (b) disable CI builds for node 1-4 until the Palantir PR lands, or (c) wait to merge this into master until the Palantir PR lands. None of those are great options.

[dblock]

Can we just lock down the version of tslint?

[dblock]

And all that done in https://github.com/alexa-js/alexa-app/pull/329, thanks @kobim. This should be rebased.

[matt-kruse]

Oh man, I don't know how to rebase. Off I go to learn more git...

[dblock]

Just merge from master.

Learning things is great though :) Rebase is a cleaner merge.

git checkout master
git pull upstream master
git checkout future-features
git rebase master
# fix conflicts, etc.
git push origin future-features -f

[matt-kruse]

Thanks. I hope that did the trick...

[dblock]

Tests look good. Minor comments on making the javascript prettier ;)

@lazerwalker @kobim you both cool with this code? merge at will

[dblock]

Please do update CHANGELOG @matt-kruse.

[matt-kruse]

I didn't realize there were strict coding guidelines. I find the overhead of making a simple change like this to be a little overwhelming! If you require exact formatting I can make the noted changes, but if not then I'd prefer to just move on and get this out there. I have people waiting on this change to be published.

[dblock]

Thanks for contributing, @matt-kruse. Merged via https://github.com/alexa-js/alexa-app/commit/3d9777e2f6112bb64b584599e5502908440cb31e

[matt-kruse]

Thanks! I will try to do better in the future. ;)