alexa-js / alexa-app

A framework for Alexa (Amazon Echo) apps using Node.js
https://www.youtube.com/watch?v=pzM4jv7k7Rg
MIT License
1.03k stars 212 forks source link

Hard-coded "test" view in package index.js is a problem #336

Open roschler opened 6 years ago

roschler commented 6 years ago

PREFACE: Thanks many times over for creating this NPM package!

I had a bit of head-scratcher setting up my first Node.js app that uses this package. The first test I ran was trying to access my Node.js app from a browser. No matter what URL I tried I got the following error dump:

Failed to lookup view "test" in views directory "views"
Error: Failed to lookup view "test" in views directory "views"
at Function.render node_modules/express/lib/application.js:580:17)
at ServerResponse.render node_modules/express/lib/response.js:971:7)
at node_modules/alexa-app/index.js:841:15
at Layer.handle [as handle_request] node_modules/express/lib/router/layer.js:95:5)
at next node_modules/express/lib/router/route.js:137:13)
at Route.dispatch node_modules/express/lib/router/route.js:112:3)
at Layer.handle [as handle_request] node_modules/express/lib/router/layer.js:95:5)
at node_modules/express/lib/router/index.js:281:22
at Function.process_params node_modules/express/lib/router/index.js:335:12)
at next node_modules/express/lib/router/index.js:275:10)
at expressInit node_modules/express/lib/middleware/init.js:40:5)
at Layer.handle [as handle_request] node_modules/express/lib/router/layer.js:95:5)
at trim_prefix node_modules/express/lib/router/index.js:317:13)
at node_modules/express/lib/router/index.js:284:7
at Function.process_params node_modules/express/lib/router/index.js:335:12)
at next node_modules/express/lib/router/index.js:275:10)

This was confusing because it only happened when I initialized the alexa-app object and nowhere in my code is a path to a route or view named "test". This happens due to the code below found in the package index.js module, since this code fires when it's not a Alexa request and only when the debug option is used (i.e. - no schema or utterance is defined).

if (options.debug) {
  target.get(endpoint, function(req, res) {
    var schemaName = req.query['schemaType'] || 'intent';
    var schema = self.schemas[schemaName] || function() {};
    if (typeof req.query['schema'] != "undefined") {
      res.set('Content-Type', 'text/plain').send(schema());
    } else if (typeof req.query['utterances'] != "undefined") {
      res.set('Content-Type', 'text/plain').send(self.utterances());
    } else {
      res.render("test", {
        "app": self,
        "schema": schema(),
        "utterances": self.utterances()
      });
    }
  });
}

I realized after the fact that this is what you mean in the sample code comments about being able to respond to POST requests:

// now POST calls to /test in express will be handled by the app.request() function

But I think this needs a little more explanation since a typical Node.JS developer will be used to the usual set up of a route and a corresponding view in app.js. Perhaps this text would be better?:

/* With the above Alexa app options you will automatically be provided with a route that responds to POST requests to the "/test" document. However, if you use a typical view engine like Jade/Pug for example, you will need to set up a view named "test" to render the request or you will get an error from Express complaining about a missing view for "/test". For example, "test.jade" if you're view engine is Jade. /

Also, it would be helpful to give some people suggestions as what to put in the "view".

I think a better approach would be to change the Alexa app constructor to something like the following:

var alexa_app = require("alexa-app");
var alexaApp = new alexa_app.app(appName, testRouteName);

And explain to people that testRouteName is an optional parameter with the default name "test". Then modify the code in the package index.js. file to use that route name when setting up the route and to call render() with that route name; with the default value for testRouteName being "test" if testRouteName is undefined or NULL. This will give experienced Node.JS developers a clearer picture of what is going on.

Also, this saves the developer from having to rename any existing view named "test" if they already have one.

dblock commented 6 years ago

This makes sense, try to PR an explanation, maybe even remove the problematic code or provide a less confusing error?