Use error handler

[mjsalinger]

Add a new option, useErrorHandler which will invoke next(err) if set to true on any errors. If not set or set to false, the library will continue to send the response.

[etodanik]

Is there any reason this isn't merged? Any way to help?

[etodanik]

On second thought, after testing this it doesn't work at all.
On this line:
if (this.useErrorHandler === true) {

this is the actual global node environment? How does that make sense?

Also, this:

    if (e instanceof UnauthorizedRequestError) {
      return res.status(e.code);
    }

Just leaves the request hanging without resolving it ever at all.

[visvk]

This code binds this to the handleError function.

ExpressOAuthServer.prototype.token = function(options) {
  return (function(req, res, next) {
    var request = new Request(req);
    var response = new Response(res);

    return Promise.bind(this)
      .then(function() {
        return this.server.token(request, response, options);
      })
      .tap(function(token) {
        res.locals.oauth = { token: token };
      })
      .then(function() {
        return handleResponse(req, res, response);
      })
      .catch(function(e) {
        return handleError.call(this, e, req, res, response, next);
      })
      .finally(next);
  }).bind(this);
};

[maxtruxa]

See review comments.

[maxtruxa]

return res.status(e.code); leaves the response hanging.

How about:

res.status(e.code);

if (e instanceof UnauthorizedRequestError) {
  res.send();
} else {
  res.send({ error: e.name, error_description: e.message });
}

[maxtruxa]

Down here, this === global.

We need to either pass useErrorHandler as an argument or invoke handleError (and to be consistent handleResponse) using handleError.call(this, ...).

[mjsalinger]

Good catch, updating to use handlerError.call(this, ...) and handleResponse.call(this, ...)

[visvk]

This is not enough. It's nested function, so there will not be this === ExpressOAuthServer value until you bind this to the nested function.
I recommend to use strict mode, so you will see this problem clearly.

[maxtruxa]

Oh, I just realized what you're talking about. The problem is that this is already wrong by the time handleError.call(this, ...) is called. Promise.bind(this) is supposed to help with that, but this refers to the returned middleware.

This can be solved by changing

var server = this.server;

to

var that = this;

and

return Promise.bind(this)

to

return Promise.bind(that)

in all 3 handlers.

Obviously references to server have to be replaced by this.server after this change.

[mjsalinger]

Yep, I see it.. Pushing changes...

[maxtruxa]

See this review comment

[maxtruxa]

See review comments.

[maxtruxa]

There's two spaces missing.