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

SyntaxError: Unexpected token u #124

Closed
skeggse opened this issue Mar 28, 2014 · 11 comments · May be fixed by #125
Closed

SyntaxError: Unexpected token u #124

skeggse opened this issue Mar 28, 2014 · 11 comments · May be fixed by #125

Comments

@skeggse
Copy link

skeggse commented Mar 28, 2014

I'm not sure what's causing this yet, apart from the obvious that JSON.parse is passed undefined. I'm not trying to do anything particularly fancy, I'll see if I can track it down or reduce it to a workable test-case.

Node v0.10.24 and v0.10.26.

undefined:1
undefined
^
SyntaxError: Unexpected token u
    at Object.parse (native)
    at unpack (/{REDACTED}/node_modules/axon/node_modules/amp-message/index.js:136:32)
    at decode (/{REDACTED}/node_modules/axon/node_modules/amp-message/index.js:83:15)
    at new Message (/{REDACTED}/node_modules/axon/node_modules/amp-message/index.js:35:37)
    at Parser.<anonymous> (/{REDACTED}/node_modules/axon/lib/sockets/sock.js:223:15)
    at Parser.EventEmitter.emit (events.js:95:17)
    at Parser._write (/{REDACTED}/node_modules/axon/node_modules/amp/lib/stream.js:91:16)
    at doWrite (_stream_writable.js:221:10)
    at writeOrBuffer (_stream_writable.js:211:5)
    at Parser.Writable.write (_stream_writable.js:180:11)
@tj
Copy link
Owner

tj commented Mar 28, 2014

hmm I don't think we really have any asserts in place (delegating that to the user) so you may want to add a few for data being sent. One thing for debugging is to add if (msg == null) console.trace('wut') to axon in your node_modules so you can see where it was sent

@tj tj closed this as completed Mar 28, 2014
@tj
Copy link
Owner

tj commented Mar 28, 2014

re-open if it turns out to be axon but I think there's just some input getting messed

@skeggse
Copy link
Author

skeggse commented Mar 28, 2014

I had to step away but before I left I think I discovered that the buffer slice method is returning undefined! I really hope that isn't the case, I'll know more in a bit.

@tj
Copy link
Owner

tj commented Mar 28, 2014

interesting! that would be weird, let me know if you find anything

@skeggse
Copy link
Author

skeggse commented Mar 28, 2014

Ha silly human make mistake.

Don't mind me, just...passing undefined to the send method.

@skeggse
Copy link
Author

skeggse commented Mar 28, 2014

It seems like it would be valuable to catch errors thrown during the deserialization process and enable the user of axon to catch those errors, rather than allowing the errors to halt the process. If I pulled something of that sort together, would you consider a pull request?

@tj
Copy link
Owner

tj commented Mar 28, 2014

we should add asserts on input, and assume everything that gets in is valid. Even just assert(msg, 'message required')-ish asserts would be fine

@gjohnson
Copy link
Collaborator

+1 to assert. gogogo!

@skeggse
Copy link
Author

skeggse commented Mar 28, 2014

Alright, so I can make the tests fail by adding push.send(undefined); to line 16 of test.arg-types.js, simply because it causes the Unexpected token u SyntaxError.

The message is then encoded as a buffer containing 110000000b6a3a756e646566696e6564, which includes j:undefined. That means that, as confirmed by walking through with Node's debugger, the JSON.stringify line in node-amp-message is being passed undefined, which returns undefined, which is coerced into the string 'undefined'. I'm wondering whether this would be more suited for amp-message rather than axon, given that we're not currently looping through the args before buffering them in Socket#pack.

Also, given that send usually queues messages until the socket is actually ready, the error won't be thrown until the socket is ready which means I can't use assert.throws in the test case.

Anywho, I submitted pull request #125, but it'll need some revision.

@tj
Copy link
Owner

tj commented Mar 28, 2014

for amp-message I'd almost rather just convert it to null and send that, since it's a valid value, just not ideal since it usually represents a user error. Maybe we should do that instead, there's nothing really wrong with sending undefined

@skeggse
Copy link
Author

skeggse commented Mar 28, 2014

Sounds good to me, 👍 for simplicity.

Alternatively, if we don't want to break some weird fringe case that somehow depends on the value being exactly undefined, we could just check if the arguments is equal to the string 'undefined' during the decode process.

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

Successfully merging a pull request may close this issue.

3 participants