-
Notifications
You must be signed in to change notification settings - Fork 500
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
base: main
Are you sure you want to change the base?
RxJava3 support #685
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't change this
There was a problem hiding this comment.
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:_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why add coil?
There was a problem hiding this comment.
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"> |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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>( |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't duplicate this
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. |
Satisfies #352
Adds Support for RxJava3