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

RxJava3 support #685

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

JagadeeshBabuDamarasingu
Copy link

@JagadeeshBabuDamarasingu JagadeeshBabuDamarasingu commented Jun 8, 2023

Satisfies #352
Adds Support for RxJava3

Copy link
Contributor

@elihart elihart left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Can you describe your method? Did you simply copy every file over exactly from the rx2 module and then update the references for rx3?

Some of these files have no relationship to rxjava and should not be duplicated - can you create a common rxjava module to share those?

It is a shame to duplicate most of this code at all, like the subscribe functions, but the functions need to return a Disposable that is specific to each Rx version. Ideally we would be able to provide a base class that contains the bulk of the code, and each rx version would just map the result disposable to the right version. If you can think of a good way to do that it would be great

@@ -0,0 +1,4 @@
POM_NAME=Mavericks
POM_ARTIFACT_ID=mavericks-rxjava2
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong artifact id

@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="UTF-8"?>
<project version="4">
Copy link
Contributor

Choose a reason for hiding this comment

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

don't add this file. you can git ignore it

Choose a reason for hiding this comment

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

sure

@@ -1,11 +1,6 @@
<component name="ProjectCodeStyleConfiguration">
<code_scheme name="Project" version="173">
<JetCodeStyleSettings>
<option name="PACKAGES_TO_USE_STAR_IMPORTS">
Copy link
Contributor

Choose a reason for hiding this comment

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

don't change this

Choose a reason for hiding this comment

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

I have noticed that there might have been some automatic changes made by Android Studio itself. I will investigate this further to identify and verify any modifications that have occurred.

retrofitRxJava = "com.squareup.retrofit2:adapter-rxjava2:_"
retrofitRxJava2 = "com.squareup.retrofit2:adapter-rxjava2:_"
retrofitRxJava3 = "com.squareup.retrofit2:adapter-rxjava3:_"
coil = "io.coil-kt:coil:_"
Copy link
Contributor

Choose a reason for hiding this comment

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

why add coil?

Choose a reason for hiding this comment

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

this one I added as part of rxjava demo, which is still in progress

@@ -0,0 +1,5 @@
<manifest package="com.airbnb.mvrx.rxjava2">
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think this is needed. declare the package name in the build.gradle file via the namespace property

@@ -0,0 +1,11 @@
# Mavericks + RxJava3

[//]: # (TODO refine the documentation)
Copy link
Contributor

Choose a reason for hiding this comment

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

are you planning to address this?

Copy link
Author

Choose a reason for hiding this comment

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

I am currently in the process of building an rxjava demo app, and as part of that, I had intended to create comprehensive documentation that includes various examples taken directly from the demo itself, However, I apologize for not including it at this time. I'll also add a guide on migration from rxjava2 to rxjava3.

*
* @see Mavericks
*/
object MvRx {
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be duplicated

*
* @see MavericksState
*/
interface MvRxState : MavericksState
Copy link
Contributor

Choose a reason for hiding this comment

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

this shouldn't be duplicated

/**
* Base ViewModel implementation that all other ViewModels should extend.
*/
abstract class BaseMvRxViewModel<S : MavericksState>(
Copy link
Contributor

Choose a reason for hiding this comment

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

some of this code doesn't need to be duplicated. each rx version can share a InternalMvRxViewModel super class which can be shared from a new module

message = "MvRx has been replaced with Mavericks",
replaceWith = ReplaceWith("MavericksViewModelFactory<VM, S>")
)
interface MvRxViewModelFactory<VM : MavericksViewModel<S>, S : MavericksState> : MavericksViewModelFactory<VM, S>
Copy link
Contributor

Choose a reason for hiding this comment

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

don't duplicate this

@JagadeeshBabuDamarasingu
Copy link
Author

JagadeeshBabuDamarasingu commented Jun 17, 2023

Hi @elihart, when I initially developed this PR, my primary objective was to ensure compatibility with the existing rxJava2 API. I wanted users to be able to seamlessly transition from rxJava2 to rxJava3 by simply refactoring their codebase. To achieve this, I made sure not to disrupt the rxJava2 module, which is why you might notice a few duplicate classes.

Thank you for your feedback, I will make improvements to this MR accordingly.

I would like to express that this PR marks my first open source contribution, and I sincerely apologize for any mistakes I may have made along the way. As a newcomer to this realm, I am constantly learning and striving to improve my skills and understanding of Open Source practices.

I greatly appreciate your patience.

@JagadeeshBabuDamarasingu JagadeeshBabuDamarasingu marked this pull request as draft June 17, 2023 05:58
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