RxSwiftCommunity / RxRealm

RxSwift extension for RealmSwift's types
MIT License
1.15k stars 265 forks source link

Move to realm 5 #143

Closed M0rtyMerr closed 4 years ago

M0rtyMerr commented 4 years ago

Resolve #139

muukii commented 4 years ago

@rynecheow Hi, I put the suggestion that fixes the indent issue from GitHub review. We can ship the version that supports Realm 5, if we could commit this fixed from here.

M0rtyMerr commented 4 years ago

Hey @muukii @catelina777 @Loupehope @rynecheow thank you so much for fast answers! And sorry for my long fixes. There was a threading issue with tests and I didn't have time to dig into it. Now I fixed indents, found the issue and clean up tests. Could u please approve it again? Thanks.

BTW, really important question: if synchronousStart: true, should we dispatch the first item on provided notifications queue? Or it should be dispatched on the current thread?

muukii commented 4 years ago

@MortyMerr

BTW, really important question: if synchronousStart: true, should we dispatch the first item on provided notifications queue? Or it should be dispatched on the current thread?

It seems the current implementation would be good, but there are some chances that causes an exception for using incorrect thread?

I think, synchronousStart means we can get the current item immediately that dispatches on the current stack. So If it dispatches on notification queue, in some case we can't get the first item synchronously.

If the parameter name was emitsCurrentValue, we could understand it emits the value on the specified queue.

muukii commented 4 years ago

but there are some chances that cause an exception for using incorrect thread?

I thought the exception might not happen. Because the first one will be for the thread that started observation and the received item would be for the thread that the queue uses.

However, Since this behavior, the developers take mistakes using thread in receiving item closure.

Observable...(queue: .somethingQueue) // somethingQueue uses Thread 1 as an example
  .subscribe(onNext: { element in
    // first time - element for Thread 0

    // second time - element for Thread 1
 })

So, My suggestion, when set the syncronousStart flag, make the specifying queue disabled. Let me know if I wrong understanding. Thanks.

icanzilb commented 4 years ago

Here's my 2c and a little of background info to help you decide how to solve this issue.

synchronousStart was added because realm notifications are by default asynchronous and so sometimes when you bound a table view or some labels on screen you would have a little flash when the view controller appears with empty controls for a moment and then in a split second the first realm notification would kick in and the controls will update with the initial values.

I'm not so sure if in reality there is another use case outside of binding UI controls but maybe there is ...

In any case I personally would expect with or without synchronous start that the API will respect the queue parameter and emit on that queue.

M0rtyMerr commented 4 years ago

Yeah, I also can imagine only this case for synchronousStart. I don't think it should emit initial value on provided queue, because it will bring this "little flash" back