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

Future because the native impl in Navigation 2.8 #135

Open
hrach opened this issue Apr 7, 2024 · 21 comments
Open

Future because the native impl in Navigation 2.8 #135

hrach opened this issue Apr 7, 2024 · 21 comments

Comments

@hrach
Copy link
Collaborator

hrach commented Apr 7, 2024

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 hrach changed the title Future because the navite impl in Navigation 2.8 Future because the native impl in Navigation 2.8 Apr 7, 2024
@StylianosGakis
Copy link

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.

@hrach
Copy link
Collaborator Author

hrach commented May 14, 2024

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
Copy link
Collaborator Author

hrach commented May 18, 2024

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

@hrach
Copy link
Collaborator Author

hrach commented May 18, 2024

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

@cj1098
Copy link

cj1098 commented May 30, 2024

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 :)

@hrach
Copy link
Collaborator Author

hrach commented May 30, 2024

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
Copy link
Collaborator Author

hrach commented Jun 4, 2024

@StylianosGakis
Copy link

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?

@hrach
Copy link
Collaborator Author

hrach commented Jun 6, 2024

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

@StylianosGakis
Copy link

StylianosGakis commented Jun 6, 2024

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

@hrach
Copy link
Collaborator Author

hrach commented Jun 6, 2024

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
Copy link
Collaborator Author

hrach commented Jun 6, 2024

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

@StylianosGakis
Copy link

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)
}

@hrach
Copy link
Collaborator Author

hrach commented Jun 11, 2024

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

StylianosGakis added a commit to HedvigInsurance/android that referenced this issue Jun 11, 2024
From discussion:
kiwicom/navigation-compose-typed#135 (comment)

This allows us to keep our more complex types in navigation still using
kotlinx.serialization to turn them into strings and back for nav to use
@StylianosGakis
Copy link

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!

@hrach
Copy link
Collaborator Author

hrach commented Jun 11, 2024

I am sad I had to write this blogpost:

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

@StylianosGakis
Copy link

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?

@hrach
Copy link
Collaborator Author

hrach commented Jun 11, 2024

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

StylianosGakis added a commit to HedvigInsurance/android that referenced this issue Jun 11, 2024
From discussion:
kiwicom/navigation-compose-typed#135 (comment)

This allows us to keep our more complex types in navigation still using
kotlinx.serialization to turn them into strings and back for nav to use
@Twinsen81
Copy link

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?

@hrach
Copy link
Collaborator Author

hrach commented Oct 14, 2024

@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 :/

@Twinsen81
Copy link

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

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

No branches or pull requests

4 participants