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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants