raamcosta / compose-destinations

Annotation processing library for type-safe Jetpack Compose navigation with no boilerplate.
https://composedestinations.rafaelcosta.xyz
Apache License 2.0
3.23k stars 134 forks source link

Destination argument names instead of values are passed when values are null #661

Closed Nek-12 closed 1 month ago

Nek-12 commented 4 months ago

Related to #660: after hacking around that issue, another surfaced, where string names were passed instead of actual values when opening the start destination and no arguments were provided.

Instead of passing

GFYGamesDestinationNavArgs(
    challengeId = null,
    gameId = null
)

What gets passed is:

GFYGamesDestinationNavArgs(
    challengeId = "{challengeId}",
    gameId = "{gameId}",
)

This seems to happen because GFYGamesDestination is inside the GFYGamesNavGraph and which is the start graph, and that destination is the app start one, leading to it being the start destination of the app, so no arguments were provided during regular (launcher icon) app startup.

The generated code does not seem to have any issues however, at least I couldn't find anything wrong with it.

Upd corrected typo as per comment

dev-weiqi commented 4 months ago

same here, but I got parameterName = "{parameterName}"

raamcosta commented 4 months ago

This is fixed on next release. It was a change on official library 2.8.x where they added functionality to let us use a "resolved" route (i.e with arguments present) as the start destination of the NavHost.

APIs will be updated accordingly on next release, which means a small easy to fix breaking change, more about that will be on release notes.

Nek-12 commented 3 months ago

I have tried this with 2.1.0-beta11 and it doesn't seem to be fixed yet. Is this fix only on 2.0.x or is something preventing this from working? We don't have references to the official navigation library anywhere in the project.

Nek-12 commented 3 months ago

Yeah, confirmed not fixed fully. This is how our graph looks like:

 *
 * * πŸ—ΊοΈ[RootGraph]
 * * βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™β†³πŸ“[GameMaker]
 * * βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™β†³πŸ—ΊοΈπŸ[GamesGraph]
 * * βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™β†³πŸ“πŸ[Games]
 * * βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™β†³πŸ“[Challenges]

And our nav controller definition:

 DestinationsNavHost(
    navGraph = NavGraphs.root, // no start route defined
    engine = navHostEngine,
    navController = navController,
    defaultTransitions = DefaultTransitionStyle,
    // ... 
)

Not quite the usual setup since our root destination is not in the root graph.

dev-weiqi commented 3 months ago

@Nek-12 A bit weird, I had the same issue as you, but it got fixed after I upgraded to 2.1.0-beta11. Here's my code

 *
 * * πŸ—ΊοΈ[GachaBattleNavGraph]
 * * βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™β†³πŸ“πŸ[GachaBattleListRoute]
 * * βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™β†³πŸ“[GachaBattleProgressRoute]
 * * βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™β†³πŸ“[GachaBattleSetupRoute]
 * * βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™βˆ™β†³πŸ“[GachaBattleWaitRoute]
 */

nav controller definition:

val startDirection = when (startDestination) {
    GachaBattleStartDestination.List -> GachaBattleListRouteDestination()
    GachaBattleStartDestination.Setup -> GachaBattleSetupRouteDestination()
    is GachaBattleStartDestination.Wait -> GachaBattleWaitRouteDestination(battleId = startDestination.battleId)
    is GachaBattleStartDestination.Progress -> GachaBattleProgressRouteDestination(battleId = startDestination.battleId)
}

DestinationsNavHost(
    navGraph = NavGraphs.gachaBattle,
    navController = navHostController,
    defaultTransitions = DefaultTransitionStyle,
    start = startDirection,
) {
    // ...
}
Nek-12 commented 3 months ago

The only difference I see from @dev-weiqi 's setup is that they have a start destination defined and we do not and are using the generated method.

miduch commented 2 months ago

same issue with 2.1.0-beta11

miduch commented 2 months ago

same issue with 2.1.0-beta11

Issue seem to have disappeared in 2.1.0-beta12

raamcosta commented 2 months ago

Interesting πŸ€” @Nek-12 could you also check on your side?

Nek-12 commented 1 month ago

Sorry for the long response. We did not have time to upgrade dependencies bc of compose breaking changes. I just tested, no longer repro. Closing the issue.