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

Add observe extensions for Flow to reduce nesting and boilerplates #4338

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Goooler
Copy link
Contributor

@Goooler Goooler commented Mar 27, 2024

No description provided.

@Goooler
Copy link
Contributor Author

Goooler commented Mar 27, 2024

This depends on #4337.

@Goooler Goooler force-pushed the observe-flow branch 2 times, most recently from 93c013c to b83783e Compare March 27, 2024 10:37
@@ -51,6 +51,7 @@ android {

kotlinOptions {
freeCompilerArgs = [
"-Xcontext-receivers",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 109 to 112
context(LifecycleOwner)
fun <T> Flow<T>.observe(
collector: FlowCollector<T>? = null
): Job = observe(this@LifecycleOwner, collector)
Copy link
Contributor Author

@Goooler Goooler Mar 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can use Fragment's viewLifecycleOwner instead of itself for all invocations in Fragments? This means we can unify

  viewModel.uiEvents.observe(viewLifecycleOwner) {
      // Some logic.
  }

and

  viewModel.uiEvents.observe {
      // Some logic.
  }

to the same in Fragments, and sync with root Views' lifecycles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished in 38bdb76, feel free to review.

@connyduck
Copy link
Collaborator

That is nice, but also quite a big change. What do you think @charlag?

@connyduck connyduck requested a review from charlag March 27, 2024 11:25
@cbeyls
Copy link
Contributor

cbeyls commented Mar 30, 2024

Be careful of subtle behavior changes: most of the current code doesn't use repeatOnLifecycle() but the new observe() functions do. Code that used to run once could now be repeated and in some cases that's not what you want.

@cbeyls
Copy link
Contributor

cbeyls commented Mar 30, 2024

I think a balance needs to be found between clarity, flexibility and reducing nesting.
IMO the repeating part should be optional and mentioned explicitly. I also believe it's better to mention the lifecycle explicitly for clarity.

I think having two levels of nesting is already fine:

lifecycle.launch {
    flow.collect {
    }
}

A utility function launchAndRepeatOnLifecycle could be created for cases where you want the repeating behavior:

fun Lifecycle.launchAndRepeatOnLifecycle(
    state: Lifecycle.State = Lifecycle.State.STARTED,
    block: suspend CoroutineScope.() -> Unit
): Job = coroutineScope.launch {
    repeatOnLifecycle(state, block)
}

fun LifecycleOwner.launchAndRepeatOnLifecycle(
    state: Lifecycle.State = Lifecycle.State.STARTED,
    block: suspend CoroutineScope.() -> Unit
): Job = lifecycle.launchAndRepeatOnLifecycle(state, block)

Then to collect repeatedly, also two levels of nesting:

lifecycle.launchAndRepeatOnLifecycle {
    flow.collect {
    }
}

Note that you can also collect multiple things inside a lifecycle repetition by launching child coroutines. This is also more efficient than launching multiple instances of repeatOnLifecycle():

lifecycle.launchAndRepeatOnLifecycle {
    launch {
        flow1.collect {
        }
    }
    launch {
        flow2.collect {
        }
    }
}

In that case it would indeed create 3 levels of nesting. But the collecting code could also be moved to separate suspending functions to improve readability.

@Goooler
Copy link
Contributor Author

Goooler commented Mar 31, 2024

Be careful of subtle behavior changes: most of the current code doesn't use repeatOnLifecycle() but the new observe() functions do.

That is what I'm considering too, remove it for now.

@Goooler Goooler changed the title Add observe extensions for Flow to reduce nesting Add observe extensions for Flow to reduce nesting and boilerplates Mar 31, 2024
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

3 participants