Node-jsonwebtoken: async .sign throws instead of callback with an error object

[mdebruijne]

In version 6.0.0 .sign is changed to an async callback with an error argument. This is really appreciated so many thanks for this. However, instead of using an error object in the callback, errors are thrown.

Two examples;

'use strict'

const jwt = require('jsonwebtoken')

jwt.sign('string', 'secret', {noTimestamp: true}, (err, token) => {
  console.log(err)
  console.log(token)
})

'use strict'

const jwt = require('jsonwebtoken')

jwt.sign({key: 'value'}, 'secret', {algorithm: 'invalid'}, (err, token) => {
  console.log(err)
  console.log(token)
})

Expected result;
Return an error object in the callback and don't throw

Actual result;
Errors are thrown and callback isn't called

Tested with version 6.1.0

[jfromaniello]

I have followed node.js core practices to validate arguments.

This is an example of async function that throws an error on an invalid parameter:

> var fs = require('fs');
> fs.readFile({}, function (err) {console.log('there was an error')})
TypeError: path must be a string
    at TypeError (native)
    at Object.fs.readFile (fs.js:250:11)
    at repl:1:4
    at REPLServer.defaultEval (repl.js:262:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:431:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:211:10)

here is another example with the crypto module:

> var crypto = require('crypto');
> crypto.pbkdf2('foobar', 'foobar', 5, 2, 'invalid', function () {})
TypeError: Bad digest name
    at TypeError (native)
    at pbkdf2 (crypto.js:562:20)
    at Object.exports.pbkdf2 (crypto.js:548:10)
    at repl:1:8
    at REPLServer.defaultEval (repl.js:262:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:431:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)

[simon-p-r]

Yes as per issue #180 it would be more useful to return error rather than throw, only throw if there is no callback provided.

[mdebruijne]

@jfromaniello

Thank you for your explanation. Unfortunately there is a lot of inconsistency regarding error handling in the Node.js ecosystem, but I fully agree with your reasoning.

I hope you don't mind asking me one more question before closing this issue; do you have an example where jsonwebtoken utilizes the new async callback with an error argument?

[jfromaniello]

Let's vote with 👍 and 👎 reactions in this comment (I will wait 24hs). I am not entirely convinced of my reasoning either to be honest.

• 👍 means return err in callback always and never throw.
• 👎 means throw an error on invalid args

[mdebruijne]

Personally I prefer to keep the Node.js Core practice if it is documented correctly, but at this moment the documentation isn't correct and complete.

In the documentation are examples that mention it will use the error callback, but they actually throw. Also the other way around, where examples that mention it throws are actually returned with an error callback.

Some things are not documented at all, for example when error callbacks occur with jwt.sign (see my previous question). Another example; the application throws if the algorithm doesn't match. This got me confused and therefore I created this issue, because I misunderstood the desired behavior.

I think an application should not throw, but instead propagate error asynchronously if the conditions are not documented explicitly.

[smulube]

Hi, is this still a live issue? I entirely agree with the last comment by @mdebruijne - I think the most important thing is that the documentation shouldn't say one thing will happen but then do another, i just spent a few headscratching minutes wondering what I'd done wrong.

I don't know how official this is, but I think this is a good reference on error handling in Node: https://www.joyent.com/developers/node/design/errors which contains the following guidelines:

The general rule is that a function may deliver operational errors synchronously (e.g., by throwing) or asynchronously (by passing them to a callback or emitting error on an EventEmitter), but it should not do both. This way, a user can handle errors by either handling them in the callback or using try/catch, but they never need to do both. Which one they use depends on what how the function delivers its errors, and that should be specified with its documentation.

However I guess my vote would have been to propagate the error via callback if one has been supplied, and throw if the function is being called synchronously.

[jfromaniello]

Fixed in v7.0.0. Thanks for all the feedback

[mdebruijne]

Thanks @jfromaniello !

[jfromaniello]

@mdebruijne no problem, sorry for the delay

[smulube]

thanks, that's great!