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

router.bind is overridden as an http method #41

Open
wesleytodd opened this issue May 19, 2016 · 2 comments
Open

router.bind is overridden as an http method #41

wesleytodd opened this issue May 19, 2016 · 2 comments
Labels

Comments

@wesleytodd
Copy link
Member

Not sure if this is a new things in node 6, I'm running 6.1.0, but the core http.methods include the http BIND method. When we loop the methods we end up overriding function.bind on the router instance causing weird behavior. The test that broke was as follows:

it('should reject missing callback', function () {
  var router = new Router()
  assert.throws(router.bind(router, {}, {}), /argument callback is required/)
})

The intent here is obvious, but it ended up calling the method setup here.

There are three solutions I can see:

  1. Filter out known bad names (aka blacklist)
  2. Only generate known good methods (aka whitelist)
  3. Document the fact that we overrode .bind

I can make a PR either way but wanted to open the discussion first.

Also for reference, here is the list I got by logging the methods array:

[ 'acl',
  'bind',
  'checkout',
  'connect',
  'copy',
  'delete',
  'get',
  'head',
  'link',
  'lock',
  'm-search',
  'merge',
  'mkactivity',
  'mkcalendar',
  'mkcol',
  'move',
  'notify',
  'options',
  'patch',
  'post',
  'propfind',
  'proppatch',
  'purge',
  'put',
  'rebind',
  'report',
  'search',
  'subscribe',
  'trace',
  'unbind',
  'unlink',
  'unlock',
  'unsubscribe' ]
@dougwilson
Copy link
Contributor

So since I noticed this from a 5.x release back in January, I've through a bunch about it, because it affects this module and Express itself. I think that if we want to continue to do router[method] style, the methods should be a static list, not dynamic, and we should have an additional API that takes a method name so people can use any method they want to use as well. This would also have the benefit for things like TypeScript and JSDoc comments to actually function with the methods, since they won't be auto-generated.

@wesleytodd
Copy link
Member Author

Ok, so maybe we should bet a 2.0 roadmap that includes this and #42 at the minimum. I can work on this over the next few days and put together the PR.

I will open a new issue to discuss supporting something where the method is passed in so we can focus this issue on specifying the static method list.

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

No branches or pull requests

2 participants