-
Notifications
You must be signed in to change notification settings - Fork 141
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
add nonstandard http methods #178
Conversation
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.
lgtm
Co-authored-by: Manuel Spigolon <[email protected]>
Co-authored-by: Manuel Spigolon <[email protected]>
Co-authored-by: Ayoub El Khattabi <[email protected]>
Co-authored-by: Tomas Della Vedova <[email protected]>
@cmawhorter It would make more sense to me if the default behavior for this change is that the methods provided by the user extend the default router methods instead of replacing them. Is there some scenario where none of the default methods is needed? I'm curious about your use case. |
@AyoubElk find-my-way is a solid, lightweight general purpose router without any real dependency on http (just a couple lines of code that are easy to work around). i'm using it entirely outside of http with aws lambda for routing rpc calls and it's working great. i consciously decided not to extend because i reasoned that if someone is customizing, it's better to be explicit and pretty trivial to add the missing methods back in. plus this way has the added benefit of allowing people to remove standard methods they don't want to support. |
That makes sense, thanks for the explanation! |
* update ConstraintStrategy typing * add tests for custom constraints with objects as values * remove constraint with objects test * add type tests for custom contraints * update variable names of the constraint strategy interface
test/methods.test.js
Outdated
@@ -80,6 +91,17 @@ test('does not register /test/*/ when ignoreTrailingSlash is true', t => { | |||
) | |||
}) | |||
|
|||
test('defaults to built-in http METHODS for invalid option', t => { | |||
t.plan(1) | |||
const findMyWay = FindMyWay({ httpMethods: 'invalid' }) |
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.
I think this should throw :)
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.
i did it this way for backwards compat with people that might possibly (incorrectly) be passing an options object with httpMethods. unlikely, but i opted to keep the current behavior which is nothing. i agree it should throw though and i'm fine either way.
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.
Let's throw, we can't save everyone :)
In my opinion it's always better to be explicit and fail hard rather than silently fail or silently not doing what you were expecting.
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.
i'm happy to make the change and agree with the sentiment. this is just changing the contract which always makes me nervous. if you're not worried though, i'm not worried.
Co-authored-by: Manuel Spigolon <[email protected]>
Co-authored-by: Manuel Spigolon <[email protected]>
rebased and merged my changes back in. i also added some additional docs and tests to verify things working correctly |
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.
I fear something went wrong while merging with master, I see commits that should not be here. Can you revert what you did? Otherwise doing the review is rather complex.
Thanks!
I'll take a look when I get a chance. I made a half-hearted attempt to rebase and failed miserably most likely. Sorry about that. |
Closing for no activity, feel free to reopen when you have time! |
Closes #177.
Summary: Added Router option
httpMethods
. If not provided or the provided value is not an array, defaults to built-in http.METHODSEdit: Forgot about ts so added support there in a bc way. Also added docs and another test to make sure shorthands throw.