-
Notifications
You must be signed in to change notification settings - Fork 12
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
Allow to pass name/options as third argument in standalone mode #26
base: master
Are you sure you want to change the base?
Conversation
} | ||
Router.prototype.add = function (method, path, callback) { | ||
var hasOptions = (typeof callback !== 'function' && !Array.isArray(callback)); | ||
var callbacks = [].slice.call(arguments, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slicing arguments prevents the whole function from being optimized by v8. I think that instead we should change the function to expect four arguments (method, path, options, callbacks)
and, if options
is a function or an array, set callbacks to options (or [options]) and then set options to an empty object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know and my first version was using Bluebird workaround, but even then function was marked as not optimized. Instead, I've used what was in the code:
https://github.com/visualfanatic/named-routes/blob/d24b2538213279cca0437224e8afbc30d36c1338/router.js#L312
Thanks for putting together this PR!
|
Thanks for quick reply! I'll add a few new test cases and update the docs accordingly. |
Sorry for the delay, but the missing tests are now included. @alubbe, could you verify if everything is now good? Would it be okay if I would rewrite the tests (add more cases and split existing into smaller groups)? |
Could you please update the docs, as well? |
This PR changes how
router.add
works:Closes #25