pushtorefresh / storio

Reactive API for SQLiteDatabase and ContentResolver.
Apache License 2.0
2.55k stars 182 forks source link

RxJava 2 support #685

Closed geralt-encore closed 6 years ago

geralt-encore commented 8 years ago

Any thoughts about how to introduce it in a better manner without dropping RxJava 1 support? Also object methods have to be reworked since they are emitting null if there is no requested object.

geralt-encore commented 8 years ago

Totally forgot about Maybe - it's perfect for that case.

artem-zinnatullin commented 8 years ago

Yeah, there is some work required to do RxJava v2 support.

TL;TR plan:

  1. Wait for final release.
  2. Add basic support for Flowable because it supports backpressure which is required for IO operations.
  3. Add support for Single/Completable.
  4. Add/Think about support for Maybe.

RxJava 2 can work with RxJava 1 in same classpath, so I don't see problems with that except naming of the methods (I think we'll just add "2" to it).

On 2 Oct 2016 10:30 am, "Ilya Zorin" notifications@github.com wrote:

Totally forgot about Maybe - it's perfect for that case.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pushtorefresh/storio/issues/685#issuecomment-250974064, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7B3FCuyaL5AWs-sb4gOL3psnCF8K3Pks5qv8ABgaJpZM4KHoyr .

geralt-encore commented 8 years ago

Not sure if I understand you correctly, so you want to have both RxJava and RxJava 2 in the same library? It sounds not really good having both of these dependencies when you need only one of them in your project. Regarding supporting Maybe - it's a perfect fit for object methods since null is prohibited in RxJava 2 and was kinda hacky in a first place.

nikitin-da commented 8 years ago

StorIO has just provided rxjava dependency - you can use rx only when you already have this dependency in your project

geralt-encore commented 8 years ago

Thanks for clarification @nikitin-da! Anyway, I will be happy to help with this task.

artem-zinnatullin commented 8 years ago

Great, let's wait for release then :)

On 3 Oct 2016 5:00 am, "Ilya Zorin" notifications@github.com wrote:

Thanks for clarification @nikitin-da https://github.com/nikitin-da! Anyway, I will be happy to help with this task.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/pushtorefresh/storio/issues/685#issuecomment-251058128, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7B3LHi2oPNnNQosC_7eDxso5pEMCCMks5qwMQlgaJpZM4KHoyr .

geralt-encore commented 8 years ago

https://github.com/ReactiveX/RxJava/releases/tag/v2.0.0

artem-zinnatullin commented 8 years ago

Yup, still waiting for more stable build though, but we can start migration.

skrugly commented 8 years ago

Good reason to lay the foundation for StorIO v2, huh?

artem-zinnatullin commented 8 years ago

Hah, probably, but we're using semantic versioning so StorIO will become v2 as soon as we break API, adding RxJava v2 is planned as non-breaking change at the moment, so I guess StorIO will still be v1.

BUT: I'd like to keep both RxJava v1 and v2 only for the period of migration (in our internal and yours, users's) apps from RxJava v1 to v2 and when we'll see that RxJava v1 is not actively used anymore -> we'll drop support for RxJava v1 and that'll in fact require us to upgrade major version of StorIO!

@Adambl4 @nikitin-da @geralt-encore sounds good to you?

nikitin-da commented 8 years ago

I agree, both versions will be required for a certain period of time

geralt-encore commented 8 years ago

Sounds good 👍

artem-zinnatullin commented 8 years ago

Though I admit that with https://github.com/akarnokd/RxJava2Interop from @akarnokd we can leave only RxJava v2 support and force users to use interop library to convert types between v1 and v2, but without extension methods support in plain Java it'll be non-fluent and add a lot of boilerplate to the code.

geralt-encore commented 8 years ago

What about using interop library internally? Completely migrate to v2 and provide methods for v1 interop via the library. Then just wipe it after some time.

skrugly commented 8 years ago

RxJava1 has 5k methods, RxJava2 another 9k. Forcing users to have v2 in classpath if they don't need it is a crime against humanity.

nikitin-da commented 8 years ago

I don't like the idea to keep RxJava v2 methods only. On the one hand it will speed up migration, but on the other hand it will lead to boilerplate on java6 and step towards to multidex 😱 on this period. 👍 for asRxObservable and asRxObservable2 Sure it will be more difficult for us, but much more democratic for users

artem-zinnatullin commented 8 years ago

@geralt-encore

What about using interop library internally? Completely migrate to v2 and provide methods for v1 interop via the library. Then just wipe it after some time.

:+1:

PR welcome!

@Adambl4 nobody will force you download RxJava v2, we'll add it same way we added RxJava v1 support, it's optional dependency!

outofdate commented 7 years ago

Any updates on RxJava2 support?

artem-zinnatullin commented 7 years ago

Please use adapter library from @akarnokd for now.

On Wed, Dec 14, 2016, 16:09 outofdate notifications@github.com wrote:

Any updates on RxJava2 support?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/pushtorefresh/storio/issues/685#issuecomment-267029569, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7B3KftfTT5dtjyUF6Srpk04XLWnztOks5rH-qAgaJpZM4KHoyr .

mayuroks commented 7 years ago

Bump!, any progress on RxJava v2 support?

artem-zinnatullin commented 7 years ago

Not yet, starting integrating it into work projects, then I'll look into support for StorIO, PRs welcome!

geralt-encore commented 7 years ago

I guess I can start working on what we agreed in https://github.com/pushtorefresh/storio/issues/767. There are still a couple of open questions:

Everything else looks pretty clear to me. Also, I want to add Maybe to both object and objects method, but it should be addressed after the migration in a separate PR, I guess.

@artem-zinnatullin @nikitin-da what do you guys think?

geralt-encore commented 7 years ago

@artem-zinnatullin so I have been looking into RxJava 2 migration lately and I was thinking since 2.0 is going to introduce API breaking changes because of interceptors we can introduce more of them to make migration easier? I was thinking about:

I understand that it is not the best way for migration I honestly don't see any way to migrate while maintaining backwards compatibility since we haven't done anything about it for quite some time. And I think that it is better to make migration happen like that than don't make it at all.

btw I need some help to implement OnSubscribeExecuteAsBlocking for RxJava2. The current implementation is based on some internal RxJava APIs which I am not aware of. OnSubscribeExecuteAsBlockingSingle and OnSubscribeExecuteAsBlockingCompletable were pretty straightforward to implement, but I have no idea how to correctly rewrite OnSubscribeExecuteAsBlocking.

artem-zinnatullin commented 7 years ago

Your points are fare, lemme start migration so you and @nikitin-da could catch up and continue the work?

On Sun, May 21, 2017, 15:49 Ilya Zorin notifications@github.com wrote:

@artem-zinnatullin https://github.com/artem-zinnatullin so I have been looking into RxJava 2 migration lately and I was thinking since 2.0 is going to introduce API breaking changes because of interceptors we can introduce more of them to make migration easier? I was thinking about:

  • Migrating completely to RxJava 2
  • Changing Observable to Flowable everywhere and method names as well to reflect it
  • Removing object method since it doesn't fit into RxJava 2 non-null model

I understand that it is not the best way for migration I honestly don't see any way to migrate while maintaining backwards compatibility since we haven't done anything about it for quite some time. And I think that it is better to make migration happen like that than don't make it at all.

btw I need some help to implement OnSubscribeExecuteAsBlocking for RxJava2. The current implementation is based on some internal RxJava APIs which I am not aware of. OnSubscribeExecuteAsBlockingSingle and OnSubscribeExecuteAsBlockingCompletable were pretty straightforward to implement, but I have no idea how to correctly rewrite OnSubscribeExecuteAsBlocking.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/pushtorefresh/storio/issues/685#issuecomment-302934766, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7B3LcxMkM7AW3fN_V2mcIbkxw0XV4xks5r8DLpgaJpZM4KHoyr .

geralt-encore commented 7 years ago

Yes! It would be really nice if you made sure that the foundation works correctly since you are the guy who originally wrote it and you know rx way better than me =) I can take care of all the more high-level stuff, tests and etc. And @nikitin-da of course if he is interested in it and has enough time =)

artem-zinnatullin commented 7 years ago

So I've started migration yesterday, it'll definitely take a lot of evenings after work to complete. So what I'll do is — convert all StorIO main/src to RxJava 2 and use Interop library in tests so tests won't be modified that much. Then I'll create a PR so you could review it and merge, and then we'll be able to convert tests to RxJava 2 little by little.

Does that sound good?

nikitin-da commented 7 years ago

Sound nice! It will allow to do it iteratively and together 👍

geralt-encore commented 7 years ago

Sounds good! Let me know if can help you. I would be happy to help as much as possible.

nikiJava commented 7 years ago

Hello, guys! Are there any news about rxjava 2 integration in library?

artem-zinnatullin commented 7 years ago

All work is public and can be found in initial-2.x branch, I've migrated almost all StorIO codebase to RxJava 2.x

I need to make couple tests however, regarding need in Flowable and then a week of evenings or so to finish the work. If someone wants to continue my work, I'm more than happy to make a PR to 2.x branch so somebody could work on top of it

geralt-encore commented 7 years ago

I am ready to help. I was stuck on some low-level parts when I was trying to migrate it myself. I assume that you already have dealt with them so I am ready to help with whatever needed.

ZakTaccardi commented 7 years ago

For nullabillity in RxJava2, we need a way to emit a null field for when a record is not found while keeping a hot stream, so Maybe does not suffice.

Has this been considered? Maybe some type of converter/adapter? Or does .onErrorReturn() suffice to catch the null pointer exception and return our own Optional implemention (does this style of error handling have side effects?)

geralt-encore commented 7 years ago

I was thinking about the same thing recently. Converter/adapter won't work since they apply after the emission. Emitting nulls and catching them with onErrorReturn() seems like an anti-pattern to be honest. The only solution that I see is to bundle some Optional implementation with StorIO and use it for that case.

ZakTaccardi commented 7 years ago

Or does .onErrorReturn() suffice to catch the null pointer exception and return our own Optional implemention (does this style of error handling have side effects?)

This definitely does not work, because it terminates the stream.

The only solution that I see is to bundle some Optional implementation with StorIO and use it for that case.

This seems like the best solution. Just make sure that it's not called Optional and something more specific to this lib to not confuse it with our own Optional implantations.

Rainer-Lang commented 7 years ago

Will you add a migration guide?

nikitin-da commented 7 years ago

@Rainer-Lang Right now it is only available via https://github.com/akarnokd/RxJava2Interop When we will finish build in rxjava2 support of cause we should add some readme about migration

Rainer-Lang commented 7 years ago

@nikitin-da Thanks for your feedback.

geralt-encore commented 7 years ago

@artem-zinnatullin can I continue migration based on your 2.x branch or is it still early and some crucial things are missing?

nikitin-da commented 7 years ago

@geralt-encore I was stuck on this issue https://github.com/ReactiveX/RxJava/issues/5234 but it seems some workaround was released https://github.com/ReactiveX/RxJava/pull/5590

geralt-encore commented 7 years ago

ah, you are working on it! Didn't know about that. So let me know if you need any help then.

Rainer-Lang commented 7 years ago

@nikitin-da @geralt-encore Thanks for the update. BTW, I don't want to use the Interop-lib for something so essential like the database.

pavlospt commented 7 years ago

@geralt-encore Is there any ETA on RxJava2 support? We are heavily dependent on StorIO for some basic functionality and we would like to see if we need to investigate for other solutions :) !!

nikitin-da commented 7 years ago

@pavlospt sorry for delay. Migration is in progress in branch https://github.com/pushtorefresh/storio/commits/az/initial-2.x There are couple of unresolved issues for now: optional wrapper and some problems with class loader for use cases without rx dependency

nikitin-da commented 7 years ago

I hope we will resolve it in 2 weeks

pavlospt commented 7 years ago

@nikitin-da No need to say sorry :) Thank you for your work on the project and for the insights :) !!