Unhandled promise rejections & friends :)

[DanielSmedegaardBuus]

Just a couple of minor edits.

• err || !res.ok continues to dereference on err, even though it might be undefined (when res.ok is falsey). Also, string concatenation on an Error will trigger its toString() method, so the result when doing "dumb" concatenation is the same.
• + operand takes higher precedence than &&, so two error message string concatenations would return just the error object's message, without the intended prefix. I opted to continue the style from your own methods, discarding the truthy check on err and just concatenating it.
• The most important one is that some promise rejections weren't handled since promises are wrapped in promises rather than chaining, and so e.g. a thrown Error in authenticate (like when configuration options are missing or erroneous) would cause methods to never return, and a node log warning about the unhandled promise rejection.

Cheers! :)

[DanielSmedegaardBuus]

I'll fix the undefined error, and create a failing test. The test won't be this week, though, I have to finish a project today and am leaving for Luxembourg tomorrow, so it'll be next week :)

About the Promise thing. Yes, when an error is thrown inside a Promise, it's automagically converted to a rejection. But:

  getKey(set, kid) {
    return new Promise((resolve, reject) => {
      return this.authenticate().then(() => {
        request.get(`${this.endpoint}/keys/${set}/${kid}`).authBearer(this.token.token.access_token).end((err, res) => {
          if (err || !res.ok) {
            reject({ error: 'Could not retrieve validation key: ' + error })
            return
          }
          resolve(res.body.keys[0])
        })
      }, reject)
    })
  }

You're first returning a new Promise, on which you're passed in the resolve and reject functions. The inner Promise is returned by this function, but that doesn't chain it to the outer Promise, its return value does not apply to it, it'll just wait for resolve() or reject() to be called.

Then, if OAuth2.create() throws inside the authenticate() method, the Promise will catch it and call its own reject(err). However, since return this.authenticate(...) is only providing a callback to resolve, and there's no .catch() clause either, the reject(err) call from authenticate() ends up in the void.

You could also do return this.authenticate().then(...).catch(reject), and AFAICT just return that directly rather than wrapping it in the outer Promise, but I may be missing something...

[aeneasr]

Awesome, thanks for explaining that!

[DanielSmedegaardBuus]

Today was cake day, so I had a little while while eating my brownie, so the tests are up. I had to update jest to v20, since that's when expect.reject was introduced :)

[aeneasr]

Awesome, thank you for your contribution!

[DanielSmedegaardBuus]

Oh no, thank you for making this module :)