kiwicom / navigation-compose-typed

Type-safe arguments for Jetpack Navigation Compose using Kotlinx.Serialization
MIT License
226 stars 9 forks source link

Default arguments for startDestination #40

Closed StylianosGakis closed 1 year ago

StylianosGakis commented 1 year ago

I had the following use case: I want to use a NavHost for a specific part of the app (trying out compose navigation in an Activity-based application) but the screen which I am navigating to needs the home destination to be passed in an argument. This isn't quite possible by default using createRoutePattern.

Some more context from this discussion https://slack-chats.kotlinlang.org/t/9627868/i-want-to-adopt-navigation-compose-in-an-app-where-we-got-ac#8b324622-3961-4bcd-975d-87a38b57ae63

What I want to do basically is to follow Ian's advice to set the default value myself, something which is not possible with the current APIs of this library. Maybe rightly so, but I'm gonna go ahead and discuss what I've done because I want to hear your thoughts on it.

To do this, I need to basically override either the entire createNavArguments, or make it accept some extra configuration. In order not to throw the entire createNavArguments away, and lose the ability to change the data class without having to touch this code again, we can give ourselves the power to instead provide a custom function which acts on the NavArgumentBuilder for a specific field name. Again, a bit type-unsafe as we'll rely on the name of the field, but maybe there's something that can be done there too. Here's what I've done which worked for me:

Create this createNavArguments function:

@ExperimentalSerializationApi
public fun createNavArguments(
  serializer: KSerializer<*>,
  extraNavArgumentConfiguration: Map<String, NavArgumentBuilder.() -> Unit> = emptyMap(), // ⬅️ Added this
): List<NamedNavArgument> =
  List(serializer.descriptor.elementsCount) { i ->
    val name = serializer.descriptor.getElementName(i)
    navArgument(name) {
      // Use StringType for all types to support nullability for all of them.
      type = NavType.StringType
      val isOptional = serializer.descriptor.isNavTypeOptional(i)
      nullable = isOptional
      // If something is optional, the default value is required.
      if (isOptional) {
        defaultValue = null
      }
      // ⬇️ Extra 2 lines
      val extraConfiguration = extraNavArgumentConfiguration[name] ?: return@navArgument
      extraConfiguration()
    }
  }

And we use this one by making the two composable functions take this extra configuration:

@ExperimentalSerializationApi
@MainThread
internal inline fun <reified T : Destination> NavGraphBuilder.composable(
  deepLinks: List<NavDeepLink> = emptyList(),
  extraNavArgumentConfiguration: Map<String, NavArgumentBuilder.() -> Unit> = emptyMap(), // ⬅️ Added this
  noinline content: @Composable T.(NavBackStackEntry) -> Unit,
) {
  composable(
    kClass = T::class,
    serializer = serializer(),
    deepLinks = deepLinks,
    extraNavArgumentConfiguration = extraNavArgumentConfiguration,
    content = content,
  )
}

@ExperimentalSerializationApi
@MainThread
fun <T : Destination> NavGraphBuilder.composable(
  kClass: KClass<T>,
  serializer: KSerializer<T>,
  deepLinks: List<NavDeepLink> = emptyList(),
  extraNavArgumentConfiguration: Map<String, NavArgumentBuilder.() -> Unit> = emptyMap(), // ⬅️ Added this
  content: @Composable T.(NavBackStackEntry) -> Unit,
) {
  registerDestinationType(kClass, serializer)
  composable(
    route = createRoutePattern(serializer),
    arguments = createNavArguments(serializer, extraNavArgumentConfiguration), // ⬅️ Call the new createNavArguments here
    deepLinks = deepLinks,
  ) { navBackStackEntry ->
    decodeArguments(serializer, navBackStackEntry).content(navBackStackEntry)
  }
}

This makes the API of calling this look like this:

composable<CancelInsuranceDestinations.TerminationDate>(
  extraNavArgumentConfiguration = mapOf(
    "insuranceId" to { defaultValue = insuranceId.id },
  ),
) { ... }

Do you feel like this use case is valuable enough to warrant some addition to the existing APIs?

If no I can keep this local function and live with it, I assume I will have to use it more in the future since I'm gonna be doing this slow migration, but that would be fine.

If yes, how would you feel this could be done in a better way?

p.s. the naming of everything was done in haste just to see if this works, some more careful decisions can be made for sure. p.p.s. You're a genius for making this library 😅 A very interesting way to take what kotlinx.serialization has to offer and use it for this! I am so glad you brought this to my attention on this slack discussion, so I really wanted to try it out. The ability to use what this library has to offer optionally wherever you want, and drop down to manual handling (like in my case) when you want to is so convenient!

hrach commented 1 year ago

We have faced the issue as well but somehow we were able to workaround it this way:

@Serializable
data class HomeDestination(
 val userId: Int?,
) : Destination

@Composable
fun AppActivityNavHost(userId: Int) {
  NavHost(
    startDestination = createRoutePattern<HomeDestination>(),
  ) {
    composable<HomeDestination> {
      HomeDestination(userId ?: this@AppActivityNavHost.userId)
    }
  }
}

Alternatively, if you really need non nullable userId, we were considering having the destination twice:

@Serializable
data object HomeDestination : Destination

@Serializable
data class UserDestination(
  val userId: Int,
) : Destination

@Composable
fun AppActivityNavHost(userId: Int) {
  NavHost(
    startDestination = createRoutePattern<HomeDestination>(),
  ) {
    composable<HomeDestination> { HomeDestination(userId) } // here it references the arg
    composable<UserDestination> { HomeDestination(userId) } // here it references the property
  }
}

But it is true that I've considered some explicit support for this. What I was thinking was rather making it work like this:

  1. Have a custom composable which takes the destination instance itself.
  2. The decodeArguments will take this destination instance and will read those default directly from it, if they are not in the URL.
@Serializable
data class UserDestination(
  val userId: Int,
) : Destination

@Composable
fun AppActivityNavHost(userDestination: UserDestination) {
  NavHost(
    // special pattern builders needed to make all those args optional -> it won't loose the 
    // typesafety at it is based on the user-specified type; though this could crash if
    // the standard createRoutePattern() would be used here
    startDestination = createRoutePatternWithDefaults(userDestination),
  ) {
    composableWithDefaults<HomeDestination>(userDestination) { HomeDestination(userId) }
  }
}

This way we could also utilize everything or nothing pattern. Trying to reuse defaultValue seemed a bit error-prone.

StylianosGakis commented 1 year ago

Hmm yeah, I think the double declaration and the nullable options sound like good and safe workarounds for right now, to remove my little hack, thanks for that!

As far as a more explicit support for this goes. I think the WithDefaults option sounds very reasonable. The ability to misuse it is definitely tricky, also the ability to surface this API might be tricky too, as before one meets this problem, they may wonder why this exists in the first place. I wonder if these could be mitigated by some lint checks or something like this.

Since you've thought of this before, have you not implemented it because: You tried it out and didn't find an API that you liked You were satisfied with doing the workarounds You just didn't get down to it yet

Or maybe something else? I personally would love to use and test this out.

hrach commented 1 year ago

We are using the workarounds for now. Though it is probably more reasonable for us since we don't need to define the parameter at all:

@Serializable
data object HomeDestination : Destination

@Composable
fun AppActivityNavHost(userId: Int) {
  NavHost(
    startDestination = createRoutePattern<HomeDestination>(),
  ) {
    composable<HomeDestination> {
      HomeDestination(userId)
    }
  }
}

I quickly tried some prototyping, but I was also in the middle of something else so I scratched that in the end.

But somehow I still feel that I don't have the best picture what is the use case for this. In the full compose app, it doesn't seem that you would utilize a parameter for the start destination. So we're probably talking about:

What do you think? Are there other use cases? What's yours?

StylianosGakis commented 1 year ago

Yeah I played a bit more with this and you are right, the workarounds are plenty sufficient, without increasing the API surface of this library unnecessarily. I went with those and it's honestly working perfectly fine! Thanks again for the discussion, I am very confident with not making any changes for this and working these approaches to solve this issue. I think we can close this tbh, will do it myself and feel free to re-open it if you want for some other reason.

hrach commented 1 year ago

Ok, let's close it now and see later if somebody has those needs as well.