Skip to content
This repository has been archived by the owner on Dec 3, 2024. It is now read-only.

Refactor to use Dagger2 Hilt #2053

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adamszewe
Copy link
Contributor

@adamszewe adamszewe commented Feb 4, 2024

WIP
Refactoring the app to use Dagger2 Hilt 🚀
https://dagger.dev/hilt/

@imsodin
Copy link
Member

imsodin commented Feb 6, 2024

Please don't invest any more time in this for the moment!

This is a big change, so lots of things to go wrong, and I don't have much time and inclination to review it. What's the motivation for this change? It looks like it replaces manual-ish dependency injection with something more annotation based.

Edit: Dang I forgot to state an important bit: I recognise you already contributed quite a few things - thanks a lot for that! That's basically why I am asking and even considering this.

@adamszewe
Copy link
Contributor Author

adamszewe commented Feb 6, 2024

Please don't invest any more time in this for the moment!

This is a big change, so lots of things to go wrong, and I don't have much time and inclination to review it. What's the motivation for this change? It looks like it replaces manual-ish dependency injection with something more annotation based.

Edit: Dang I forgot to state an important bit: I recognise you already contributed quite a few things - thanks a lot for that! That's basically why I am asking and even considering this.

👋 @imsodin
Yes, the main purpose of this change would be to:

  1. get rid of the dagger component and old way of injecting classes, which requires the injected class to request injection explicitly
  2. make it easier to provide and inject dependencies in general

It's a big change, maybe too big for now.
Thanks for the early feedback 😃
So, the good news is that the refactor, as it is now, works as expected. I tested manually all the screens I found and didn't find any issues. This is because there are not many classes being injected and there is only a single dagger component in the app.
But I agree it would be a bad idea to merge it as it is, as I may not be aware of some edge cases and would avoid breaking the app for the users.

I'd do the following then:

  • keep this PR as draft, in case any one else wanted to attempt the same, to keep the conversation open
  • simplify the current DI in the app in future PRs (there are a couple of classes that don't need injection at all)
  • try again in few weeks when this change would be easier to do and would result in a much smaller PR that is easier to review

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants