raamcosta / compose-destinations

Annotation processing library for type-safe Jetpack Compose navigation with no boilerplate.
https://composedestinations.rafaelcosta.xyz
Apache License 2.0
3.18k stars 132 forks source link

Multi Module support #12

Closed mhaqs closed 2 years ago

mhaqs commented 2 years ago

I tried using the plugin with a multi module Android project, and D8 doesn't like the generation of classes.

For example:

I have module A and B, which are included in the app module. The app module has

android{
...
    applicationVariants.all {
        sourceSets {
            getByName("main") {
                java.srcDir(File("build/generated/ksp/$name/kotlin"))
            }
        }
    }
...
}

but D8 fails during compilation with the following error:

* What went wrong:
Execution failed for task ':app:mergeLibDexInternalDebug'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.DexMergingTaskDelegate
   > There was a failure while executing work items
      > A failure occurred while executing com.android.build.gradle.internal.tasks.DexMergingWorkAction
         > com.android.builder.dexing.DexArchiveMergerException: Error while merging dex archives:
           Type com.ramcosta.composedestinations.AnimatedDestinationStyle$DefaultImpls is defined multiple times: <module1-path>\build\.transforms\7673bf153d1e0b34756cfeb713417353\transformed\classes\classes.dex, <module2-path>\build\.transforms\2692adfb593ffb4aebe8fa9a5adf48f0\transformed\classes\classes.dex
           Learn how to resolve the issue at https://developer.android.com/studio/build/dependencies#duplicate_classes.

Is it something I'm missing? Do you have a multi module example that I can follow? Thanks.

raamcosta commented 2 years ago

Hi @mhaqs ! πŸ™‚

Can you tell us a little bit more about your setup? Do modules A and B both use Compose Destinations library? Do both use @Destination annotations? Can you show us the different build.gradle files?

mhaqs commented 2 years ago

Yeah, both modules use compose-destinations . Both modules have this in the build.gradle file.

plugins {
    id(GradlePlugins.ksp).version(kspVersion)
}

dependencies {
    implementation (Libs.composeDestinations)
    ksp(Libs.composeDestinationsKSP)
}

Both modules are then included in the app module which has the generatedPath snippet for all variants. I have tried it with local path additions in the module build gradle files as well, but it doesn't work either way.

raamcosta commented 2 years ago

Ok thanks.

So honestly this is not something I've thought about before. But for Compose Destinations to work, we would need a central place (in your case the app module) to generate all the files. The problem is that the annotations are spread in modules A and B and there is no way (that I know of) of making KSP process annotations from other modules.

So I don't think this is possible, at least not without some serious work and thought. And for now, I am just a guy maintaining the library and I have a job too, so there is just no way. (But I am looking for people to help me, in case you're interested 😁)

All that said, I'd like to take a look in a code base like the one you're trying to build just with normal compose navigation. Just to see how it would work, maybe this gives me some ideas.

All I can think of is preparing the destinations all in the same module (your app module or just create a "navigation" module) then each would call other Composables from module A and B. This way navigation logic would also be all in one place, but then the actual Composables which compose actual UI would be in the feature modules. If it's really necessary to have the @Destination in different modules, then Compose Destinations is not for you atm 😒

Sorry for the long answer πŸ™‚

mhaqs commented 2 years ago

Thanks for responding. To look at a codebase that does multi module navigation, you can look at https://github.com/chrisbanes/tivi. That's exactly the kind of set up we have for our project. I was mostly trying to find a simpler way of defining nested/complex graphs, and your plugin seemed to do just that πŸ˜„ .

The idea of having a navigation module is good, and that's what I was thinking as well. Although, that creates more code structure issues than its worth e.g. injecting deps from other modules.

I did look at some issues on the KSP repo, and found ones like this: https://github.com/google/ksp/issues/372. It's really more a question of KSP being able to look at metadata from other modules. That does mean that when a module generates metadata, the classes in that metadata must be in a unique package.

So, for a quick workaround, when the compose-destinations-codegen module generates this metadata, it asks for a package location. This package location should then be used to reference where the generated code resides.

For example:

Module A

plugins {
    id(GradlePlugins.ksp).version(kspVersion)
}

composeDestinations {
  packageName.set("com.example.module.a")
}

dependencies {
    implementation (Libs.composeDestinations)
    ksp(Libs.composeDestinationsKSP)
}

Module B

plugins {
    id(GradlePlugins.ksp).version(kspVersion)
}

composeDestinations {
  packageName.set("com.example.module.b")
}

dependencies {
    implementation (Libs.composeDestinations)
    ksp(Libs.composeDestinationsKSP)
}

I haven't used KSP before, so I might just be writing nonsense. Thanks for the honest response

raamcosta commented 2 years ago

Hmmm yeah looking at what you said + that Chris Banes app + that KSP issue I do have some ideas..

No promises, but I'll keep you posted when I try some stuff ;)

raamcosta commented 2 years ago

In the mean time, my earlier comment is still valid: You can use @Destination only on your app module, deal with all navigation stuff there, then just call composables of your feature modules. This is actually the equivalent of what Chris Banes is doing in his app πŸ™‚

mhaqs commented 2 years ago

Yeah, I'll try a few things and see how it turns out. Mostly that I can have a navigation module just below the app module which includes all feature modules and deals with destinations.

hrach commented 2 years ago

Hi, my two bits how this could work:

package featureA

val destinations: List<Destination> = listOf(...)
Destinations.NavHost(featureA.destinations + featureB.destinations)

The main difference to current solution is:

The actual destination sharing is an implementation detail and it is up to the implementator - i.e. the list of destinations may stay "hidden" and developer may utilize Dependency Injection and use provider/factory pattern (for both list of destinations & and factory for Routed instance)

raamcosta commented 2 years ago

EDIT: Outdated:

Just wanna add that the reason why CoreExtensions and DestinationsNavHost are generated is because they adapt to the dependencies the user is using (Accompanist stuff).

To put them in the core, I'd have to publish more artifacts and even then it would not be straightforward. The other reason is the sealed Destination feature.

I think it is possible to configure my ksp processor at gradle level to generate them only in one of the modules (the one that would call DestinationsNavHost) and all others would just generate the Destination objects.

Other than that, you have some good ideas there :)

I honestly don't know when I'll go down this rabbit hole, I still like the approach in my previous comment. Marking Composables as @Destination makes sense to be done always in the same module, because just by doing that you're making a navigation decision. And I think it makes sense that all those are done in the same module. Feature modules should focus on displaying the UI. Then your annotated Composables would just call the feature module ones. Maybe take advantage of the annotated Composable to prepare additional stuff for the feature module Composable (example create the ViewModel and pass lambdas for UI Events and UI State classes and just pass those to the feature module Composable).

If we take a look at the Chis Banes example, we will see that that is actually what he's doing (without Compose Destinations of course).

raamcosta commented 2 years ago

Hi @mhaqs and @hrach πŸ‘‹

So good news, with version 1.1.1-beta, Destination generated classes will be put by default on a package with the common prefix from all @Destination annotated Composable functions. Besides, you can change this explicitly with:

ksp {
    // To turn off navigation graphs generation:
    // (if you prefer creating NavGraph instances or you prefer using the normal NavHost)
    arg("compose-destinations.generateNavGraphs", "false")

    // To change the package name where the generated files will be placed
    arg("compose-destinations.codeGenPackageName", "your.preferred.packagename")
}

Note also the first configuration, which is probably also a good thing to do for multi-module projects. I haven't had the time to try this yet, but, if I'm right, with these configs, each module can generate its own Destination files, and then the module that calls DestinationsNavHost can build a NavGraphSpec class and pass it to the DestinationsNavHost call.

With these changes and much more features/improvements that I implemented in the library since our discussion, I'd be thrilled if any one of you could try this setup πŸ™‚ I also will make the time some day (hopefully soon) to set up a multi-module project to try it (or who knows maybe I'll make a PR on Chris Banes tivi repo πŸ˜„ ).

Thank you guys once again, and hope to hear from you soon!

Rafael Costa

mhaqs commented 2 years ago

Hey thanks for the tag and putting in the effort for this to work. I'll give it a try some time on this weekend, maybe. I'll respond back with my findings.

I chose to go down the path of having a global navigation module on the root app. But I'd still like it to live on respective modules.

Thanks once again

hrach commented 2 years ago

Thanks, will check in the following days.

mhaqs commented 2 years ago

I've tested the library with multi modules, and multi variants as well, and it does work now without changing any configurations. Thanks for putting in the effort to bring the feature πŸ‘

raamcosta commented 2 years ago

Cool thanks for letting me know. Btw I'm curious on how you're setting up the navi graphs. You mentioned that you didn't need any configuration. Does that mean that each module is creating it's own nav graph? Or are you ignoring the auto generated Na graphs objects and creating them yourself?

Thanks in advance!

mhaqs commented 2 years ago

Every module has its own graph indeed, and their own start destinations. The app has isolated zones which do not require traversing other graphs once you're past using them. This keeps the graph groups clean. We just group all destinations together, and when the user moves to the next graph, the start destination is changed as well.

It's complex, and that's why organising the destination logic required that we don't have something global. But, even with a root and nested graph structure, the isolated nav graph generation should still be ok, as long as your root graph knows how to traverse them. That's what tivi app does, and that's what you touch on in the wiki here: https://github.com/raamcosta/compose-destinations/wiki/Defining-your-navigation-graphs#manually-defining-navigation-graphs.

Moreover, the usability of a library like Compose-Destinations is reduced greatly when you have to manually manage the NavHosts and pass a lot of information/states down the graph. It'll be up to anyone going this route to weigh out, if it's worth investing time to let a library generate the boilerplate or do it manually.

frozenjava commented 2 years ago

Hello. I don't have anything else to add to this at the moment. Just wanted to say I also have a few apps that are multi-modular following the same pattern mentioned above and I am looking into using this library as well.

If there are two of us commenting there are probably more out there so it might be a use-case that should be kept in mind.

Great work so far on the library, really makes life easier!

jmadaminov commented 2 years ago

Hi, thanx for a cool lib, I just wanted to know if anyone tested the deeplinking and state restoration in a multimodule project with this lib... I've read in couple places where Ian Lake was stating that having multiple NavHosts is a bad thing to do in compose and by using this library in a multi module project with multiple DestinationsNavHost would be that "Bad thing" right? Would really appreciate some clarifications on this issue please...

raamcosta commented 2 years ago

@frozenjava First of all, thank you! :) I am basically expecting this to be fully working already! I just need to find the time to test it properly, but the support should be there :) I hope next week I can get the time to fully test things and make changes if needed to improve the support.

@jahongir9779 Thank you as well! πŸ‘

I've read in couple places where Ian Lake was stating that having multiple NavHosts is a bad thing to do in compose and by using this library in a multi module project with multiple DestinationsNavHost would be that "Bad thing" right?

Yes, multiple DestinationsNavHost is equivalent to multiple NavHost indeed. To be honest, I don't see what is the problem with having multiple of them, but I may be missing something if Ian Lake said it. Anyway, even with a multi-modular project, you don't necessarily need multiple DestinationNavHosts. You can build a single root navigation graph and put all generated Destination in there, even ones built on different modules. You can also build nested nav graphs for each module and have them be nested in the root one you build in the module you call DestinationsNavHost. There are plenty of ways to do this.

Deep Linking and state restoration should work just like with normal compose navigation (the only difference is how you define the Deep Links which is in the annotation), even in a multi-module project.

llama-0 commented 2 years ago

@mhaqs & @hrach Hi there! I'm about to start building a multi-module application in compose. In my prerequisite research, I stumbled on this discussion (thanks a lot for that, btw) and now I wonder whether any of you used Hilt as a DI framework in your projects? Does it work well with this library?

raamcosta commented 2 years ago

Hi @llama-0 ! πŸ‘‹

Yes, it will work just fine with Hilt, there is really no overlap between them so you won't find any issues there :) If you find any other problem, for example with multi-module support please do let me know! I still have not had the time to test multi-module apps with this library properly, I'm thinking I will be able to very soon.

JohnBuhanan commented 2 years ago

I'm here trying to build multi-module also!

Good luck everyone :)

raamcosta commented 2 years ago

@JohnBuhanan I am at risk of sounding repetitive here but please let us know how that goes! πŸ™‚

I am currently finishing support for passing arguments to back destination in a type-safe and easier way. After I release that I'll start looking into a multi-module sample.

JohnBuhanan commented 2 years ago

Oh nice, that will be the next piece of the puzzle needed for returning results.

And you bet.

jmadaminov commented 2 years ago

Hi, I've tried it in a multi module project. Tried both with DestinationsNavHostand NavHost with my own NavGraph classes (by setting the arg("compose-destinations.generateNavGraphs", "false")) but both approaches ended with:

DexArchiveMergerException: Error while merging dex archives: ... ...DestinationKt is defined multiple times

do you have any ideas on what I might be doing wrong?

raamcosta commented 2 years ago

By default, the package name used for all generated files will be the common part of all package names of the annotated Composables. It seems to me like in your case multiple modules are using the same package name. If so, you can explicitly ask for a package name where all generated files will live by module.

On each module's build.gradle where you use compose destinations, you would add:

ksp {
    arg("compose-destinations.codeGenPackageName", "your.package.name.modulea")
}

replacing modulea with the name of that module.

Let me know if this helped :)

jmadaminov commented 2 years ago

not sure if my understandings are right, please correct me if I'm wrong...

I'm trying to have multiple NavGraphs within a Single DestinationsNavHost. I have different feature modules with screens that need to be put inside a separate NavGraphs. To achieve this:

1) I need to set compose-destinations.codeGenPackageName to a common module while allowing NavGraph generation. 2) Assigning navGraph param for each @Destination inside a NavGraph with only a single start = true screen 3) Include all of the @Destinations in a composable(..){} inside DestinationNavHost 4) Navigate to a different navGraph by destinationsNavigator.navigate(NavGraphs.[yourNavGraphName])

does these steps sound right?

raamcosta commented 2 years ago

Hmm.. It really depends on your setup, I'd need more specifics. But the most consistent way of doing this would be each feature module should do this:

ksp {
    arg("compose-destinations.generateNavGraphs", "false") // after this, you should not use "navGraph" or "start" on annotation
    arg("compose-destinations.codeGenPackageName", "your.packagename.featureA") // replacing featureA with the feature name
}

Then in your navigation module where you call DestinationsNavHost, also disable nav graph generation and create an object like this:

object NavGraphs {

    val featureA = NavGraph(//... in the list of destinations, use all generated destinations of module "featureA")
    //...
    val featureN = NavGraph//... one of this per nested navigation graph
    //...

    val root = NavGraph(
        startRoute = featureX, // use the feature where your app starts
        //... leave list of destinations empty (probably?) then in "nestedNavGraphs" get all the above nav graphs, so:
        nestedNavGraphs = listOf(featureA, featureB, /*...*/, featureN)
    )
}

You then pass that NavGraphs.root to the DestinationsNavHost call. You don't need to add any composable call inside DestinationsNavHost unless you really need to pass some component that the library can't provide.

To navigate you are on point. But that will only work from your navigation module where you defined the above NavGraphs. If you need to navigate from featureA to featureB, one way to do it is that featureA declares some interface with the navigations it needs to do, then this common navigation module would provide an implementation where it can use the feature NavGraphs above.

jmadaminov commented 2 years ago

Thank you for your time, I've tried what you advised, but now having ClassCastException: ...Destination cannot be cast to packagename.destinations.TypedDestination

because the screens in different modules are using their own generated Destination.kt

raamcosta commented 2 years ago

You're right πŸ€” I should make clear that this support is still very experimental, I am now on process to start experimenting with it and later on there will be probably added features to make all this easier.

In the mean time, you can solve this by just using DestinationSpec instead of Destination in the NavGraph data class. Basically, in your NavGraphs object, replace instantiating the generated NavGraph with using NavGraphSpec directly, so:

object NavGraphs {

    val featureA = object: NavGraphSpec {(//... in the list of destinations, use all generated destinations of module "featureA")
    //...
    val featureN = object: NavGraphSpec {//... one of this per nested navigation graph
    //...

    val root = object: NavGraphSpec {
        startRoute = featureX, // use the feature where your app starts
        //... leave list of destinations empty (probably?) then in "nestedNavGraphs" get all the above nav graphs, so:
        nestedNavGraphs = listOf(featureA, featureB, /*...*/, featureN)
    )
}

All untested code, but this would be the idea. This should work because all generated Destination implement the same DestinationSpec.

jmadaminov commented 2 years ago

Thanx a lot πŸ™‚

raamcosta commented 2 years ago

Let me know if you get that to work. Are you working on something we can see the code? Is it open source? πŸ™‚

jmadaminov commented 2 years ago

It worked! πŸ˜„ I did everything exactly as you said)) no special tweaks needed. This is the NavGraph that I've tested

object NavGraphs {

    val auth = object : NavGraphSpec {
        override val destinationsByRoute: Map<String, DestinationSpec<*>>
            get() = mapOf(Pair(AUTH_GRAPH, PhoneAuthScreenDestination))
        override val route: String
            get() = AUTH_GRAPH
        override val startRoute: Route
            get() = PhoneAuthScreenDestination
    }

    val root = object : NavGraphSpec {
        override val destinationsByRoute: Map<String, DestinationSpec<*>>
            get() = mapOf(Pair("root", SplashScreenDestination))
        override val route: String
            get() = "root"
        override val startRoute: Route
            get() = SplashScreenDestination
        override val nestedNavGraphs: List<NavGraphSpec>
            get() = listOf(auth)
    }

}
raamcosta commented 2 years ago

Cool! A way to simplify that code:

object NavGraphs {

    val auth = object : NavGraphSpec {
        override val route = AUTH_GRAPH
        override val destinationsByRoute = listOf(PhoneAuthScreenDestination).associateBy { it.route }
        override val startRoute = PhoneAuthScreenDestination
    }

    val root = object : NavGraphSpec {
        override val route = "root"
        override val destinationsByRoute = listOf(SplashScreenDestination).associateBy { it.route }
        override val startRoute = SplashScreenDestination
        override val nestedNavGraphs = listOf(auth)
    }

}

Note that the route of each destination must not equal the route of the NavGraph itself. The associateBy is your friend here: you just need to make a list of Destinations.

jmadaminov commented 2 years ago

awesome !))

JohnBuhanan commented 2 years ago

@raamcosta

I debated trying to clean this up more, but maybe getting it in front of your eyes sooner is more important.

https://github.com/JohnBuhanan/MVI-Public

Can you see if there is an easier way to get EntryPointActivity.Initialize to work?

Is anything about my approach salvageable?

raamcosta commented 2 years ago

Is there a way for us to connect without spamming this thread? I'm on twitter if you can DM me. Because this doesn't seem as easy to answer as other questions πŸ˜„ https://twitter.com/raamcosta

JohnBuhanan commented 2 years ago

I'll try :P

miduch commented 2 years ago

is there a simple sample available for multi module?

raamcosta commented 2 years ago

No for now, but I am working on a fork of a known Chris Banes project: tivi (https://github.com/chrisbanes/tivi) In the next few days, I'll be doing a few small changes on Compose Destinations Library to be easier to use in multi-module projects (you can already do it, but I want to try to make it easy). Later I'll make my fork public so everyone can see an example of a multi-module project in practice πŸ™‚

llama-0 commented 2 years ago

It would be great if someone, who succeeded in applying your lib to the multi-module project will share a basic example because the configuration is a pain. I still can't manage to generate NavGraphs -- the build/generated/ksp/debug folder is empty (no kotlin folder present)

raamcosta commented 2 years ago

Are you looking at the folder from the IDE? If yes, check the folder in the terminal/explorer. Because you might be missing the last step of the README:

kotlin {
    sourceSets {
        debug {
            kotlin.srcDir("build/generated/ksp/debug/kotlin")
        }
        release {
            kotlin.srcDir("build/generated/ksp/release/kotlin")
        }
    }
}

This (or similar) needs to apply to each module.

llama-0 commented 2 years ago

I added these lines, yes. Probably they are not recognized by some of my modules or smth. Now I'm learning to apply your lib only for 1-module project and it's working. And the next step is to expand to 2 modules in order to go slowly and hopefully by the end of the day I'll figure it out.

raamcosta commented 2 years ago

You need those lines for each module's build.gradle. So yeah copy paste those into the build.gradle of each module πŸ™‚

miduch commented 2 years ago

here is super simple example @llama-0, seems to work fine

https://github.com/miduch/ComposeNavigation/raw/master/screenshots/composenavigation.mp4

https://github.com/miduch/ComposeNavigation

hopefully @raamcosta can point out how this could be done better :)

not sure how to have onboarding route shown only once upon first run, and after that always homescreen route is root? now when auto launched from homescreen route, there is some flicker happening

raamcosta commented 2 years ago

@miduch Things to improve:

not sure how to have onboarding route shown only once upon first run, and after that always homescreen route is root? now when auto launched from homescreen route, there is some flicker happening

You can pass a startRoute to DestinationsNavHost call, overriding the startRoute set in the NavGraphSpec of your NavGraph.

raamcosta commented 2 years ago

Other than that, it is a nice sample. Thank you @miduch πŸ‘

miduch commented 2 years ago

Thanks for quick response. Applied some of your suggested changes and updated sample project.

Maybe each module should not depend on others of same level? Example: homescreen module shouldn't depend on the onboarding. I realize that this is the simplest way to navigate from homescreen to onboarding screens, but it may not be ok on most multi module projects.

Yes this is problem, any suggestions?

You can pass a startRoute to DestinationsNavHost call, overriding the startRoute set in the NavGraphSpec of your NavGraph.

Did you mean something like this?

fun AppNavigation() {
    val showOnboarding = listOf(true, false).random()
    DestinationsNavHost(
        navGraph = AppNavGraphs.home,
        startRoute = if (showOnboarding) AppNavGraphs.onboarding.startRoute else AppNavGraphs.home.startRoute
    )
}

it gives me java.lang.IllegalArgumentException: navigation destination on_boarding_screen is not a direct child of this NavGraph

miduch commented 2 years ago

following seems to launch onboarding screen without that flicker

@Composable
fun AppNavigation() {
    val showOnboarding = listOf(true, false).random()
    if (showOnboarding) {
        DestinationsNavHost(
            navGraph = AppNavGraphs.home,
            startRoute = AppNavGraphs.onboarding
        )
    } else {
        DestinationsNavHost(navGraph = AppNavGraphs.home)
    }
}

but now navigator.popBackStack() doesn't work anymore from OnBoardingScreen when done button is pressed.

raamcosta commented 2 years ago

Try this:

fun AppNavigation() {
    val showOnboarding = listOf(true, false).random()
    DestinationsNavHost(
        navGraph = AppNavGraphs.home,
        startRoute = if (showOnboarding) AppNavGraphs.onboarding else AppNavGraphs.home
    )
}
raamcosta commented 2 years ago

Thanks for quick response. Applied some of your suggested changes and updated sample project.

Maybe each module should not depend on others of same level? Example: homescreen module shouldn't depend on the onboarding. I realize that this is the simplest way to navigate from homescreen to onboarding screens, but it may not be ok on most multi module projects.

Yes this is problem, any suggestions?

Oh my god sorry I wanted to give suggestion then forgot πŸ˜… What I've done in the tivi repo is I define an interface on each module that needs to navigate with the methods it needs. For example, on "Discover feature module":

interface DiscoverNavigator {
    fun openTrendingShows()
    fun openPopularShows()
    fun openRecommendedShows()
    fun openShowDetails(showId: Long, seasonId: Long?, episodeId: Long?)
    fun openUser()
}

Then in the DestinationsNavHost call level I have a navigator that implements all those interfaces of the feature modules:

class CommonNavigator(
    private val navController: NavController,
) : RecommendedShowsNavigator,
    ShowSeasonsNavigator,
    ShowDetailsNavigator,
    DiscoverNavigator,
    TrendingShowsNavigator,
    PopularShowsNavigator,
    FollowedNavigator,
    WatchedNavigator,
    SearchNavigator,
    AccountUiNavigator,
    EpisodeDetailsNavigator {

    override fun openTrendingShows() {
        navController.navigateTo(TrendingShowsDestination)
    }

    //...

And finally I need to pass this navigator:

fun DestinationScope<*>.currentNavigator() = CommonNavigator(navController)

    DestinationsNavHost(
            //....
    ) {

        composable(DiscoverDestination) { Discover(currentNavigator()) }

        //....