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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[kmp] Replace Dagger-Hilt with Koin #215

Open
Kaaveh opened this issue Jun 16, 2024 · 17 comments
Open

[kmp] Replace Dagger-Hilt with Koin #215

Kaaveh opened this issue Jun 16, 2024 · 17 comments
Assignees
Labels

Comments

@Kaaveh
Copy link
Owner

Kaaveh commented Jun 16, 2024

Replace Dagger-Hilt with Koin

@hadi-norouzi
Copy link

Hi @Kaaveh
Can I work on it?

@Kaaveh
Copy link
Owner Author

Kaaveh commented Aug 27, 2024

Hey @hadi-norouzi
Sure! 🙌🏻

@hadi-norouzi
Copy link

Hi @Kaaveh
I've changed all modules with Koin but I have one issue.
As you have mentioned in the architecture diagram of the project domain:market has a dependency on data:market-repoistory. but in the code, it's not, data:market-repository depend on domain:market.

I've created a Koin module for each module and upper layer modules includes lower modules in their modules like this:

database module:

val localModule = module {
    single { Database() }
}

remote module:

val apiModule = module {
     single { Api() }
}

repository module:

val repositoryModule = module {
    single { Repository(get(), get())
}

I think we need to change the domain:market to be depends on data:market-repository. That's my suggestion.
I would be happy if you could suggest any helpful comment.

@Kaaveh
Copy link
Owner Author

Kaaveh commented Sep 6, 2024

This will violate the CLEAN arch. The implementation of the domain layer must be agnostic to the data layer.

@hadi-norouzi
Copy link

This will violate the CLEAN arch. The implementation of the domain layer must be agnostic to the data layer.

Yeah we could achieve these by change direction of dependency.

now is:
feature -> domain <- repository.

We could move repository interface to the repository module and only export that interface to domain module.

and change dependencies like this:
feature -> domain -> repository.

I think because of dagger-hilt current arch is possible but it's not possible in koin

@Kaaveh
Copy link
Owner Author

Kaaveh commented Sep 7, 2024

This will violate the CLEAN arch. The implementation of the domain layer must be agnostic to the data layer.

Yeah we could achieve these by change direction of dependency.

now is: feature -> domain <- repository.

We could move repository interface to the repository module and only export that interface to domain module.

and change dependencies like this: feature -> domain -> repository.

I think because of dagger-hilt current arch is possible but it's not possible in koin

What do you think @VahidGarousi ?

@VahidGarousi
Copy link
Collaborator

@Kaaveh I need a day to review and respond

@Kaaveh
Copy link
Owner Author

Kaaveh commented Sep 16, 2024

This will violate the CLEAN arch. The implementation of the domain layer must be agnostic to the data layer.

Yeah we could achieve these by change direction of dependency.

now is: feature -> domain <- repository.

We could move repository interface to the repository module and only export that interface to domain module.

and change dependencies like this: feature -> domain -> repository.

I think because of dagger-hilt current arch is possible but it's not possible in koin

Hey @hadi-norouzi
I spent more time on this issue to understand it. I wonder why you want to invert the dependency between the data and domain modules. Is there any problem with Koin?
According to Dependency Inversion in SOLID, I put the Repository interface in the domain module and all use-cases depend on the interface.

@hadi-norouzi
Copy link

hadi-norouzi commented Sep 16, 2024

This will violate the CLEAN arch. The implementation of the domain layer must be agnostic to the data layer.

Yeah we could achieve these by change direction of dependency.
now is: feature -> domain <- repository.
We could move repository interface to the repository module and only export that interface to domain module.
and change dependencies like this: feature -> domain -> repository.
I think because of dagger-hilt current arch is possible but it's not possible in koin

Hey @hadi-norouzi I spent more time on this issue to understand it. I wonder why you want to invert the dependency between the data and domain modules. Is there any problem with Koin? According to Dependency Inversion in SOLID, I put the Repository interface in the domain module and all use-cases depend on the interface.

Do you agree with me if we want to provides each dependencies in each module we need to have koin module for each android module like this:

val localModule = module {
    single { Database() }
}

val apiModule = module {
     single { Api() }
}

and upper layers should first include lower module and after that provides their dependencies like this:

val repositoryModule = module {
    include(localModule, apiModule)
    single { Repository(local = get(), api = get())
}

So repository module needs to import local and api modules.
Until repository module it's OK, but when we need to includes repository koin module in domain module it can't because domain is not depend on repository and have no access to that.

Another solution

We could import domain and data modules in app module and in koinStart function first include data module and blow that includes domain module.

startKoin {
    modules(dataModule, domainModule)
} 

@Kaaveh
Copy link
Owner Author

Kaaveh commented Sep 16, 2024

@hadi-norouzi
Use cases are used in viewModels. I think at least it can be provided in the feature modules.

@rezazarchi
Copy link
Contributor

rezazarchi commented Sep 25, 2024

I was looking for this issue to help if it is pending on something. I have a suggestion for your problem. Put your Koin modules into feature modules. Each feature module depends on core domain and also data modules, which give us reference to our needed classes. With such an arrangement, you can access modules in the app module to include them in the startKoin scope of the ComposeNewsApplication.
By the way, I saw domain:market module was added in app module dependencies. so i don't know what's the problem here.

@Kaaveh
Copy link
Owner Author

Kaaveh commented Sep 25, 2024

I saw domain:market module was added in app module dependencies. so i don't know what's the problem here.

@rezazarchi

I think it's due to the support for larger screens. It needs refactoring soon.

@Kaaveh
Copy link
Owner Author

Kaaveh commented Sep 25, 2024

@hadi-norouzi
Can you proceed or still have problems?

@rezazarchi
Copy link
Contributor

rezazarchi commented Sep 28, 2024

I saw domain:market module was added in app module dependencies. so i don't know what's the problem here.

@rezazarchi

I think it's due to the support for larger screens. It needs refactoring soon.

We can refactor multi-screens using the new adaptive-layout and adaptive-navigation-suite libraries later.
(Which I know this is off-topic, so let's discuss it in another related issue.)

@Kaaveh
Copy link
Owner Author

Kaaveh commented Sep 28, 2024

I saw domain:market module was added in app module dependencies. so i don't know what's the problem here.

@rezazarchi
I think it's due to the support for larger screens. It needs refactoring soon.

We can refactor multi-screens using the new adaptive-layout and adaptive-navigation-suite libraries.

Ya, Currently I'm working on refactoring the whole project in the next few days.

@hadi-norouzi
Copy link

I was looking for this issue to help if it is pending on something. I have a suggestion for your problem. Put your Koin modules into feature modules. Each feature module depends on core domain and also data modules, which give us reference to our needed classes. With such an arrangement, you can access modules in the app module to include them in the startKoin scope of the ComposeNewsApplication. By the way, I saw domain:market module was added in app module dependencies. so i don't know what's the problem here.

Yes, you are right. It's the solution.

@Kaaveh
Copy link
Owner Author

Kaaveh commented Nov 20, 2024

Hey @hadi-norouzi ,
I'd like to ping you and ask about your progress on this task.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

No branches or pull requests

4 participants