Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Dependency injection #79

Merged
merged 14 commits into from
Feb 9, 2025
Merged

Dependency injection #79

merged 14 commits into from
Feb 9, 2025

Conversation

minoneer
Copy link
Collaborator

@minoneer minoneer commented Feb 7, 2025 •
edited
Loading

The goal of this PR is to improve the maintainability of the plugin by moving towards dependency-injection-based initialization.

Current issue:
The plugin currently uses a combination of static references, a massive single-instance service locator (the main uSkyBlock class), and passing dependencies down to child objects. This means

  • initialization order is implicit, and changing anything in the order may break stuff due to undocumented implicit dependencies
  • an overloaded main plugin class
  • implicit cyclic dependencies between services
  • changing a class usually requires changes to several others, including the main plugin class
  • Things are hard to test, as everything depends on everything else

This PR breaks these dependencies through dependency injection using Guice.

  • Most services, commands, and listeners are initialized through DI
  • Some services were refactored to depend on the actual type instead of the uSkyBlock class. This is an ongoing effort and will require continued effort in the future
  • Moved most of the initialization code out of the main plugin class

Initial testing looks good, but since this is a larger change, it may cause some initial instability or hidden issues.

Closes #14
Resolves #76

@minoneer minoneer added enhancement New feature or request maintenance Maintenance chores, e.g., migrate deprecated API's labels Feb 7, 2025
@minoneer minoneer requested a review from Muspah February 7, 2025 13:57
@minoneer minoneer self-assigned this Feb 7, 2025
@minoneer
Copy link
Collaborator Author

minoneer commented Feb 7, 2025

@Muspah, I would appreciate a second opinion on this; sorry for the massive diff.

@minoneer
Copy link
Collaborator Author

minoneer commented Feb 7, 2025

PS: hiding whitespace changes helps ;)

Muspah
Muspah approved these changes Feb 9, 2025
View reviewed changes
Copy link
Member

@Muspah Muspah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, it's "boring" stuff but it needs to be done to overhaul all the legacy crap that's in the codebase. Thanks very much for your continuous effort.

Are we able to run some real-world tests before releasing to Spigot/Paper/...? I do run a small demo server, but without many players except for myself.

@minoneer
Copy link
Collaborator Author

minoneer commented Feb 9, 2025

LGTM, it's "boring" stuff but it needs to be done to overhaul all the legacy crap that's in the codebase. Thanks very much for your continuous effort.

Are we able to run some real-world tests before releasing to Spigot/Paper/...? I do run a small demo server, but without many players except for myself.

Thanks!

I did some quick testing on my test setup - mostly to make sure commands, challenges, etc. still work as expected. I can update to our live server and see if we run into issues. We don't have a ton of players in skyblock, but a handful of regulars.

@Muspah
Copy link
Member

Muspah commented Feb 9, 2025

Sounds good. Feel free to merge when you're ready and I will include it into my SQL-support branch when possible so it gets tested on my demo too. Lets just prevent that it gets included in some trivial MC version update release without testing :)

@minoneer
Copy link
Collaborator Author

minoneer commented Feb 9, 2025

Sounds good!

Can we create a release for spigot & paper at the current master head (4d7afec)? It includes a bug fix for 1.21.4, biome updates, and a couple other things that should be stable (we are running it in production). It is compatible with [1.20.5, 1.21.4].

minoneer added 14 commits February 9, 2025 22:47
@minoneer
Add guice for dependency injection
56c3d59
@minoneer
Use DI for biome related classes
18ace4c
@minoneer
Initialize most services via dependency injection
880dbc4 
- Make most dependencies obvious by moving injections to the constructor
- Break some cyclic dependencies
- Reduce reliance on the main plugin class
@minoneer
Restructure Command and Listener lifecycle management
41dc320
@minoneer
Initialize sub commands via dependency injection
22cb876
@minoneer
Cleanup more bootstrapping
09f0158
@minoneer
Additional bootstrap cleanup
cce54a2
@minoneer
Move Island level refresh task out of main plugin class
de0d86a
@minoneer
More lifecycle cleanup
c76e564
@minoneer
Move version info to its own module
e1d9e3d
@minoneer
Fix static legacy initialization
ff125f0
@minoneer
Move directory initializations to their proper classes
1aad18d
@minoneer
Add Settings to early legacy initialization
025761b
@minoneer
Remove redundant imports from MainConfigMenu
3485366
@minoneer minoneer force-pushed the dependency-injection branch from 2ec9fad to 3485366 Compare February 9, 2025 21:48
@minoneer minoneer merged commit 8ad2dc2 into uskyblock:master Feb 9, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Muspah Muspah Muspah approved these changes

Assignees

@minoneer minoneer

Labels
enhancement New feature or request maintenance Maintenance chores, e.g., migrate deprecated API's
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cleanup Plugin Initialisation
2 participants
@minoneer @Muspah