Morfly / MultiModuleMovies

Multi-Module Movies is a The Movie DB project that demonstrates how to build a scalable multi-module architecture for Android apps that use Jetpack Compose.
MIT License
163 stars 25 forks source link

Everything depends on common module #4

Open AradiPatrik opened 1 year ago

AradiPatrik commented 1 year ago

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

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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:

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

ber4444 commented 1 year ago

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 commented 1 year ago

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!