Conversation
fastify.js
Outdated
| if (!opts.handler && typeof opts.schema === 'function') { | ||
| opts.handler = opts.schema | ||
| opts.schema = {} | ||
| } |
There was a problem hiding this comment.
This is not ok. This block should go within the get/post/put etc code. This does the same job, but it is unreadable, and it might allow further bugs down the road.
There was a problem hiding this comment.
Ok initially that code was inside the shorthand functions but then I put it there and not in the shorthand function to avoid code repeat.
I'll update this :)
mcollina
left a comment
There was a problem hiding this comment.
You should also add a missing schema test in post/put and get.
| if (!handler && typeof schema === 'function') { | ||
| handler = schema | ||
| schema = {} | ||
| } |
There was a problem hiding this comment.
we can probably put this into a little helper, something like route(fill({ method: 'PUT', url }, schema, handler)).
Or _route({ method: 'PUT', url }, schema, handler), which contains this logic.
The important piece is that it should not be applied to the options, but to the function arguments.
There was a problem hiding this comment.
Ok! I'll go with _route.
fastify.js
Outdated
| } | ||
| return route({ | ||
| method: 'PUT', | ||
| method: method, |
There was a problem hiding this comment.
you can use the shorthands for the options route({ method, url, schema, handler }).
There was a problem hiding this comment.
We haven't talked about this, the Node support will be ≥ v4?
There was a problem hiding this comment.
yes. We might even choose v6+ if needed. Currently the codebase only run on v6 because of the destructuring in tests.
There was a problem hiding this comment.
I've run the tests under node v4 and only throw.test.js failed (because of the destructuring assignment), with a super tiny fix we can make the code base v4 compatible.
Should we support v4? In my opinion, yes.
There was a problem hiding this comment.
let's finish this off (shorthands for properties works on v4 and v6), and send a PR to add v4 support to travis (I've just added it in master).
9d190ee to
bc64395
Compare
* update/cleanup tests * stop server in afterEach * fix verify
….20.0 Update fastify-cli to the latest version 🚀
|
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. |
No description provided.