Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

RxJava 2 support #33

Closed
VictorAlbertos opened this issue Sep 8, 2016 · 22 comments
Closed

RxJava 2 support #33

VictorAlbertos opened this issue Sep 8, 2016 · 22 comments

Comments

@VictorAlbertos
Copy link

HI @android10

Are you planning to add support for RxJava2?

@android10
Copy link
Owner

@VictorAlbertos not until we have final version. And of course PRs are always welcome :).

@VictorAlbertos
Copy link
Author

I can give a try ;)

For my own libraries I added support for RxJava2 creating another branch. Just like RxAndroid did it. Does it would be ok for you?

@android10
Copy link
Owner

@VictorAlbertos feel free. Honestly sometimes the final api can change so that is why I like to wait until we have a final version. I guess RxJava 2 is mature enough but it does not hurt to wait a bit longer. :). Thanks for the feedback! 👍

@VictorAlbertos
Copy link
Author

I'm already working on the PR, but please I want you to take a look at this issue from RxJava:

ReactiveX/RxJava#4515

It seems that Subscriber has been reserved only for Flowable, which is like an Observable but allows to handle backpressure with different strategies (Observable does not handle back pressure anymore). Now, in order to subscribe to an Observable you need to use an Observer.

So, my question is how do you want to address this situation?

I suggest to refactor RxLogSubscriber and its companions to RxLogObserver, that way Frodo will keep supporting Observables. And later if you want we can add support for this new type Flowable.

@VictorAlbertos
Copy link
Author

VictorAlbertos commented Sep 8, 2016

@android10 I finished :)

Take a look at this branch from my fork:
https://github.com/VictorAlbertos/frodo/commits/2.x

I'm going to need you to create a branch ("2.x" if you will) in order to perform the PR. Because it's not possible to create a PR to a non-existing branch xD

Some notes:

I hope you find the PR acceptable :)

@VictorAlbertos
Copy link
Author

By the way, all test are passing and as far as I can tell the sample module outputs the right messages in the log.

@VictorAlbertos
Copy link
Author

Hi @android10

Are you planning to review the changes any time sooner? I said so because I need to start using your library with 2.x version.

Thanks :)

@android10
Copy link
Owner

@VictorAlbertos I will review the changes but keep in mind that we need to find a way to make it backward compatible.
Replacing everything with the new stuff is not gonna work since I do not want to maintain 2 libraries for 2 different versions of RxJava.

@android10
Copy link
Owner

@VictorAlbertos other thing. I'm not gonna ship a version of frodo supporting RxJava 2 until we have a final version.

@VictorAlbertos
Copy link
Author

If you don't want to create another branch/module to support 2.x, I think Frodo needs to be refactored in a really heavy way in order to be enterely decoupled from RxJava. And once that refactoring is done, two others modules need to be created, one for 1.x and another one for 2.x, trying to share as much code as possible from that "core" module. But I don't see how this could be achieved with Frodo.

Along the process of porting my own libraries, only Mockery has the kind of structure suitable for this approach. The rest of them required to be another entirely library, hosted in another branch or module.

Nevertheless, if you think I can help you out with this, just let me know :)

@android10
Copy link
Owner

@VictorAlbertos yup, definitely needs a refactor. What you suggested is an option but at the moment I will freeze it until we have a final version of RxJava 2. Other point to not work on that yet, is that the adoption is pretty low.

Of course, any idea is always welcome. Again, having 2 separated frodo versions for 2 separated RxJava versions is a lot of overhead, specially with maintenance. Let's get back to this issue soon.

@VictorAlbertos
Copy link
Author

Oki then ;)

@petrnohejl
Copy link

Hello. Final version of RxJava 2 has been already published. @android10 Are you going to add a support for RxJava 2?

@android10
Copy link
Owner

@petrnohejl that is the idea...but still thinking how the approach will be.
Any feedback is welcome.

Maybe it is better to create another library that supports only RxJava 2 rather than modifying frodo to support the latest one, since they use different types and packages...

A couple of questions need an answer:

1 - Create frodo2?
2 - In case of supporting both versions of RxJava, what the annotations will look like?

I will appreciate anyone shading some light here.

@petrnohejl
Copy link

I think it would make sense to create a completely new library "Frodo 2". RxJava 2 has a different API so supporting both versions of RxJava would require a huge refactoring and changes. Moreover it would have 2 dependencies - both RxJava libs.

@VictorAlbertos
Copy link
Author

Yes, it should be another library. Indeed, almost every library out there which is a reactive extension has adopted this approach. Normally, rather than creating a new repo, they create another branch.

About the annotations, I think the repo which I created months ago could be a good starting point.

@android10
Copy link
Owner

@VictorAlbertos I have something already working with some refactor, so I will also upload it in order to combina efforts and come up with a new version.

@TonyTangAndroid
Copy link

Looking forwarding to Rxjava2 support with "Frodo2"

@android10
Copy link
Owner

@TonyTangAndroid It is a WIP. Will come to light soon.

@android10
Copy link
Owner

Closing since this is going to happen here:
https://github.com/android10/frodo2

@AlexTrotsenko
Copy link

I know, that there is this android10/frodo2#1 issue, but is there any plan to publish frodo2 in mavenCentral/jcenter?

I have not looked deep inside, but as far as I see:

VictorAlbertos commented on Sep 9, 2016
By the way, all test are passing and as far as I can tell the sample module outputs the right messages in the log.

@android10
Copy link
Owner

@AlexTrotsenko that is the idea. I have not had enough time to work on the project but it is on its way. I will grab it again and generate a preview so people can start using it :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

5 participants