Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Android Lifecycle integration #158

Merged
merged 23 commits into from
Jun 8, 2020
Merged

Android Lifecycle integration #158

merged 23 commits into from
Jun 8, 2020

Conversation

aballano
Copy link
Member

@aballano aballano commented May 8, 2020

Same API as the Kotlinx coroutines integration that provides auto cancelation for LifecycleOwners (Activities, Fragments, etc.). This part you can already review :)

I also added a deferUntilActive function that allows devs to defer the emission of an IO until the LifecycleOwner is in the Create state, so it should behave like a LiveData.

TODO:

  • Figure out if it's possible to move the root build.gradle's added config for android to the specific module Thanks @rachelcarmena!
  • Enable publish and submodule config on the module (currently failing)
  • We might need to add the android sdk to the CI somehow to test this module
  • Finish the tests for deferUntilActive removed for now

Fixes #152

owner: LifecycleOwner,
cb: (IOResult<E, A>) -> Unit
) {
if (owner.lifecycle.currentState.isAtLeast(State.CREATED)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this initial state required?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise we potentially might launch an IO after the onDestroy phase and therefore might never be cancelled

Copy link
Member

@JorgeCastilloPrz JorgeCastilloPrz May 25, 2020

Choose a reason for hiding this comment

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

Doesn't it feel a bit confusing from the user perspective? If the condition is not matched the dev doesn't get any error but the job is not run.

If the condition is not matched we could throw, if we wanna be strict (this is already an unsafe run), or alternatively, we could install a new lifecycle observer in that case, so it runs the job automatically if the lifecycle reaches de CREATED state afterwards. (Note this could get called earlier in the lifecycle).

I've not dug into the implementation yet, but I'd say something like that is what the Android lifecycle aware coroutines do under the hood with those launchWhenXXXX methods.

Copy link
Member Author

@aballano aballano May 25, 2020

Choose a reason for hiding this comment

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

(Note this could get called earlier in the lifecycle)

I don't think so, at least from what I understood here https://developer.android.com/reference/androidx/lifecycle/Lifecycle.State#summary the CREATED state is the minimum active state you can have, before it there's only DESTROYED

or alternatively, we could install a new lifecycle observer in that case, so it runs the job automatically if the lifecycle reaches de CREATED state afterwards.

As far as I'm concerned that wouldn't work because of this (taken from the launchWhenXXX fun)

image

(where handleDestroy is simply cancelling the parent and finishing)

I've not dug into the implementation yet, but I'd say something like that is what the Android lifecycle aware coroutines do under the hood with those launchWhenXXXX methods.

Note that we're in the unsafeRunScoped function which is different from the launchWhenXXX, the latter can be easily implementing by simply flatmapping an ioOfEvent to the IO we want to start at that state:

IO.async { cb ->	
    owner.lifecycle.addObserver(object : LifecycleEventObserver {	
      override fun onStateChanged(source: LifecycleOwner, event: Event) {	
        if (event == eventTarget) {	
          source.lifecycle.removeObserver(this)	
          cb(IOResult.Success(Unit))	
        }	
      }	
    })	
  }

@github-actions
Copy link

github-actions bot commented May 15, 2020

Hood benchmark comparison:
⚠️ Concurrentqueue (Threshold: 15.175393547005855)

Benchmark Value
master-branch 217.07235968225086
pull-request 213.08579769534376

@aballano
Copy link
Member Author

aballano commented May 22, 2020

@nomisRev I'm ditching out the deferUntilActive fun since it might not be that simple: what livedata does is have an inner count of auto-removal-subscribers to track if it's active, which doesn't make sense for IO but for Stream I presume. Also with the other changes coming to suspend I think we could safely revisit the launchWhenXX syntax then.

Feel free to locally revert to 2eed439 to see the function

@aballano aballano marked this pull request as ready for review May 22, 2020 13:27
@rachelcarmena
Copy link
Member

@aballano , I fixed the first TODO item in #162.

About the second one:

  • Enable publish and submodule config on the module (currently failing)

They are failing because they are based on Java plugin and it's not compatible with the Android plugins. It seems Android modules will need a different configuration for testing and publication. Let's research it! 🙌

@nomisRev
Copy link
Member

@rachelcarmena yes, releasing Android involves a different setup. @JorgeCastilloPrz has some released Android libraries, he can help us there.

@JorgeCastilloPrz
Copy link
Member

JorgeCastilloPrz commented May 25, 2020

@rachelcarmena @nomisRev In this repo you can see how the deploy script for the android module is slightly different from the pure kotlin one. The difference is pretty much where to find sources and doc sources.

arrow-fx-android/build.gradle Outdated Show resolved Hide resolved
arrow-fx-android/build.gradle Outdated Show resolved Hide resolved
Copy link
Member

@rachelcarmena rachelcarmena left a comment

Choose a reason for hiding this comment

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

It looks good to me!! The configuration can be re-organized afterwards to be able to re-use it for other Android modules. 🙌

Copy link
Contributor

@danimontoya danimontoya left a comment

Choose a reason for hiding this comment

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

good job alberto!! 🙌

@nomisRev nomisRev merged commit b1c218e into master Jun 8, 2020
@nomisRev nomisRev deleted the ab/lifecycle-integration branch June 8, 2020 14:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

["Request"] Android Lifecycle
6 participants