fixes #1

[weichensw]

No description provided.

[coveralls]

Coverage remained the same at 100.0% when pulling 1e07c51 on xbtsw:master into 9586469 on omsmith:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 1e07c51 on xbtsw:master into 9586469 on omsmith:master.

[omsmith]

Is the errHandler because you'd provide a new one for use on the "upgrade" event?

I would also hesitate calling this an error handler, as it is simply a "default" or unmatched handler, yes?

Maybe we expose this as .default(fn);?

[omsmith]

It should be invalid to not have a default handler, so the check for if(this._errHandler) (or whatever new name) in the dispatcher is unnecessary

[weichensw]

the dispatch will call handler(arguments) since websocket will have 3 arguments (instead of res and req), this errHandler is called when there's no route matching, instead of res.end() directly, since in websocket, there's no res anymore.

So in a sense, it's a error (no route matched). The original behavior here is 404 and res.end(), which have the assumption of a http request.

[weichensw]

I agree the if(this._errHandler) is redundant now. Initially I didn't have a default for the errHandler so I have a if check when calling it. (i.e. I didn't errHandler || function(){...}) and later on I changed it into the current way. I will remove it.

[coveralls]

Coverage remained the same at 100.0% when pulling afbe013 on xbtsw:master into 9586469 on omsmith:master.

[omsmith]

Not on my phone anymore so perhaps I can write a more detailed answer.

Don't think errHandler is a good name here. An error handler should just be another way of specifying .on('error', listenerFn) and stop the error events from being raised.

This would be better exposed as a default handler. As that's the only "error" it handles.

For example:

PathRouter.prototype.default = function setDefaultHandler () {
  this._router.default.apply(this._router, arguments);
  return this;
}

function defaultHandler (req, res) { /* ... */ }

function Router () {
  // ...

 this._default = defaultHandler;
}

Router.prototype.default = function setDefaultHandler (fn) {
  if ('function' !== typeof fn) {
    throw new Error('"fn" must be a function');
  }

  this._default = fn;
}

Router.prototype.dispatch = function dispatch (req) {
  // ...

  this._default.apply(this, arguments);
}

If it doesn't matter to you - I could actually write that out and commit it with tests in place.

[omsmith]

I've got those changes in #3 which I'll merge once the tests pass.

I'll do the any-arity handler change after

[weichensw]

I see your point now. Yes I agree in this perspective it can be called default too.

I don't really mind the name, so long as the any-arity handler is there :)

Thanks for the help!