-
Notifications
You must be signed in to change notification settings - Fork 150
Rewrite executing observable #68
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
Rewrite executing observable #68
Conversation
To guarantee consistent delivery of true / false values
the `executing` events are not guaranteed to be delivered on MainScheduler
|
This PR supersedes #67 |
|
Brilliant! I struggled for a while for a better way to express that executing state, this is really clever. |
|
... and of course after I pushed the first commits I found a better way to do it. Always looking to achieve the zen of Rx :) |
|
Great! I'll try it directly in my codebase to check if it solves my problem (Action sometimes doesn't get re-enabled when a .just() observable is directly returned) and let you know. Hopefully I'll be able to update again to RxSwift 3.1 (right now I'm stuck at 3.0 because of this) |
|
Let me know if you have more issues. If you want to chat directly I'm on the RxSwift Slack (come join https://rxswift.slack.com, we can discuss Action in the Community channel) |
|
Will surely do! I have something I'd like to propose. Meanwhile, I can confirm my problem is resolved with your fix. |
|
Perfect, we'll merge it ASAP (just waiting for someone else to review, that's how the process goes) @ishkawa ? |
Sources/Action/Action.swift
Outdated
| Observable.concat([ | ||
| Observable.just(true), | ||
| $0.ignoreElements().map { _ in true }.catchError { _ in Observable.empty() }, | ||
| Observable.just(false)]) |
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.
Putting these operations here looks nice 👍
I think it might be better to replace ignoreElements().map { _ in true } with flatMap { _ in Observable<Bool>.empty() if it is possible, because the former returns unrelated value true but the latter doesn't.
Additionally, naming $0.ignoreElements().map { _ in true }.catchError { _ in Observable.empty() } may improve readability.
executing = executionObservables
.flatMap { execution -> Observable<Bool> in
let execution = execution
.flatMap { _ in Observable<Bool>.empty() }
.catchError { _ in Observable.empty() }
return Observable.concat([
Observable.just(true),
execution,
Observable.just(false)])
}
.startWith(false)
.shareReplay(1)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.
Excellent ideas, let me push these ones and we're good to go.
|
@fpillet Your PR looks good 👍 I left 2 trivial comments. |
Rewrote the
executingobservable, leading to simpler code.Also Integrated @stefanomondomino's idea of using a
shareReplay(1)to guarantee that late subscribers get the current execution state.