-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
refactor: clean fastify mainfile #6107
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
base: main
Are you sure you want to change the base?
refactor: clean fastify mainfile #6107
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.
Although I like the fact that we are seeking for improving readability of the codebase, especially if this helps people ease the onboarding to make their first or recurrent contributions; I'd like to also callout for being a bit careful of spawning files and nested folders.
Often, this jumps imposes a bit of overhead while reading a codebase causing a bit of burden to understand as you need to jump across several files just to see a single implementation (which the locality of the functions and other details can help).
My point for what I'd like to callout for to seek for a balance between split and file overhead.
I see your point and appreciate the feedback. It's always a balance between avoiding giant files/functions and not over-splitting things. In the code base overall, I don't think we need to span more files. I actually considered consolidating everything into a single That said, I'm generally more cautious about splitting existing functions, especially when it involves complex algorithms, as it can hurt readability. But I also think that smartly splitting functions with good documentation can help clarify the structure, almost like offering a step-by-step specification of the algorithm. I expect to be told when one of my proposals is view as unnecessary or has an impact on readability. |
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 can't find the "dismiss review" item on this PR. This approval is just me dismissing my review.
bb0f744
to
722cc15
Compare
I put all the setup functions in one file in ./lib so there is only one |
/** | ||
* Sets up the `.ready()` method that controls Fastify's boot lifecycle. | ||
* It ensures all onReady hooks are executed and returns a promise or accepts a callback. | ||
* | ||
* @param {import('../fastify.js').FastifyInstance} fastify | ||
* @returns {Function} The ready function | ||
*/ |
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 decided to keep the doc annotations simplistic, because strict typing with JsDoc is a bit cumbersome.
While studying the source code in depth, I found it somewhat difficult to follow how the various pieces are connected. I didn’t expect it to be trivial and overall it's reasonably comprehensible. But I believe we can improve clarity and structure with some targeted refactoring.
Scope
This PR is focusing on the main file
fastify.js
that currently spans nearly 1,000 lines. It contains a mix of responsibilities: options processing, instance methods, helper functions, and bootstrapping logic. This makes onboarding and maintenance harder than it needs to be.Refactoring Proposal
I offer to extract most of the setup logic into dedicated files under a new
lib/setup/
directory:(Update: use kebab-case)
Additionally, I'm moving some functions into files where their concerns are better aligned.
For example:
defaultRoute
andonBadUrl
→lib/route.js
addHook
→lib/hooks.js
Readability
To help navigate within
fastify.js
, I offer to add section markers to delineate the main phases of instance creation:And others (INSTANCE SETUP PHASE, INSTANCE METHODS, INTERNAL HELPERS...).
While such comments aren't a substitute for well-structured code, they provide helpful guidance given that we're still working within a single large factory function rather than splitting it into smaller modules.