Add support for handler in shorthand route options. #843
Conversation
docs/Routes.md
Outdated
| fastify.get('/', opts) | ||
| ``` | ||
|
|
||
| > Note: if the handler is specified in both the `options` and as the third parameter to the shortcut method then the third parameter takes precedence over the the value supplied in the `options`. |
There was a problem hiding this comment.
I don't like "magic" fallback. For me it's better throw an error if handler options and the third params are given
|
The answer on what should happen if the handler appears in both places depends on what situation we think the user is in when this happens. If the user has accidentally put the handler function in both places without realizing it, we should throw an error to let them know they made a mistake. But if the user is doing this on purpose and expects I'm not sure which situation would be the most common. I also think it would be very rare for the handler to be passed in both places at the same time so I'd be fine with either 1 or 2. |
|
What I mean is: Ok: fastify.get('/', optionsWithoutHandler, handler)Ok: fastify.get('/', optionsWithHandler)Throw: fastify.get('/', optionsWithHandler, handler)With this PR, the last snippet is valid. In that way I'm for 2 |
|
OK, I'll update to the 2rd way. |
|
Funny, there was already a test that says it's okay for the handler to be overwritten. @shuperry Yes that test should be changed if we're going with option 2. |
|
We will need a few other throws, I will push later. |
… third and in options.
|
@allevo I updated to option 2, let's discuss. |
…hand route system configuration test.
nwoltman
left a comment
There was a problem hiding this comment.
This is missing a test for the new feature. Currently there are only tests for the new error throwing.
|
@nwoltman I already add test |
test/hooks.test.js
Outdated
| const fastify = Fastify() | ||
| fastify.register((instance, opts, next) => { | ||
| instance.addHook('onRoute', function (route) { | ||
| t.strictEqual(route.handler.length, 2) |
There was a problem hiding this comment.
This doesn't test that it's the right handler (it just tests that it is some function with a length of 2). The handler should be created outside of the options object so that the comparison can be:
t.strictEqual(route.handler, handler)There was a problem hiding this comment.
@nwoltman I updated the tests, see the code again please.
|
@allevo I'm waiting to end this PR. |
|
Thanks for the patience @shuperry! Landed now! |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Checklist