Skip to content

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jean-michelet
Copy link
Member

@jean-michelet jean-michelet commented May 4, 2025

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)

lib/
└── setup/
    ├── setup-avvio.js
    ├── setup-client-error-handler.js
    ├── setup-inject.js
    └── setup-ready.js

Additionally, I'm moving some functions into files where their concerns are better aligned.
For example:

  • defaultRoute and onBadUrllib/route.js
  • addHooklib/hooks.js

Readability

To help navigate within fastify.js, I offer to add section markers to delineate the main phases of instance creation:

// === OPTIONS PREPROCESSING ===========================================
//
// Normalize and validate the user-supplied `options` object,
// then fill in default values.

...
// === INSTANCE OBJECT =================================================
//
// Construct the main `fastify` object, including the public API.
//

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.

@jean-michelet jean-michelet requested a review from mcollina May 4, 2025 12:42
@jean-michelet jean-michelet changed the title Refactor/clean fastify mainfile refactor: clean fastify mainfile May 4, 2025
@jean-michelet jean-michelet requested a review from jsumners May 5, 2025 20:42
@jsumners jsumners requested a review from a team May 6, 2025 10:47
Copy link
Member

@metcoder95 metcoder95 left a 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.

@jean-michelet
Copy link
Member Author

@metcoder95

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 setup-instance.js file for this PR, but since the steps are different in nature, I felt that separating them didn’t negatively impact the readiness.

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.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label May 9, 2025
@Eomm Eomm added the internals Change that won't impact the surface API. label May 9, 2025
Copy link
Member

@jsumners jsumners left a 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.

@jean-michelet jean-michelet force-pushed the refactor/clean-fastify-mainfile branch from bb0f744 to 722cc15 Compare May 15, 2025 20:37
@jean-michelet
Copy link
Member Author

@Eomm

I put all the setup functions in one file in ./lib so there is only one require.

Comment on lines +239 to +245
/**
* 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
*/
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation internals Change that won't impact the surface API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants