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

MBL-1068: Remove RXJava1 and related dependencies from our source code. #2170

Merged
merged 26 commits into from
Nov 19, 2024

Conversation

Arkariang
Copy link
Contributor

@Arkariang Arkariang commented Nov 15, 2024

📲 What

Wrapping up a huge migration the android team started long ago.
Android architecture component ViewModel and Android architecture component Lifecycle was adopted to properly handle lifecyle.

All of these dependencies can be removed now regarding lifecycle, (even the authors of the libraries stopped using them a while back, if you wanna know more take a look here ):

implementation "com.trello:rxlifecycle:$rx_lifecycle_version"
implementation "com.trello:rxlifecycle-components:$rx_lifecycle_version"

Removed all helper dependencies to make Views become reactive. For Views we communicate with the VM via listeners, and RXJava2 migration has allowed us to use compose safely via
Completable.subscribeAsState

 implementation "com.jakewharton.rxbinding:rxbinding:$rx_binding_version"
 implementation "com.jakewharton.rxbinding:rxbinding-recyclerview-v7:$rx_binding_version"
 implementation "com.jakewharton.rxbinding:rxbinding-support-v4:$rx_binding_version"  

Removed retrofit wrapper for RXJava1 no longer needed
implementation "com.squareup.retrofit2:adapter-rxjava:$retrofit_version"

Removed as well from the Environment object all the RXJava1 versions for all the networking clients, User and Configuration objects etc ...

🤔 Why

Migration completed!! It was done in small chunks:

👀 See

No user facing changes, this is the last piece for a very long ongoing migration :)

| | |

📋 QA

  • In order to test the mofications made for CurrentUser, log in, kill the app, and open the app again several times, you should not see any UI tied to logged out users.

Story 📖

MBL-1068

.map { it.getValue() }
.skip(1)
.filter { `object`: User? -> `object`.isNotNull() }
.subscribe { u: User? ->
Copy link
Contributor Author

@Arkariang Arkariang Nov 15, 2024

Choose a reason for hiding this comment

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

Subscription to the internal user observable is no longer needed. The user is persisted and emited on Login() method and refresh() method.

- Make sure the internal user observable is private and cannot have new emissions to the observable from outside `CurrentUser` object.
@@ -0,0 +1,15 @@
package com.kickstarter.libs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just migrated from Java to kotlin

if (accessToken.isNotNull() && accessToken.isNotEmpty()) {
client.fetchCurrentUser()
.doOnError {
forceLogout(it.message ?: "")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When app comes back from background refresh the current logged in user. If any error happens force a logout.

@@ -48,52 +48,56 @@ class DiscoveryPagerAdapter(
* Passes along root categories to its fragment position to help fetch appropriate projects.
*/
fun takeCategoriesForPosition(categories: List<Category>, position: Int) {
Observable.from(fragments)
Observable.fromIterable(fragments)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Quite sure we can get rid in the adapter of all the RXJava subscriptions. Seems the only goal of having an observable from a collection and operate on it instead of directly operating on the collection (by not having it be mutable) is to avoid concurrency issues.
Should create a ticket on our techDebt backlog to addres it outside the RXJava migration

@@ -61,9 +61,9 @@ class RewardCardAdapter(private val delegate: Delegate) : KSAdapter() {
fun takeCards(cards: List<StoredCard>, project: Project) {
sections().clear()
addSection(
Observable.from(cards)
Observable.fromIterable(cards)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as here

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 51.61290% with 15 lines in your changes missing coverage. Please review.

Project coverage is 68.97%. Comparing base (e62ebf8) to head (1f46d6f).

Files with missing lines Patch % Lines
...rc/main/java/com/kickstarter/libs/CurrentUserV2.kt 63.63% 3 Missing and 1 partial ⚠️
app/src/main/java/com/kickstarter/libs/Logout.kt 42.85% 4 Missing ⚠️
...kickstarter/libs/utils/ApplicationLifecycleUtil.kt 0.00% 4 Missing ⚠️
...om/kickstarter/libs/utils/extensions/ProjectExt.kt 0.00% 2 Missing ⚠️
...tarter/viewmodels/FacebookConfirmationViewModel.kt 0.00% 0 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2170      +/-   ##
============================================
+ Coverage     68.85%   68.97%   +0.11%     
+ Complexity     2225     2162      -63     
============================================
  Files           366      346      -20     
  Lines         23165    22845     -320     
  Branches       3360     3346      -14     
============================================
- Hits          15951    15758     -193     
+ Misses         5403     5288     -115     
+ Partials       1811     1799      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@Arkariang Arkariang marked this pull request as ready for review November 15, 2024 21:37
@Arkariang Arkariang self-assigned this Nov 15, 2024
Copy link
Contributor

@leighdouglas leighdouglas left a comment

Choose a reason for hiding this comment

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

yayayayayayayay! 🎉

Copy link
Contributor

@jlplks jlplks left a comment

Choose a reason for hiding this comment

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

Looks amazing!

@Arkariang Arkariang merged commit f7f29a1 into master Nov 19, 2024
3 checks passed
@Arkariang Arkariang deleted the imartin/MBL-1068 branch November 19, 2024 01:07
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.

4 participants