Fix "Cannot read property 'new' of undefined" init error #91

fremail · 2016-12-19T17:04:51Z

The error is caused on getting AudioPlayer events. I got for these events: AudioPlayer.PlaybackStarted, AudioPlayer.PlaybackNearlyFinished and AudioPlayer.PlaybackFinished.

{
    "errorMessage": "Cannot read property 'new' of undefined",
    "errorType": "TypeError",
    "stackTrace": [
        "new alexa.request (/var/task/node_modules/alexa-app/index.js:219:27)",
        "/var/task/node_modules/alexa-app/index.js:317:18",
        "tryCatcher (/var/task/node_modules/alexa-app/node_modules/bluebird/js/main/util.js:26:23)",
        "Promise._resolveFromResolver (/var/task/node_modules/alexa-app/node_modules/bluebird/js/main/promise.js:483:31)",
        "new Promise (/var/task/node_modules/alexa-app/node_modules/bluebird/js/main/promise.js:71:37)",
        "request (/var/task/node_modules/alexa-app/index.js:316:10)",
        "handler (/var/task/node_modules/alexa-app/index.js:448:8)"
    ]
}

As Amazon wrote:

The session object is included for all standard requests, but it is not included for AudioPlayer or PlaybackController requests.

Additional info regarding to this error you can find in PR #88 and Issue #78

coveralls · 2016-12-19T17:06:02Z

Coverage decreased (-0.3%) to 80.734% when pulling c5debe1 on fremail:master into 819e0c8 on matt-kruse:master.

dblock · 2016-12-19T18:13:46Z

This needs a test and a CHANGELOG entry, please?

dblock · 2016-12-19T18:14:16Z

index.js

@@ -145,6 +145,14 @@ alexa.request = function(json) {
      return null;
    }
  };
+  // session is not included for AudioPlayer or PlaybackController requests


Maybe add the https://developer.amazon.com/public/solutions/alexa/alexa-skills-kit/docs/alexa-skills-kit-interface-reference#request-body-parameters link here too?

dblock · 2016-12-19T18:16:59Z

Before we merge this - since the Alexa requests don't include session objects for these audio ones, maybe we shouldn't be making a session object to work around that, but being more defensive? Otherwise we're creating something not supported by the platform, no?

fremail · 2016-12-19T19:36:51Z

Actually I agree with you, we shouldn't provide session interface in case of its absence.

We can set request.userId, request.applicationId, etc. from request context object. But what will we do with request.session() function? Will we define it in that case?

dblock · 2016-12-19T20:32:56Z

Maybe we should throw an error when you access session() in this case?

Deny using session when the Alexa requests don't include session objects

coveralls · 2016-12-20T13:55:10Z

Coverage decreased (-44.9%) to 36.087% when pulling 82615b7 on fremail:master into 819e0c8 on matt-kruse:master.

dblock · 2016-12-20T14:05:36Z

index.js

    } else {
-      this.response.sessionAttributes[key] = val;
+      throw "NO_SESSION";


Is this a pattern we use elsewhere? Maybe throw a more helpful message?

Oh scratch that, I see it below.

dblock · 2016-12-20T14:06:41Z

index.js

@@ -180,6 +204,9 @@ alexa.app = function(name, endpoint) {
    "NO_LAUNCH_FUNCTION": "Try telling the application what to do instead of opening it",
    // When a request type was not recognized
    "INVALID_REQUEST_TYPE": "Error: not a valid request",
+    // When a request and response don't contain session object
+    // https://developer.amazon.com/public/solutions/alexa/alexa-skills-kit/docs/alexa-skills-kit-interface-reference#request-body-parameters
+    "NO_SESSION": "There is not session to store or get values",


Maybe "this request doesn't support session attributes" or something like that?

fremail · 2016-12-20T14:08:00Z

Sorry I didn't write tests, dunno how to do it.
And it seems existed tests failed because the test requests don't contain context object, though should have it. Link

The idea of my changes:
Deny using session in request and response objects, so all functions using it will throw "NO_SESSION". To check if you can use those functions you can call request.hasSession() or response.hasSession()

dblock · 2016-12-20T14:09:16Z

index.js

@@ -3,15 +3,20 @@ var AlexaUtterances = require("alexa-utterances");
 var SSML = require("./to-ssml");
 var alexa = {};

-alexa.response = function() {
+alexa.response = function(hasSession) {


I think there's a cleaner way to do this. The response could be aware of the session instead of whether the request "has a session". So the session would be constructed in the request object and the response would get the actual session here. This way sessionAttributes moves the new session object as attributes and can abstracts away a bunch of functions such as the implementation inside this.session here making things better organized and a lot cleaner.

As I see your idea:

request.session = { this.get = function(key) {...}; this.set = function(key, value) {...}; this.clear = function() {...}; this.details = {...}; this.attributes = {...}; this.isNew = bool; }

Just don't understand why we need to store the same data in this.sessionDetails and in properties of this.

BTW, these changes will break compatibility.

And we need to edit response.response from the session object. Don't sure how we can do that.

I haven't actually written the code, give it a shot. Obviously backwards compat breaking is bad, maybe we can minimize that once we see what it looks like.

Maybe we need to move session outside of request and response since it's an independent object not related to request or response instantly.
I'd even say it's between them.
So we can provide it as an alone object:

app.pre = function(request, response, type, session) { ... }; app.intent("sampleIntent", { "slots": {...}, "utterances": [...] }, function(request, response, session) { ... } );

And call session methods from existed methods of request and response for backwards compatibility (and mark them as deprecated).

dblock · 2016-12-20T14:10:57Z

See my comment above wrt hasSession and I can help you with tests. Start by having the tests that exist pass (npm test), then try to add a test that checks that given one of those requests that causes a session not to exist you can get a response. Then add a test that lands in the throw part with NO_SESSION.

dblock · 2016-12-20T14:11:33Z

And thanks for the hard work! Hang in there we'll get this to a mergeable state soon.

Mark all moved to session object properties and methods as deprecated Update test request json Update readme

coveralls · 2016-12-21T15:38:48Z

Coverage increased (+0.6%) to 81.667% when pulling 0091c3c on fremail:master into 819e0c8 on matt-kruse:master.

fremail · 2016-12-21T15:48:58Z

So I separate session object from alexa.request and alexa.response - now it's alexa.session.

These methods work for 100% backwards compatibility, but marked as deprecated:

request.session()
response.session()
response.clearSession()

The same for these properties:

request.sessionDetails
request.sessionId
request.sessionAttributes
request.isSessionNew

I passed session as the last argument for all handlers (pre, post, intent, error, etc.) for backwards compatibility.

Now it needs to do response.prepare() to save sessionAttributes into response.response object.

dblock

I think this is very close. I would move this last session argument into being available via request.session() or requet.getSession() whatever is more appropriate and then the interface doesn't need to change much. What do you think?

dblock · 2016-12-21T16:00:45Z

README.md

+## session
+```javascript
+// check if you can use session (read or write)
+Boolean session.isAvailable()


It's a little strange to ask the session whether it's available. Why wouldn't this be on request.hasSession()? And then I think we don't need to change the first function because request.session would be a thing?

Do you want to make a gettter (request.getSession()) in request object for getting session object so we don't need to pass session as a last argument to handlers?
And check the session via request.hasSession().

So using the session would be:

if (request.hasSession()) { var session = request.getSession(); session.set("fruit", "apple"); console.log(session.get("fruit")); }

Yes! This is exactly it.

Remove session from last arguments of handler functions

coveralls · 2016-12-21T21:05:43Z

Coverage increased (+0.5%) to 81.557% when pulling 9168601 on fremail:master into 819e0c8 on matt-kruse:master.

dblock · 2016-12-21T21:23:40Z

The code looks perfect. Just need two tests: for the loop that involves no session and another that ends up in that NO_SESSION exception being thrown.

dblock · 2016-12-21T21:24:45Z

Lets also add an UPGRADING.md document that describes the methods that have been deprecated. For a good example see https://github.com/ruby-grape/grape/blob/master/UPGRADING.md

Fix an error - NO_SESSION exception on start of app Add UPGRADING.md

coveralls · 2016-12-22T12:59:55Z

Coverage increased (+4.2%) to 85.246% when pulling 708461d on fremail:master into 819e0c8 on matt-kruse:master.

dblock

Perfect, merging. Nice work!

dblock · 2016-12-22T13:08:33Z

Merged squashed. You'll have to fix/rewind/rebase your master branch (you should start working on branches). See http://code.dblock.org/2015/08/31/getting-out-of-your-first-git-mess.html that might be helpful.

fremail · 2016-12-22T13:20:44Z

Woohoo!
Will read it, thanks

fremail · 2016-12-22T18:21:53Z

I cleaned up my master branch, now it has only one commit for this PR, but the commit is after merge of this PR :(

dblock · 2016-12-22T19:11:02Z

@fremail

git checkout master
git checkout -b backup-master
git checkout master
git reset HEAD~50 --hard
git pull upstream master
git push origin master -f

fix "Cannot read property 'new' of undefined" init error

c5debe1

dblock mentioned this pull request Dec 19, 2016

Warn doesn't seem to add anything to pull requests danger/danger-js#52

Closed

dblock reviewed Dec 19, 2016

View reviewed changes

Add hasSession() functions to request and response objects

82615b7

Deny using session when the Alexa requests don't include session objects

dblock reviewed Dec 20, 2016

View reviewed changes

Separate session object from request and response objects

0091c3c

Mark all moved to session object properties and methods as deprecated Update test request json Update readme

dblock requested changes Dec 21, 2016

View reviewed changes

Add request.hasSession() and request.getSession()

9168601

Remove session from last arguments of handler functions

Add tests for a new session object

708461d

Fix an error - NO_SESSION exception on start of app Add UPGRADING.md

dblock approved these changes Dec 22, 2016

View reviewed changes

dblock merged commit e2b8c6c into alexa-js:master Dec 22, 2016

dblock mentioned this pull request Dec 22, 2016

AudioPlayer functionality #88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "Cannot read property 'new' of undefined" init error #91

Fix "Cannot read property 'new' of undefined" init error #91

fremail commented Dec 19, 2016

coveralls commented Dec 19, 2016

dblock commented Dec 19, 2016

dblock Dec 19, 2016

dblock commented Dec 19, 2016

fremail commented Dec 19, 2016

dblock commented Dec 19, 2016

coveralls commented Dec 20, 2016

dblock Dec 20, 2016

dblock Dec 20, 2016

dblock Dec 20, 2016

fremail commented Dec 20, 2016

dblock Dec 20, 2016

fremail Dec 20, 2016

dblock Dec 20, 2016

fremail Dec 21, 2016 •

edited

Loading

dblock commented Dec 20, 2016

dblock commented Dec 20, 2016

coveralls commented Dec 21, 2016

fremail commented Dec 21, 2016 •

edited

Loading

dblock left a comment

dblock Dec 21, 2016

fremail Dec 21, 2016 •

edited

Loading

dblock Dec 21, 2016

coveralls commented Dec 21, 2016

dblock commented Dec 21, 2016

dblock commented Dec 21, 2016

coveralls commented Dec 22, 2016

dblock left a comment

dblock commented Dec 22, 2016

fremail commented Dec 22, 2016

fremail commented Dec 22, 2016 •

edited

Loading

dblock commented Dec 22, 2016

Fix "Cannot read property 'new' of undefined" init error #91

Fix "Cannot read property 'new' of undefined" init error #91

Conversation

fremail commented Dec 19, 2016

coveralls commented Dec 19, 2016

dblock commented Dec 19, 2016

Choose a reason for hiding this comment

dblock commented Dec 19, 2016

fremail commented Dec 19, 2016

dblock commented Dec 19, 2016

coveralls commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fremail commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fremail Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

dblock commented Dec 20, 2016

dblock commented Dec 20, 2016

coveralls commented Dec 21, 2016

fremail commented Dec 21, 2016 • edited Loading

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fremail Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 21, 2016

dblock commented Dec 21, 2016

dblock commented Dec 21, 2016

coveralls commented Dec 22, 2016

dblock left a comment

Choose a reason for hiding this comment

dblock commented Dec 22, 2016

fremail commented Dec 22, 2016

fremail commented Dec 22, 2016 • edited Loading

dblock commented Dec 22, 2016

fremail Dec 21, 2016 •

edited

Loading

fremail commented Dec 21, 2016 •

edited

Loading

fremail Dec 21, 2016 •

edited

Loading

fremail commented Dec 22, 2016 •

edited

Loading