Conversation
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
Have you checked if benchmarks are staying the same?
|
I would also seek to refactor out the 404 handling, override and route definition, if possible. |
I agree, but I think we should split this work in multiple pull requests, otherwise, we will end up to break any other ongoing contribution. |
|
|
||
| module.exports = ContentTypeParser | ||
| module.exports.buildContentTypeParser = buildContentTypeParser | ||
| module.exports.helpers = { |
There was a problem hiding this comment.
Are these helper functions exposed to the public API or just meant to be used internally? If they are going to be used externally I'd request we add some types; if not, no types are needed
There was a problem hiding this comment.
The public API is the big fastify object you can see in the main file :)
There was a problem hiding this comment.
@fastify/fastify please take a look :)
As said above I think we should refactor this file in multiple pull request, to avoid a complex review and destroy other people pull request with massive rebases.
The main focus of this pr was to create a fastify object that represents our public API and start reordering the internal functions.
ps: I would also recommend to see the resulting work in the working branch, since the diff is quite messy 😄
| } else if (!options.logger) { | ||
| const logger = Object.create(abstractLogging) | ||
| logger.child = () => logger | ||
| return [logger, false] |
There was a problem hiding this comment.
Using an object instead of an array would make this code easier to understand. With the array, it's not clear what false means here.
There was a problem hiding this comment.
Thank you for the feedback, but for now I'll leave it as is. By reading the code is easy to understand what that boolean is, and if this becomes a problem in the future I'll add some more doc.
Furthermore this syntax is becoming commonly used in JavaScript, so let's start embracing it :)
There was a problem hiding this comment.
I agree with @nwoltman. Just reading this function, I can't begin to tell you how this result will be used. Going over to fastify.js I can see that it is meant to be {logger, hasLogger}. I think this function should return {logger, hasLogger: false}.
See https://docs.google.com/document/d/1hWb-lQW4NSG9yRpyyiAA_9Ktytd5lypLnVLhPX9vamE/edit for information on array destructuring. I'm not opposed to array destructuring because of the doc, but it's something to keep in mind. I am simply opposed here due to legibility.
|
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. |
5 participants
Hello team!
As all of you know, our entry point is a mess! 😱
We worked really hard during the past year on adding new features and applied bugfixes, and we leave this file organization out in the cold.
This pr aims to start cleaning up this file. I can't do everything in this pr since it will be a massive change, furthermore, some work will be done in conjunction with #1503.
As you can see a test is failing, not sure how to fix it.Related: #1475
Checklist
npm run testandnpm run benchmark