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

Ambiguous Routes in Multi-Module projects #134

Open
jakobulbrich opened this issue May 19, 2022 · 11 comments
Open

Ambiguous Routes in Multi-Module projects #134

jakobulbrich opened this issue May 19, 2022 · 11 comments
Labels
enhancement New feature or request

Comments

@jakobulbrich
Copy link

If we have a multi module project and define a destination composable with the same name in both modules, we end up with the same route for both of them. I for example need to have two different "More" screen in two areas of my app.

Sure, we could specify a custom route in the destination and even use the COMPOSABLE_NAME placeholder there like

@Destination(
    route = "area1-$COMPOSABLE_NAME"
)

but would it be an option to automatically add a unique prefix to the route for each module (e.g. the package name) to avoid such an ambiguity in the first place?

@raamcosta raamcosta added the enhancement New feature or request label May 20, 2022
@raamcosta
Copy link
Owner

Hi @jakobulbrich 👋

At first look, it makes perfect sense, there is even a "moduleName" config that could serve as the prefix for all the destinations in the module. I'm just wondering wether or not it should also be used as prefix for the generated destination, like Area1SomeScreenDestination or if it would just be for the route.

@raamcosta
Copy link
Owner

What do you think?

@jakobulbrich
Copy link
Author

I suggest not to do so. If we e.g. have a module named "settings" and just a composable SettingsScreen in it we would end up with SettingsSettingsScreenDestination if we add the module name as a prefix. I don't think that is something one would expect.

Also we are able to distinguish destinations with import aliases when using them if necessary. import area1.MoreScreenDestination as Area1MoreScreenDestination

@jakobulbrich
Copy link
Author

But all destinations of a module could be grouped in a sealed class/interface.
As far as I know they are already sealed, but if we would place it in the body, we would end up with Settings.SettingsScreenDestination or SettingsDestination.SettingsScreen. Not sure about what the best naming would be and if this would be convenient to use. Or if this is necessary at all.

What do you think about that?

@raamcosta
Copy link
Owner

Ok yeah, makes sense 🙂
Then it is all about that route uniqueness requirement right?

Btw, with this setup, does it crash immediately when calling DestinationsNavHost? Or how does it behave with non unique routes? I was wondering 🤔

@raamcosta
Copy link
Owner

But all destinations of a module could be grouped in a sealed class/interface. As far as I know they are already sealed, but if we would place it in the body, we would end up with Settings.SettingsScreenDestination or SettingsDestination.SettingsScreen. Not sure about what the best naming would be and if this would be convenient to use. Or if this is necessary at all.

What do you think about that?

That would be messy in the code, it would be a super big file. They are sealed, but just not nested.

@jakobulbrich
Copy link
Author

That would be messy in the code, it would be a super big file. They are sealed, but just not nested.

Yes I also thought so. And since there isn't an actual issue with the class names, we should keep them as they are and only make the routes unique 👍

Btw, with this setup, does it crash immediately when calling DestinationsNavHost? Or how does it behave with non unique routes? I was wondering 🤔

It actually navigated to one of them but not quite sure how it decides which one it takes 😄

@raamcosta
Copy link
Owner

Uhh so that’s even worse, I could help there by asserting on my side when calling the nav host. I’m guessing for vanilla compose navigation the second “composable” call overrides the first.

@raamcosta
Copy link
Owner

raamcosta commented May 20, 2022

I mean when there is a single navigation module, I am already failing at compile time. But when in multi module, there is no way of knowing for sure at that time.

@jakobulbrich
Copy link
Author

jakobulbrich commented Jun 24, 2022

I now ended up with a draft of a method that generates me a unique name for each module depending on how it is nested in the root project. Maybe we can use something like that as the default for the compose-destinations.moduleName ksp argument.

fun Project.moduleName(takeUntil: (Project) -> Boolean = { false }) = buildList {
    var currentProject: Project? = project
    while (currentProject != null) {
        logger.warn(currentProject.name)
        add(currentProject.name)
        currentProject = currentProject.parent?.takeUnless {
            // Stop if the root project is reached or if it should break early
            it == project.rootProject || takeUntil(it)
        }
    }
}
    .reversed()
    .map { it.split("[^A-Za-z]".toRegex()) }
    .flatten()
    .joinToString(separator = "") {
        it.capitalize()
    }

If we are having a structure like this root/ui/my-area-one/start will generate the name UiMyAreaOneStart or MyAreaOneStart if we pass a lambda moduleName { it.name == "ui" } for the start module.

And if we specify it as compose-destinations.moduleName we will end up with a generated MyAreaOneStartNavGraph. This could also be used for the name of the Destination classes and their routes

@raamcosta
Copy link
Owner

On v2, we do enforce route uniqueness at runtime. So you'd have to manually set routes, as you said.

I honestly forgot about this, but I could still do it: prefix the route with the module name 🤔

I'll leave this open.. maybe in one more year I finally decide to try this out ahah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants