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

StateFlowExtesions #127

Closed
wants to merge 9 commits into from
Closed

StateFlowExtesions #127

wants to merge 9 commits into from

Conversation

GuilhE
Copy link
Contributor

@GuilhE GuilhE commented Apr 11, 2022

This PR adds StateFlowExtesions. This necessity was born from #122 where the author asks if it is possible to "hide" the ContainerHost.
And It is, by doing:

/**
 * Just to keep it internal without public access.
 * @param initialState The initial state of the container.
 */
internal fun <STATE : Any, SIDE_EFFECT : Any> CoroutineScope.containerHostVisibilityWrapper(initialState: STATE) =
    object : ContainerHost<STATE, SIDE_EFFECT> {
        override val container = this@containerHostVisibilityWrapper.container<STATE, SIDE_EFFECT>(initialState)
    }

using it like:

data class MyState(val message: String = "")

class MyViewModel : ViewModel() {
    private val host = viewModelScope.containerHostVisibilityWrapper<MyState, Nothing>(MyState())
    val stateFlow = host.container.stateFlow
}

But unfortunately, we need a ContainerHost<STATE, SIDE_EFFECT> to use compose extensions: collectSideEffect, collectState and collectAsState.

With this PR, we're able to use them - collectSideEffectLifecycleAware, collectStateLifecycleAware, collectAsStateLifecycleAware -, like:

@Composable
fun Screen(viewModel: MyViewModel) {
    with(viewModel.stateFlow.collectAsStateLifecycleAware().value) {
         ...
    }
}

@mattmook
Copy link
Contributor

mattmook commented Jun 24, 2022

Been discussing in depth with @Rosomack over the last hour or so and, long story short, we're not going to merge the PR because:

  • it increases the API surface for little reason other than making it easier for a small subset of ppl to hide the container
  • the functions feel like they are better placed as part of something like androidx.compose.runtime or a separate library, especially given there is no orbit code in the new functions

We discussed other ways to hide the container hosts functions, such as intent, but don't feel there's a good way without introducing additional boilerplate for the user or significant extra complexity internally, against some of our original principles when creating orbit.

Ultimately, we don't think this is a problem - the encapsulation can still be done in your own project if you so choose

@mattmook mattmook closed this Jun 24, 2022
@GuilhE
Copy link
Contributor Author

GuilhE commented Jun 24, 2022

Thanks for the feedback 😉

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

Successfully merging this pull request may close these issues.

None yet

2 participants