General Refactoring work

[ong6]

Is your request related to a problem?

Feels like the current core/site/index.js file is getting a little long (over 1.5k lines), making it hard to read and find important information, what is the general consensus around refactoring it?

Describe the solution you'd like

I propose we refactor the core/site/index.js file into all the different commands such as deploy, convert etc. Each command can be a separate class that contains its own functionality, while site can still exist as a the main driver class. This way, we can easily view all the code related to each command and make adding new commands easier.

Though I'm not sure how doable this is since everything looks very tightly coupled. Any suggestions or ideas on how to go about refactoring this would be great!

Describe alternatives you've considered

Nil

Additional context

Nil

[jonahtanjz]

Related to #1495

[lhw-1]

Let me try researching this issue further - I'll post any updates / findings if I run into issues with decoupling and refactoring :sweat_smile:

(Keeping this in mind: Completing TypeScript migration of Site/index.js first might be preferrable as mentioned in #1495 (comment) by @ang-zeyu)

The current plan is to refactor Site/index.js according to the commands specified in packages/cli/index.js, namely init, serve, build, and deploy. init can be tackled first as it is quite decoupled from the rest & will serve as continuation of #2148 (relevant comment here), but for the other three, some more digging may have to be done before coming up with further plans. (Currently I am thinking of just having a SiteServeManager, SiteBuildManager, and SiteDeployManager within Site/index.js, but this depends on how coupled the functions are)

Update 1:

After looking through Site/index.js and trying to make sense of which functions are being called where, I think that there should be two more classes in particular that can be refactored: SiteConfigManager and SitePageManager. There are numerous functions for both SiteConfig and Page that are being called during init, serve, and build, and it should be a good first step to refactor these out before working on the command-specific managers.

The advantage of having command-specific managers: SiteInitManager, SiteServeManager, SiteBuildManager, and SiteDeployManager would be that we can have more modularity with the CLI commands in the future - e.g. with this refactor, we can have the --template and the --convert option handled entirely within the SiteInitManager (it is currently being done in cli/index.js, which is alright for now, but cli/index.js is slowly building up as well - we should try to refactor some parts of it before it gets difficult to manage it like Site/index.js). Of course, this means that we would have to refactor parts of cli/index.js together with Site/index.js, but with long-term code quality in mind, I think this is worth working on.

Update 2:

After some preliminary refactoring and testing, it seems that with other files in TypeScript, it would be preferrable to not only have Site/index.js be migrated to TypeScript before any refactoring, but having cli/index.js be migrated would likely reduce obstacles for the refactoring process. (Neither seem to be mandatory, but would be preferrable)

As a side note, I have identified readSiteConfig, addDefaultLayoutToSiteConfig, and writeToSiteConfig to be possibly refactored into SiteConfigManager class, with possible a few other methods as well. If there are parts of Site/index.js that can be refactored without affecting the TypeScript migration, I will create separate PRs for those.

[ang-zeyu]

Both general directions sound good. 👍