Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
roschler opened this issue Apr 22, 2018 · 1 comment
Open

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

roschler opened this issue Apr 22, 2018 · 1 comment
Labels

Comments

@roschler
Copy link

roschler commented Apr 22, 2018

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
Copy link
Collaborator

dblock commented Apr 22, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants