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

Automatically create NavHost #28

Open
cvb941 opened this issue Jun 4, 2022 · 5 comments
Open

Automatically create NavHost #28

cvb941 opened this issue Jun 4, 2022 · 5 comments

Comments

@cvb941
Copy link

cvb941 commented Jun 4, 2022

Hi, thanks for your library, this is very much missing from the official compose navigation library.

I still quite don't understand the way the routes are set up in Navigation Compose, but it seems to me that there is still an unnecessary step when creating the NavHost composable.

NavHost(navController = navController, startDestination = MainGraph.route) {
    navigation(LoginGraph) {
        login { LoginScreen(navController) }
    }

    navigation(MainGraph) {
        camera { CameraScreen(navController) }
        home { HomeScreen(navController) }
        item { ItemScreen(navController) }
        profile { ProfileScreen(navController) }
    }
}

Since all Screens are already annotated with the @Route annotation providing all the information needed, isn't it possible for this library to generate the whole NavHost composable without having to add new screens to it manually every time?

Sorry if I'm missing something.

@levinzonr
Copy link
Owner

Hey @cvb941
Thank you for the kind words :)
I see where you are coming from, and yes, this can be done, but I personally don't like this approach.

Generating the whole NavHost, i.e generating the whole navigation graph comes with some drawbacks.
Firstly, you don't see your navigation graph until the build phase is complete, this reduces the code understanding, you can't just look into the navigation graph and see how many screens you are dealing with until you clone and build the project.

The second one is the parameters that your Route Accepts. Having the whole navGraph generated for you means, you are limited to what you can specify as the parameter, like how would you pass your lambda here?

@Composable
@Route
LoginScreen(onComplete: () -> Unit = {} ) {
} 

Again, can be done by doing some hacky "injections" but I don't think the navigation library should be responsible for that.

@cvb941
Copy link
Author

cvb941 commented Jun 6, 2022

For your first concern, I think the @Route annotated Screen composables may be telling enough to document the code, at least for a simple case like my example, where you can just style your code in this way from the get go.

To the second point, you could just take the parameters of the @Route composable and move them as parameters into the NavGraphBuilder generated extension functions.

// Old generated extension
public fun NavGraphBuilder.login(content: @Composable () -> Unit): Unit {
 route(LoginRoute, DefaultRouteTransition) {
    content()
  }
}
// New generated extension
public fun NavGraphBuilder.login(navHost: NavHostController, onLogin: () -> Unit): Unit {
 route(LoginRoute, DefaultRouteTransition) {
    LoginScreen(navHost, onLogin)
  }
}

// Usage
NavHost(navController = navController, startDestination = MainGraph.route) {
    navigation(LoginGraph) {
        // This
        login { LoginScreen(navController, onLogin = { ... }) }
        // Would turn into this
        login(navController, onLogin = { ... })
    }
 }

While at it, you might be able to automatically provide the navController parameter of the NavHostController type, if it is somehow acquirable from the NavGraphBuilder. In this case the generated extensions function would only require the remaining parameters of the original Screen composable.

public fun NavGraphBuilder.login(onLogin: () -> Unit): Unit {
 route(LoginRoute, DefaultRouteTransition) {
    LoginScreen(  this@NavGraphBuilder.navController??  , onLogin)
  }
}

NavHost(navController = navController, startDestination = MainGraph.route) {
    navigation(LoginGraph) {
        login(onLogin = { ... })
    }
}

The library could generate multiple levels of functions for the NavGraphBuilder, either for the Screens or the whole graphs. So that you could use the generated functions for most of the cases, but still customize anything your way if needed.
For example:

NavHost(navController = navController, startDestination = MainGraph.route) {
    // Just screen extensions
    navigation(LoginGraph) {
        login(onLogin = { ... })
        register(onRegistered = { ... })
    }

   // Graph extension, which would just output the code above
   loginGraph(onLogin = { ... }, onRegistered = { ... })
}

Finally, a complete NavHost with all the graphs could be generated similarly as well. The example above could be made into a single navHost extension function, which would collect all additional parameters from all the underlying graphs and screens:

navHost(navController = navController, onLogin = { ... }, onRegistered = { ... }) 

I think there might be some problems if you want to combine the pre-generated and manual approaches but then you would just revert to the way it is now and define stuff at the level in question manually. But I think that if you take additional care to structure your Screens in a compatible way, a single complete navHost extension from my last example would be all that is needed. But it might be that I'm missing something, so I'm awaiting your answer :)

@levinzonr
Copy link
Owner

// New generated extension
public fun NavGraphBuilder.login(navHost: NavHostController, onLogin: () -> Unit): Unit {
 route(LoginRoute, DefaultRouteTransition) {
    LoginScreen(navHost, onLogin)
  }
}

This I actually tried before, and I would love to pull this in, however, it doesn't work in Compose cases, since NavGraphBuilder is not a composable scope, you won't be able to pass any compose related stuff. If you know a way to do that, I'd would love to take this as an improvement 👍

   loginGraph(onLogin = { ... }, onRegistered = { ... })

An extension like this is also I gave some thought about, and I'm not entirely opposed to that (can be done if we are to solve the issue above), however having a whole navhost extension that takes all the parameters needed for all the destinations, is a no-go for me. For mid-size apps and beyond it will look very weird, to say the least

@cvb941
Copy link
Author

cvb941 commented Jun 6, 2022

// Old generated extension
public fun NavGraphBuilder.login(content: @Composable () -> Unit): Unit {
 route(LoginRoute, DefaultRouteTransition) {
    content()
  }
}
// New generated extension
public fun NavGraphBuilder.login(navHost: NavHostController, onLogin: () -> Unit): Unit {
 route(LoginRoute, DefaultRouteTransition) {
    LoginScreen(navHost, onLogin)
  }
}

But it must be working since the content function is @Composable too. The route function provides the composable scope. Or?

loginGraph(onLogin = { ... }, onRegistered = { ... })

Most of the time you handle all the actions in the screen's ViewModel anyways.

Having to define tons of callbacks looks ugh to me too, however, it kind of encapsulates the way it is and surfaces all the actions that the object can make.

I didn't like to provide a callback for every single action when I wanted to create screens without ViewModels (so that they can be previewed in the Android Studio editor), but Google recommends it that way too so ¯\(ツ)/¯ (for example here by having a Screen and a Content composable)

@levinzonr
Copy link
Owner

But it must be working since the content function is @composable too. The route function provides the composable scope. Or?
The problem is passing the params, consider smth like this:

public fun NavGraphBuilder.login(viewModel: ViewModel, onLogin: () -> Unit): Unit {
 route(LoginRoute, DefaultRouteTransition) {
    LoginScreen(navHost, onLogin)
  }
}

Then in graph builder you might want to pass your viewModel

NavGraphBuilder.graph() { 
   login(hiltViewModel()) { } 
}

Calling hiltViewModel here won't work.

Btw, as of now, I already generate this kind of builder when composable Route doesn't have any parameters. For example not having the navController dependency in your first example, would work as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants