Fix #52, #21 and #35

[dblock]

@mreinstein correctly calls that #54 fixes #52, #21 and partially #35. What this does is use alexa-app's express method instead of cooking our own, plus a test that right now would hang on master and no longer hangs in this branch.

Replaces #56 on top of #60.

This is dependent on alexa-js/alexa-app#150, I didn't find a better way to get ?utterances, ?schema and preRequest and postRequest support without moving this logic upstream to alexa-app.

[rickwargo]

This does indeed fix #21 (it was not working prior to this without a different fix). Also confirmed http/https on custom ports are working as expected.

Looks like test.ejs, schema and utterances are working, too, using GET.

[dblock]

Thanks for checking it out @rickwargo. I'll wait for @tejashah88 to do a code review of this and the upstream PR and merge. We should get things sorted out with a release next. Do you have anything blocking and not working after this?

[rickwargo]

@dblock on the surface it appears all good, but haven't had the time to dig under the covers. i'll have more time tomorrow.

[tejashah88]

According to this comment in #18, shouldn't it be this:

self.express.use("/alexa", bodyParser.urlencoded({ extended: true }));

#35 stated that "there are many express routes added which don't use urlencoded forms". So logically, one would want to only bind with Alexa-based routes.

[dblock]

Yes, but that issue is still open, I didn't get a chance to write a test for that and fix it (and I definitely don't want to change it here without a corresponding test). There were tabs->spaces changes here so it looks like this code has changed, but it has actually not. You can look at the diff without spaces, https://github.com/alexa-js/alexa-app-server/pull/64/files?w=1

[dblock]

I've updated this with a fix for #35. Now that we're mounting body-parser inside alexa-app, we can just remove it from alexa-app-server. See alexa-js/alexa-app#155 for details.

@tejashah88 this is ready for you to merge

[dblock]

Once we've released 2.4.0 I'll change this and make a release of alexa-app-server.

[tejashah88]

These changes look good so far. I'll wait for @rickwargo to say more about this before merging.

[rickwargo]

Sorry, @tejashah88, didn't get a chance to review today. I will have time in the morning.

[rickwargo]

Looks good, however bodyParser.urlencoded() is now removed via alexa-app#152 - found this testing a form post hosted in alexa-app-server. I believe this was previously handled - does it need to be handled or should it be called out in the README?

[rickwargo]

One (simple) formatting suggestion is to remove trailing slashes from the normalizedRoot in self.loadapps(). The comments have an example where the server root ends with a slash and if included, the message says, "loaded app [hello_world] at endpoint: /alexa//hello_world."

[dblock]

I think the urlencoded bodyparser is not the business of these libraries. I've added a note to UPGRADING in alexa-js/alexa-app#158.

Opened #68 for the slashes thing, it's cosmetic unless I'm missing something.

[dblock]

Looks like everyone is happy with this, merging. Thanks for your help!

[rickwargo]

@dblock cosmetic only. I'm fine either way on the urlencoded bodyparser - #64 breaks functionality so it needs to be noted. Could be I was the only one using that.

[dblock]

Yep @rickwargo, I documented this change in UPGRADING, both here (#69) and alexa-app (alexa-js/alexa-app#158).