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

Everything depends on common module #4

Open
AradiPatrik opened this issue Apr 13, 2023 · 5 comments
Open

Everything depends on common module #4

AradiPatrik opened this issue Apr 13, 2023 · 5 comments

Comments

@AradiPatrik
Copy link

AradiPatrik commented Apr 13, 2023

Hey @Morfly !
I loved your presentation on Android Worldwide. I love how every dependency is so well scoped and the modules are loosely coupled.

The only problem I can see right now is that literally every module depends on common. I feel like when we have a new feature request we will introduce new Repositories and new domain models, but the repository and the models will only be really used in one or two feature modules.

Because the repositories and the domain models all live under the common module, every change to them will recompile the entire app, right? If so do you think it would be a good idea to separate the common modules based on the domain models, eg:

banking app

  • transactions module with Transaction domain model and TransactionRepository
  • user module with User domain model and UserRepository.

These modules could work like common, but only those modules would depend on them that really need the models and repositories.

Does this make sense, or would this be unnecessary?

@Morfly
Copy link
Owner

Morfly commented Apr 16, 2023

Hi @AradiPatrik
Thank you for watching my talk!

You're raising a very reasonable point! Because the sample project is small I added only 1 common module and put everything there. However, for larger projects, it's definitely a good idea to group the domain components of the app into corresponding separate modules and avoid storing all of them in a single common module.

This will help to ensure the change in one of the common modules won't drop the local cache of the entire project but rather only of those components that use transactions or user respectively.

Thank you for highlighting this!

@ber4444
Copy link

ber4444 commented Jun 8, 2023

I think the answer given by the two of you is correct, but this repo is following the incorrect pattern of actually adding a second common-like module, data, for repos and such. And data is most likely used by all feature modules, as well as handled via a composition local the same way, so it pointlessly has an impl and api. To put it another way, making an ABI incompatible change in data/impl is equally costly as in common.

To fix this, we would need to extract higher level shared components, such as shared use cases and common UI, and put them in their narrowly scoped modules (api and impl is optional here considering Kotlin has compilation avoidance and incremental compilation) which then would be included as a library in the features.

@AradiPatrik
Copy link
Author

@ber4444 For my project I ended up having use case modules, like: ImageManipulations, FaceDetection, Segmentation

ImageManipulations contains use-cases like converting white pixels to transparent ones etc...
FaceDetection and Segmentation modules use the on device ML and provide use cases to detect and segment faces
StickerGeneration contains domain use-cases for generating stickers and depends on the modules above

I do have api and impl modules for them and in the api I have the following:

interface ImageManipulationsProvider {
    val loadImageBitmapFromUri: LoadImageBitmapFromUri
    val cropToRect: CropToRect
    val cropToContour: CropToContour
    val getImageExifRotation: GetImageExifRotation
    val createMask: CreateMask
    val scaleBitmap: ScaleBitmap
    val padBitmapToSize: PadBitmapToSize
    val addTextToImage: AddTextToImage
    // ...
}

In the impl module I have the @module and the @component

In the Application class I have lot's of code like this:

val stickerGenerationProvider = DaggerStickerGenerationComponent.builder()
            .platformProvider(platformProvider)
            .faceDetectionProvider(faceDetectionProvider)
            .imageManipulationsProvider(imageManipulationsProvider)
            .segmenterProvider(segmenterProvider)
            .dataProvider(dataProvider)
            .build()

It's a lot of boiler plate in the Application class

In the MainActivity I have this monstrosity now:

CompositionLocalProvider(
      CompositionLocals.ofType<NavigationProvider>() provides application.appProvider,
      CompositionLocals.ofType<DataProvider>() provides application.appProvider,
      CompositionLocals.ofType<PlatformProvider>() provides application.appProvider,
      CompositionLocals.ofType<FeatureEntriesProvider>() provides application.appProvider,
      CompositionLocals.ofType<FaceDetectionProvider>() provides application.appProvider,
      CompositionLocals.ofType<ImageManipulationsProvider>() provides application.appProvider,
      CompositionLocals.ofType<SegmenterProvider>() provides application.appProvider,
      CompositionLocals.ofType<StickerGenerationProvider>() provides application.appProvider,
      CompositionLocals.ofType<LandingProvider>() provides application.appProvider
) {
      App(navController, appViewModel)
}

And then again in the features I have similar code to this:

val currentDataProvider = CompositionLocals.current<DataProvider>()
val currentPlatformProvider = CompositionLocals.current<PlatformProvider>()
val navigationProvider = CompositionLocals.current<NavigationProvider>()
val stickerGenerationProvider = CompositionLocals.current<StickerGenerationProvider>()
val faceDetectionProvider = CompositionLocals.current<FaceDetectionProvider>()
val imageManipulationsProvider = CompositionLocals.current<ImageManipulationsProvider>()
val landingProvider = CompositionLocals.current<LandingProvider>()

return rememberScoped(rootEntry) {
   DaggerPromptToStickerRootComponent.builder()
                .dataProvider(currentDataProvider)
                .platformProvider(currentPlatformProvider)
                .faceDetectionProvider(faceDetectionProvider)
                .stickerGenerationProvider(stickerGenerationProvider)
                .imageManipulationsProvider(imageManipulationsProvider)
                .navigationProvider(navigationProvider)
                .landingProvider(landingProvider)
                .build()
}

So @ber4444 I really like the idea of yours where I would just not do this api/impl dance for use-cases because it leads to lots and lots of boilerplate.

But do you also suggest to ditch the common data module? Or not to have api/impl separation for the data module either? Because if we want to have one single db for our app (which is quite convenient) then using one single module for the db is inevitable, and it feels wrong to depend on the implementation details of this module from almost every use-case.

If we don't have api-impl separation between the use-cases and the data module then if the data module implementation details change then all the use-cases have to be recompiled and so all the feature modules have to be recompiled so in the end the whole world has to be recompiled.

api and impl is optional here considering Kotlin has compilation avoidance and incremental compilation

But the api and impl separation still has the benefit that if only the implementation detail of a use-cases changes then only that one single module has to be recompiled. The dependent features don't have to, since the api didn't change at all.

So @ber4444 @Morfly My question is:

  • Should I use api/impl separation for things like my use case modules?
  • If I want a single db for my app, should I use api/impl separation to share it between use cases, or should it be one single module?

Thank you again for your time and effort you've put into sharing about such a difficult topic 🚀

@AradiPatrik AradiPatrik reopened this Jun 11, 2023
@ber4444
Copy link

ber4444 commented Jun 20, 2023

Hi @AradiPatrik, in my mind the narrowly scoped use case modules would be the abstractions in front of the data module, and feature modules should not depend on data any more. And keep data as lean as possible, e.g. hold your REST interface and/or GraphQL schema + queries, but not much more.

You can measure build speed via Gradle's --scan option and see it for yourself, e.g. make an ABI-incompatible change in data/impl or common such as renaming a method, and see the speed difference between that and making it in a featureX/impl (screen destination) or usecaseX (narrowly scoped and regular) module.

Modules without api/impl separation perform extremely well (as long as they are not behind composition locals), due to Kotlin's compilation avoidance, which as per https://youtrack.jetbrains.com/issue/KT-24203/Enable-Compile-Avoidance-for-kotlin-in-gradle-builds wasn't quite available yet when @Morfly came up with this project, but it is available now.

@AradiPatrik
Copy link
Author

Hi @ber4444 Thanks that's some great information! 🙌

For my next project I will definitely skip the api/impl modules for my usecase and data classes, and have the features depend on narrowly scoped use case modules.

And keep data as lean as possible, e.g. hold your REST interface and/or GraphQL schema + queries, but not much more

What about Room database? Are you suggesting having multiple databases in the app?

Anyway I will also take your advice and use the --scan option more so I can start optimizing the build speed when there's an actual issue with build speed!

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

No branches or pull requests

3 participants