adding flexibility for attaching alexa-app to express #177

bgjehu · 2017-02-13T22:49:35Z

change alexa.express()
so that it takes either an express app or router and attach alexa-app to it under given endpoint.
leave the logic of binding router to app out of the scope of alexa-app/alexa.express()

increase flexibility for attaching alexa to either an express app or router

flexibility for attaching alexa to express

…ng-to-express # Conflicts: # CHANGELOG.md # package.json

Merge pull request #1 from bgjehu/flexibility-attaching-to-express

dblock · 2017-02-14T14:21:21Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## Changelog

 ### 4.0.0 (Next)
+* [#176](https://github.com/alexa-js/alexa-app/pull/176): Added flexibility to attach alexa-app to express app or router - [@bgjehu](https://github.com/bgjehu).


Needs a better explanation that "flexibility", maybe Express router is now optional?

dblock · 2017-02-14T14:25:53Z

This looks right to me, but I have this feeling that we're missing something, cc: @tejashah88.

@bgjehu Try this against alexa-app-server please, any failing tests?

The PR needs to be rebased, and see my minor comment on CHANGELOG above.

Please add a paragraph to UPGRADING wrt what one should do if they specified both express app and router before.

tejashah88 · 2017-02-14T15:19:14Z

Looks great so far, except for that endpoint does not have any normalization (i.e. ////test is valid).

Add utilities

bgjehu · 2017-02-14T23:54:31Z

@dblock I will work on better explanation on CHANGELOG.md, work on testing with alexa-app-server and updating UPGRADING.md
@tejashah88 I will add the usage of normalization

tejashah88 · 2017-02-14T23:57:29Z

index.js

-
-    options.expressApp.use(endpoint, router);
+    // In ExpressJS, user specifies their paths without the '/' prefix
+    var endpoint = options.endpoint;


@bgjehu This should be var endpoint = utils.normalizeApiPath(options.endpoint);

* added normalization of param 'endpoint' for alexa.express() with util fn * added upgrading guide for ExpressJS support in UPGRADING.md

bgjehu · 2017-02-15T01:30:32Z

@dblock @tejashah88

clarified changes in CHANGELOG.md
added normalization of param 'endpoint' for alexa.express() with util fn
added upgrading guide for ExpressJS support in UPGRADING.md

not related to the commit, I testedthe new logic with alexa-app-server, swap the alexa-app node_module to my fork for all the examples (including the invalid ones), all test passed. However, not changing anything in alexa-app-server for its package.json is point to alexa-app@3.0.0 instead of 4.0.0. I would apply the associated changes to alexa-app-server once it starts pointing to alexa-app@4.0.0

dblock · 2017-02-15T01:47:52Z

How do all our alexa-app-server tests pass when we're no longer attaching the router to the express instance? :)

bgjehu · 2017-02-15T02:20:18Z

I had made related changes in alexa-app-server/index.js as well before I ran the test. They didn't just pass without changes for the usage of alexa.express() would be different.

…lexa.express()

tejashah88 · 2017-02-15T03:19:28Z

index.js

-      throw new Error("You must specify an express router to attach.");
+    if (options.expressApp && options.router) {
+      console.warn("Since alexa-app@4.0.0, you are no longer required to pass both 'expressApp' and 'router'.");
+      console.warn("When both passed, 'alexa-app' would be attached to 'expressApp' instance.");


IMO, the warning message I would have done is something along the lines of "'expressApp' and 'router' are both specified, using 'expressApp'...". Generally, I want to give the user just the warning itself, and if they want to know more about it, they could look up UPGRADING.md for more information.

bgjehu · 2017-02-15T04:08:33Z

index.js

@@ -566,8 +566,7 @@ alexa.app = function(name) {
    }

    if (options.expressApp && options.router) {
-      console.warn("Since alexa-app@4.0.0, you are no longer required to pass both 'expressApp' and 'router'.");
-      console.warn("When both passed, 'alexa-app' would be attached to 'expressApp' instance.");
+      console.warn("Both 'expressApp' and 'router' are specified, attaching to 'expressApp' only.\nMore details on https://github.com/alexa-js/alexa-app/blob/master/UPGRADING.md");


@tejashah88 would this be better?

@bgjehu Sure. Let's see what @dblock has to say.

I think it should either be an exception or be backward compatible and produce a warning that this behavior is deprecated.

If we ignore router, the user is not getting the behavior from the previous release and we just tell them "sorry, it's not going to work, but we are going to pretend like it does" :)

I actually would prefer a backward compatible version here with a warning, because that allows a lot more flexibility in combining versions of alexa-app-server and alexa-app.

What do you think?

@dblock make sense. Making it backward compatible tonight.

dblock · 2017-02-15T12:42:39Z

CHANGELOG.md

@@ -1,6 +1,7 @@
 ## Changelog

 ### 4.0.0 (Next)
+* [#176](https://github.com/alexa-js/alexa-app/pull/176): Made params `expressApp` and `router` optional for `alexa.express()` - [@bgjehu](https://github.com/bgjehu).


That's not what this change does, one of the two is required. So maybe "Express integration now requires either expressApp or router, but not both"?

@dblock thanks for offering the words. I am not good with words and describing things! Will do the changes!

You're doing great, no stress!

dblock · 2017-02-15T12:44:01Z

README.md

 * `checkCert` when true, applies Alexa certificate checking _(default: true)_
 * `debug` when true, sets up the route to handle GET requests _(default: false)_
 * `preRequest` function to execute before every POST
 * `postRequest` function to execute after every POST

+Either `expressApp` or `router` has to be specified so that alexa-app could be attached to.


has to be specified so that alexa-app could be attached to => is required. I think it's clear what that does already from above.

@dblock will do

dblock · 2017-02-15T12:47:28Z

UPGRADING.md

@@ -39,6 +39,48 @@ app.intent("tellme", (request, response) => {
 });
 ```

+#### Changes to ExpressJS Support
+
+After changes, `alexa.express()` no longer requires both `expressApp` and `router` to be passes in. Instead, just pass in either an express app or router. And alexa-app would be bind to it.


That's not the whole change. Also it's unclear what alexa is, it should be app, and "to be passed in" is redundant. And the next sentence says the same thing. Maybe:

Express.js integration via `app.express()` now requires one of `expresApp` or `router`. When using a router, it will no longer be automatically mounted inside the express app and you may need to implement that outside of this call.

@dblock will do!

dblock · 2017-02-15T12:50:30Z

test/test_utils.js

@@ -0,0 +1,83 @@
+/*jshint expr: true*/


Lets delete unused functions here? The only one utilized is normalizeApiPath.

@dblock yes sir

dblock · 2017-02-15T12:50:57Z

I made some comments. Sorry to be a pest, I want to make sure we don't break a bunch of users.

bgjehu · 2017-02-16T03:14:11Z

@dblock @tejashah88
Backward compatible tested with alexa-app-server

dblock · 2017-02-16T13:04:52Z

I'm going to merge this, nice work on a spec, too!

I'd appreciate a follow-up pull request that:

Deletes unused functions in utils.
Slightly tweaks UPGRADING to say that the behavior with both is deprecated rather than removed.

dblock · 2017-02-16T13:06:13Z

@tejashah88 Interested in cutting a minor release with this change?

bgjehu · 2017-02-16T15:38:42Z

@dblock there is unused functional in utils? I saw the coverage for utils.js is 100%
will tweaks UPGRADING

dblock · 2017-02-16T15:59:06Z

coverage means there're tests, but half the functions in utils aren't actually used, eg. isValidDirectory, other than by code in those tests

bgjehu · 2017-02-17T00:18:28Z

right right right, silly me. I will double check and remove the unused

tejashah88 · 2017-02-17T03:09:00Z

@dblock We can do a release, however the CHANGELOG states that version 3.1.2 should be released today (2/16/2017).

tejashah88 · 2017-02-17T03:15:53Z

We should either go to 3.1.1 or 4.0.0, or wait until all relevant PRs are merged before deciding.

dblock · 2017-02-17T03:37:23Z

The CHANGELOG is wrong, it shouldn't have been updated. The current release is still 3.1.0.

bgjehu and others added 8 commits February 13, 2017 10:52

flexibility for attaching alexa to express

bac4131

increase flexibility for attaching alexa to either an express app or router

Merge pull request #1 from bgjehu/flexibility-attaching-to-express

3716678

flexibility for attaching alexa to express

bug fix and update tests

4f18868

update CHANGELOG, README, express example and version in package.json

5198073

Merge remote-tracking branch 'origin/master' into flexibility-attachi…

c8e04f6

…ng-to-express # Conflicts: # CHANGELOG.md # package.json

update test

ebe293a

update CHANGELOG

29125bf

Merge pull request #2 from bgjehu/master

b352fb1

Merge pull request #1 from bgjehu/flexibility-attaching-to-express

bgjehu mentioned this pull request Feb 13, 2017

add flexibility for attaching alexa-app to express #176

Closed

dblock approved these changes Feb 14, 2017

View reviewed changes

Added utilities

0b7ac4e

tejashah88 mentioned this pull request Feb 14, 2017

Added utilities #180

Closed

tejashah88 and others added 3 commits February 14, 2017 07:23

fixed istanbul in node v0.12 breaking build with non-js files

109dd7c

Merge branch 'flexibility-attaching-to-express' into add-utils

61e9633

Merge pull request #3 from tejashah88/add-utils

2749c88

Add utilities

tejashah88 reviewed Feb 14, 2017

View reviewed changes

* clarified changes in CHANGELOG.md

e2ca8c3

* added normalization of param 'endpoint' for alexa.express() with util fn * added upgrading guide for ExpressJS support in UPGRADING.md

minor name change from 'util' to 'utils'

016a8cf

tejashah88 approved these changes Feb 15, 2017

View reviewed changes

added warning when 'expressApp' and 'router' are both specified for a…

741e414

…lexa.express()

tejashah88 reviewed Feb 15, 2017

View reviewed changes

modified warning message

6b66c00

bgjehu commented Feb 15, 2017

View reviewed changes

dblock reviewed Feb 15, 2017

View reviewed changes

added backward compatible and update documentations

f516611

dblock merged commit b30997f into alexa-js:master Feb 16, 2017

dblock mentioned this pull request Feb 17, 2017

The next version will be 3.2.0. #186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding flexibility for attaching alexa-app to express #177

adding flexibility for attaching alexa-app to express #177

bgjehu commented Feb 13, 2017

dblock Feb 14, 2017

dblock commented Feb 14, 2017

tejashah88 commented Feb 14, 2017 •

edited

Loading

bgjehu commented Feb 14, 2017

tejashah88 Feb 14, 2017

bgjehu commented Feb 15, 2017 •

edited

Loading

dblock commented Feb 15, 2017

bgjehu commented Feb 15, 2017

tejashah88 Feb 15, 2017

bgjehu Feb 15, 2017

tejashah88 Feb 15, 2017

dblock Feb 15, 2017

bgjehu Feb 15, 2017

dblock Feb 15, 2017

bgjehu Feb 15, 2017

dblock Feb 15, 2017

dblock Feb 15, 2017

bgjehu Feb 15, 2017

dblock Feb 15, 2017

bgjehu Feb 15, 2017

dblock Feb 15, 2017

bgjehu Feb 15, 2017

dblock commented Feb 15, 2017

bgjehu commented Feb 16, 2017

dblock commented Feb 16, 2017

dblock commented Feb 16, 2017

bgjehu commented Feb 16, 2017

dblock commented Feb 16, 2017 •

edited

Loading

bgjehu commented Feb 17, 2017

tejashah88 commented Feb 17, 2017 •

edited

Loading

tejashah88 commented Feb 17, 2017

dblock commented Feb 17, 2017

adding flexibility for attaching alexa-app to express #177

adding flexibility for attaching alexa-app to express #177

Conversation

bgjehu commented Feb 13, 2017

Choose a reason for hiding this comment

dblock commented Feb 14, 2017

tejashah88 commented Feb 14, 2017 • edited Loading

bgjehu commented Feb 14, 2017

Choose a reason for hiding this comment

bgjehu commented Feb 15, 2017 • edited Loading

dblock commented Feb 15, 2017

bgjehu commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Feb 15, 2017

bgjehu commented Feb 16, 2017

dblock commented Feb 16, 2017

dblock commented Feb 16, 2017

bgjehu commented Feb 16, 2017

dblock commented Feb 16, 2017 • edited Loading

bgjehu commented Feb 17, 2017

tejashah88 commented Feb 17, 2017 • edited Loading

tejashah88 commented Feb 17, 2017

dblock commented Feb 17, 2017

tejashah88 commented Feb 14, 2017 •

edited

Loading

bgjehu commented Feb 15, 2017 •

edited

Loading

dblock commented Feb 16, 2017 •

edited

Loading

tejashah88 commented Feb 17, 2017 •

edited

Loading