xxfast / Decompose-Router

A Compose-multiplatform navigation library that leverage Decompose to create an API inspired by Conductor
https://xxfast.github.io/Decompose-Router/
221 stars 9 forks source link

Added function to retrieve instance from backstack #76

Closed forhad013 closed 7 months ago

forhad013 commented 10 months ago
forhad013 commented 8 months ago

https://github.com/xxfast/Decompose-Router/assets/9871524/7a2bf994-a7e4-4a5c-be54-668a06534d58

This getInstanceFromBackStack is causing an error for one case in the sample app.

Screenshot 2023-12-16 at 3 01 22 PM

In the sample app, We are updating the instance after returning from DetailScreen.

When we go back from DetailScreen for the following condition, we get the error.

if (counted.size == count) onBack()

But on navigationBackIcon action, getInstanceFromBackStack does not give an error. I will look into this.

arkivanov commented 8 months ago

Also, I think Router may have all configurations of the same type. E.g. it's pretty common to have something as follows.

@Serializable
data class Config(val imageId: String, val imageUrl: String)
xxfast commented 8 months ago

Hi @forhad013. I'm adding a little context to help @arkivanov

This function retrieves any instance that is remembered on the route.

The use case is a typical form screen flow, where users select a field from the Form screen that navigates them to an AddressDetails screen. In the AddressDetails screen, users provide some information that gets passed back to the Form screen's instance, to update the Form state.

For example, we have the following configurations

sealed class FormRootScreens {
  object Form: FormRootScreens
  data class AddressDetails(val address: Address): FormRootScreens
}

Which is hoisted in a router at some root level like

@Composable
fun FormRootScreen(){
  val router: Router<FormRootScreens> = rememberRouter { listOf(Form) } 

  RoutedContent(router) { screen -> 
    when(screen){
      Form -> FormScreen(onSelect = { address -> router.push(AddressDetails(address)) } )
      AddressDetails -> AddressScreen(screen.address)
    }
  }
}

The FormScreen has an instance that is remembered on the route that has a state that keeps track of user edits on the form in memory, that only gets written to disk when that screen is saved/finalised

data class FormState(
  val addresses: List<Address> = emptyList(),
  ...
)

class FormInstance() : Instance {
  private val stateFlow: MutableStateFlow<FormState> = MutableStateFlow(FormState())
  val state: StateFlow<FormState> = stateFlow

  suspend fun add(address: Address){
    val previous: FormState = stateFlow.value
    val updated: FormState = previous.copy(addresses = previous.addresses + address)
    stateFlow.emit(updated)
  }

  fun onSave() = TODO("Write this address to disk")
}

@Composable
fun FormScreen(
  onSelect: (Address) -> Unit 
) {
  val instance: FormInstance = rememberOnRoute { FormInstance() }

  val state: FormState by instance.collectAsState()

  FormView(
    state = state,
    onSelect = onSelect
    onSave = instance::onSave
}

This API will help the AddressDetailsScreen to be able to reference that FormInstance that was remembered on the backstack, and update its state

data class AddressDetailsState(
  val address: Address,
  ...
)

class AddressDetailsInstance(val address: Address) : Instance {
  private val stateFlow: MutableStateFlow<AddressDetailsState> = MutableStateFlow(AddressDetailsState(address))
  val state: StateFlow<AddressDetailsState> = stateFlow
}

@Composable
fun AddressDetailsScreen(
  address: Address
) {
  val instance: AddressDetailsInstance = rememberOnRoute(key=address) { AddressDetailsInstance(address) }

  val state: AddressDetailsState by instance.collectAsState()

  AddressDetailsView(
    state = state,
    onSave = {  address ->  getInstanceFromBackStack<FormInstance>().add(address) }
  )
}

I believe that is the whole use case for this. @forhad013 correct me if I recall this incorrectly.

xxfast commented 8 months ago

Admittedly getInstanceFromBackStack - even though it is accurate to what it is doing - is I think a little verbose name 🤔

I'm planning to add support for other navigation models (like pages, and slots) to decompose-router, and the word BackStack might be meaningless in that context.

Therefore I think we should omit the word BackStack from this API, and simply call it getOnRoute? What do you think @forhad013 ?

arkivanov commented 8 months ago

Thanks for the context. My personal opinion is that this API is a bit error-prone, and it also encourages the usage of instances (aka ViewModels) from the sibling screens.

It's also not quite flexible, e.g. it won't be able to access any instance from the upper level. Once you wrap an existing child into another one, or decompose a child into multiple sub-children with a common parent, the now nested child loses the access to the retained instance it has been using so far. This will be a runtime crash.

Personally, I would prefer scoping that shared instance at the parent level, and pass it down the tree manually via arguments.