navigation-compose-typed icon indicating copy to clipboard operation
navigation-compose-typed copied to clipboard

Future because the native impl in Navigation 2.8

Open hrach opened this issue 10 months ago • 18 comments

First, there is a resolution conflict, as the NavController has its own navigate method accepting an object. Therefore ours is ignored and this crashes in runtime. (but that is probably better instead of failing silently).

So after the next alpha, there should be finally some more usable API, I plan to take a look and decide how much better/worse is this library and if there is a future for it. If there is a future, we should rename the navigate method not to conflict.

Even though I do not expect the official API to be super "easy" as this one, I don't plan to transform this library to make it "better", that should be ideally a new project. So the most probable solution will be a sunset/deprecation.

hrach avatar Apr 07 '24 19:04 hrach

Yup, when those APIs land and the path to migration is clear it would be perfect for this library to provide just a little documentation on how to do it.

The API conflict is unfortunate, what I did in my testing was typealias the import like

import com.kiwi.navigationcompose.typed.composable as typedComposable

just to quickly unblock myself when I was testing some things out.

I also wonder if the official solution will be as "easy" as this one. From me trying the snapshots right now the first thing which I already did not manage to get working was navigating to destinations which have a List<Foo> in their parameters, as I mention here https://issuetracker.google.com/issues/188693139#comment69

If there are such problems still there in the official implementation, and the workaround is not as simple as one might hope for then I see a place for this library. At least for people who had big projects already built with it.

StylianosGakis avatar Apr 09 '24 08:04 StylianosGakis

The preliminary evaluation:

  • all navigation functionality is there, including pops, etc.
  • complex types have to be defined manually
  • it is not compile type safe - missing types in type map are reported in runtime, but at least immediately after the graph creation
  • even enums are not supported by default (:()
  • the good part is that we may build some abstraction that will "bring" back this, but probably in another library.

Not tested yet:

  • safety on empty strings
  • adaptability for result sharing

hrach avatar May 14 '24 22:05 hrach

My migration is currently blocked by a crash: https://issuetracker.google.com/issues/341319151

hrach avatar May 18 '24 09:05 hrach

Maybe I should have tested more before migrating. https://issuetracker.google.com/issues/341319159 Hm :/

hrach avatar May 18 '24 15:05 hrach

Have you looked at the official library's bottom sheets? They don't have the ability to declare the route as a serializable class yet. This library does :)

cj1098 avatar May 30 '24 21:05 cj1098

For integration of M3 bottom sheets into the navhost (with the official serializable API), you can use my new library: https://github.com/hrach/navigation-compose/

hrach avatar May 30 '24 21:05 hrach

https://issuetracker.google.com/issues/344943214 :/

hrach avatar Jun 04 '24 21:06 hrach

So so far you need to:

  • Wrap types you do not own in your own type even if you have made a serializer for that external class
  • Need to make sure to implement serializeAsValue yourself for all NavTypes
  • And must ensure that the serializeAsValue implementation properly URI encodes everything.

This migration is gonna be a "fun" one I am sure 😅

I know they have made implementing custom NavTypes have at least some friction, according to this https://medium.com/androiddevelopers/navigation-compose-meet-type-safety-e081fb3cf2f8 it's a hint towards pausing and thinking if you want a custom type here or not. But it certainly makes the migration a bit tricky. I think the most sensible thing to make this migration easier is to still just use kotlinx.serialization to put it into the bundle that way instead of also mixing in Parcelable in this. Maybe there's even room for a generic NavType implementation which assumes non-null and always works with Strings to put in the bundle, uses kotlinx serialization for all transformations? Have you migrated any project so far to test this out?

StylianosGakis avatar Jun 05 '24 21:06 StylianosGakis

I am thinking about using the official solution but introducing custom helpers that will do the work for you:

  • a generic JSON Nav Type serializer (I am using it already)
  • composableSomething that will define missing nav types

hrach avatar Jun 06 '24 08:06 hrach

a generic JSON Nav Type serializer (I am using it already)

Yeah that's exactly what I was thinking of when I said "Maybe there's even room for a generic NavType implementation which assumes non-null and always works with Strings to put in the bundle, uses kotlinx serialization for all transformations?" If you have one impl already for this, do you mind sharing it? I haven't had the time to do our migration yet, but I want to very soon and it would really help!

composableSomething that will define missing nav types

What do you mean by this one? I don't think I understand it

StylianosGakis avatar Jun 06 '24 11:06 StylianosGakis

import android.net.Uri
import android.os.Bundle
import androidx.navigation.NavType
import kotlinx.serialization.KSerializer
import kotlinx.serialization.json.Json
import kotlinx.serialization.serializer

@Suppress("FunctionName")
inline fun <reified T : Any?> JsonSerializableNavType(): NavType<T?> =
	JsonSerializableNavType(serializer())

class JsonSerializableNavType<T : Any?>(
	private val serializer: KSerializer<T?>,
) : NavType<T?>(isNullableAllowed = true) {
	override fun get(bundle: Bundle, key: String): T? {
		val data = bundle.getString(key) ?: return null
		return parseValue(data)
	}

	override fun parseValue(value: String): T? {
		if (value == "null") return null
		return Json.decodeFromString(serializer, value)
	}

	override fun put(bundle: Bundle, key: String, value: T?) {
		bundle.putString(key, Json.encodeToString(serializer, value))
	}

	override fun serializeAsValue(value: T?): String {
		if (value == null) return "null"
		return Uri.encode(Json.encodeToString(serializer, value))
	}
}

and then

private val typeMap: Map<KType, @JvmSuppressWildcards NavType<*>> = mapOf(
	typeOf<FilmReview?>() to JsonSerializableNavType<FilmReview?>(),
)

hrach avatar Jun 06 '24 11:06 hrach

(the sad thing is it is serialized twice: string -> object -> Bundle string -> object)

hrach avatar Jun 06 '24 11:06 hrach

Did you also find that you needed one for nullable and one for non-null nav types? Most of ours are non-null and I wonder if I should just do this for them.

class JsonSerializableNavType<T : Any>(
  private val serializer: KSerializer<T>,
) : NavType<T>(isNullableAllowed = false) {
  override fun put(bundle: Bundle, key: String, value: T) {
    bundle.putString(key, value.encodedAsString())
  }

  override fun get(bundle: Bundle, key: String): T {
    return parseValue(bundle.getString(key)!!)
  }

  override fun serializeAsValue(value: T): String {
    return Uri.encode(value.encodedAsString())
  }

  override fun parseValue(value: String): T {
    return value.decodedFromString()
  }

  private fun T.encodedAsString(): String = Json.encodeToString(serializer, this)
  private fun String.decodedFromString(): T = Json.decodeFromString(serializer, this)
}

StylianosGakis avatar Jun 10 '24 15:06 StylianosGakis

I'm using the nullable only. The destination object then checks the nullability so I guess it is ok this way.

hrach avatar Jun 11 '24 07:06 hrach

Btw for the problem of runtime crashes when trying to navigate somewhere with an empty string for a parameter with type String, afaiu kiwi library was going around this by always appending strings as query params instead of part of the path itself. Have you filed a bug report for this which I can star to see what they suggest besides "be careful to not pass empty Strings"?

Because for the official solution if you navigate somewhere with an empty string it will just put nothing there in the path, so it won't find the route so that is a runtime crash. Quite an important difference right here!

StylianosGakis avatar Jun 11 '24 11:06 StylianosGakis

I am sad I had to write this blogpost:

https://hrach.dev/posts/does-jetpack-navigation-meet-type-safety/

hrach avatar Jun 11 '24 12:06 hrach

Hey, that's a great article Jan, it captures all the problems I've encountered myself too while working on this migration, which admittedly has taken many more hours that I would've hoped for. And I am still not 100% confident I've covered all parts of the app. I also gotta admit I am trying to see if this migration will even make our codebase end up in a better spot or not really. We don't even use the result sharing capabilities of this library, but regardless I must admit it feels like at least a bit of a downgrade removing it for the "official type-safe solution". Have you tried to bring this article up to anyone from the developers of the official library to hear their thoughts? And how many of those are just decisions they made consciously and how many they're looking to resolve?

StylianosGakis avatar Jun 11 '24 13:06 StylianosGakis

Not yet, just finished the article today. I'll create tickets for those I haven't yet and link them to the article.

hrach avatar Jun 11 '24 13:06 hrach

https://hrach.dev/posts/does-jetpack-navigation-meet-type-safety/

@hrach, the majority of the issues you mentioned in the article seem to have been fixed. Would you say the Google's navigation lib is up to par now?

Twinsen81 avatar Oct 14 '24 09:10 Twinsen81

@Twinsen81 The built-in functionality is far worse. Though I migrated to it in my app. It is less safe and more verbose, some issues are shown only via lint, though it is usable.

The most annoying thing is to define a custom nav type for every non-trivial serializable type :/

hrach avatar Oct 14 '24 10:10 hrach

Thank you for sharing your opinion! We’ll consider whether migrating is worthwhile in our app.

Twinsen81 avatar Oct 14 '24 10:10 Twinsen81