mozilla-mobile / firefox-tv

Firefox for Amazon's Fire TV
https://blog.mozilla.org/blog/2017/12/20/firefox-is-now-on-amazon-fire-tv-happy-holiday-watching/
Mozilla Public License 2.0
255 stars 109 forks source link

[META] Adopt RxJava to avoid reimplementing operators in LiveData #1783

Closed mcomella closed 5 years ago

mcomella commented 5 years ago

edit: Morphed into meta bug

Note: if possible, we'd prefer incremental adoption so this doesn't require much sprint planning. As dnarcese mentioned, this means we'll need to be careful when working on related code so we don't run into rebasing issues after systemic refactors.


Why/User Benefit/User Problem

Our architecture relies on the reactive programming model. We're using the LiveData API to execute this but LiveData falls short in some essential operators. In particular, we rely on combineLatest to allowing combining multiple pieces of data. We reimplemented this for two streams but now I'm in a situation where I need to combine 3 (#1628); it's only a matter of time before we need to combine 4. This code requires duplication to write (making it hard to make consistent bug fixes, tests) so it'd be preferable to use a library that handles this for us.

Some options: we can find a library that wraps LiveData to provide these operators or use RxJava or other ReactiveStreams APIs.

However, some of these are not decisions to be taken lightly: e.g. RxJava is notorious for its complexity and difficulty onboarding new users. Also, it'll take some thinking to identify the patterns we need to convert LiveData – which has lifecycle events to worry about – to Rx and vice versa.

What / Requirements

Acceptance Criteria (how do I know when I’m done?)


fwiw, some initial thinking on using Rx:

severinrudie commented 5 years ago

Another possibility would be moving away from LiveData entirely. Uber has released a library that lets us replicate the subscription behavior of LiveData with Rx observables. A few things to note, if we use it though:

psymoon commented 5 years ago

@Baron-Severin maybe we can add and extend to this https://github.com/mozilla-mobile/android-components/blob/master/components/support/ktx/src/main/java/mozilla/components/support/ktx/android/arch/lifecycle/Lifecycle.kt ?

severinrudie commented 5 years ago

I would be pretty wary about reimplementing this kind of thing. I know the team at Google that worked on LiveData had to call up some of the programmers who had originally worked on fragment transactions in order to get all of the edge cases correct. And a really popular OSS project for ~typing~ tying Rx to Android lifecycles (RxLifecycle) was eventually deprecated in favor of either doing it by hand or using Uber's solution, because it turns out it's just a hard problem to solve and they never quite got it right.

mcomella commented 5 years ago

As a team, we decided to adopt a more advanced FRP library than LiveData. We've lightly decided on Rx over other libraries because it's ubiquitous on Android (and thus well tested, well documented) and in use by other teams at Mozilla.

As such, this bug is being morphed into a meta bug.

mcomella commented 5 years ago

We're converting this gradually when we need to use Rx operators or need to combine a LiveData and Rx stream (in which case we convert the LiveData stream): I don't think we need to keep an issue open for this.