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

Question around deep link parameters involving enums #112

Open
StylianosGakis opened this issue Jan 26, 2024 · 5 comments
Open

Question around deep link parameters involving enums #112

StylianosGakis opened this issue Jan 26, 2024 · 5 comments
Labels
question Further information is requested

Comments

@StylianosGakis
Copy link

This is not a bug per-se, but more of an attempt from me to ask you about your thought about how to best work with enums in destinations when also having to work with deep links.
After looking at the implementation and debugging some code, it turns out that enums are serialized simply by their ordinal. In my case I got a destination with a nullable enum class as a parameter. For the deep link, I can add it to the uriPattern as ...?paramName={paramName}, and then when I create an actual deep link for it, I need to do ...?paramName=0 or whatever index of that enum over there.
This means that if for example we do a follow-up release with a new enum entry, if I try to send that deep link to people out in the wild, if they still got an old version of the app, trying to open the deep link will simply crash their app, as kotlinx.serialization will try to decode that by running this line https://github.com/Kotlin/kotlinx.serialization/blob/1116f5f13a957feecda47d5e08b0aa335fc010fa/core/commonMain/src/kotlinx/serialization/internal/Enums.kt#L140 and since the index is not there it crashes the entire application.
My crash for example is kotlinx.serialization.SerializationException: 5 is not among valid com.android.MyClass enum values, values size is 5

What do you think is the best approach to make this process a bit more robust and forwards compatible? I could have the type in the destination itself be a String? for example, and then have to do in my own code a check to find any possible matches, or default to my own entry if not, something like:

val input = destinationInstance.deepLinkPopulatedField
val resolvedEnum = MyEnum.entries.firstOrNull {
  it.name == input
} ?: MyEnum.Default

Am I missing some smarter approach here? Have you encountered this before perhaps, and if yes what did you end up doing?

@hrach
Copy link
Collaborator

hrach commented Jan 26, 2024

My POV: any serious deep-linking should be separate from the navigation stack, i.e. have a separate URL parser that constructs the destination and then navigates "programmatically".

Anyway, I'll accept a PR using a name instead of an ordinal, though the issue stays - it's kind of implementation detail.

@hrach hrach added the question Further information is requested label Jan 26, 2024
@hrach
Copy link
Collaborator

hrach commented Jan 26, 2024

I'll accept a PR using a name instead of an ordinal, though the issue stays - it's kind of implementation detail.

I was thinking about this change a few days ago - but my motivation was clarity when debugging (we send those navigation urls to Sentry as breadcrumbs)

@StylianosGakis
Copy link
Author

have a separate URL parser that constructs the destination and then navigates "programmatically".

Not 100% sure I follow here. Could you elaborate a bit on this? I do like the fact that androidx.navigation kinda handles everything for you normally even for deep links, as long as they are constructed well, with the right parameter names etc.
Is whatever you've done here to fix this issue instead also something that you could share? I'm quite curious.

And changing it to the name even for debugging purposes is interesting, so I do wonder if and how many people may have been relying on the existing behavior with the numbers where such a bump would break them. And if so, what could they do to keep this working again?

@hrach
Copy link
Collaborator

hrach commented Jan 31, 2024

Kiwi.com motivations having it handled externally are:

  1. Handling BCs, preprocessing, etc.
  2. Historical reasons - we are not 100% navigation composed.
  3. Some additional logic (landing to a splash activity that redirects to other activities).

Also, I like the concise nav-graph definition and expanding the def with deeplinks is quite making the hierarchy less readable.

I do wonder if and how many people may have been relying on the existing behavior with the numbers where such a bump would break them. And if so, what could they do to keep this working again?

TBH no idea here - I even don't know how exactly that works as I have never used it.

@StylianosGakis
Copy link
Author

I see yeah, different requirements indeed.
Just to give you some context on how we have been using this is that you use this library normally, and define your route like this, where the screen is this and the deeplink is this and all this works perfectly fine since there are no arguments or whatever involved, simple.

Where they are involved, it's the same setup, but what I had done is try to kinda go backwards from what this library generates as a route for a destination, taking into consideration how some things are optional arguments, some go in the path etc, and then write the route myself adding the right name from the parameter name, with the line
override val uriPattern: String = "$baseDeepLinkDomain/chat?$chatContextParameterName={$chatContextParameterName}" in this draft PR where I was looking into this.

But I see your point as to how this library is not necessarily 100% focused on making that part work. If you have any more thoughts on this I'd love to hear them, if not I will see if I can come up with something better for ourselves, and share it here. Otherwise I might just do this manual work as I show in that draft PR and go from there 😊

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

No branches or pull requests

2 participants