Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[fix]: update RxSwift bindings to observe on main thread #294

Merged
merged 1 commit into from
Aug 7, 2024
Merged

[fix]: update RxSwift bindings to observe on main thread #294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion WorkflowRxSwift/Sources/ObservableWorkflow.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import class Workflow.Lifetime

extension Observable: AnyWorkflowConvertible {
public func asAnyWorkflow() -> AnyWorkflow<Void, Element> {
return ObservableWorkflow(observable: self).asAnyWorkflow()
ObservableWorkflow(observable: self).asAnyWorkflow()
}
}

Expand All @@ -41,6 +41,7 @@ struct ObservableWorkflow<Value>: Workflow {
let disposable = observable
.map { AnyWorkflowAction(sendingOutput: $0) }
.subscribe(on: MainScheduler.asyncInstance)
.observe(on: MainScheduler.asyncInstance)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could additionally leave the subscribe logic here... seems like it may not be necessary though if the 'subscription side effects' occur synchronously, since we'll be on the main thread when subscribe(onNext:) is executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edit: decided to 'play it safe' and just leave the existing code too. not sure what may or may not depend on it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote for removing .subscribe(on: since it's just wrong but either way observe(on: is what we want here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah... maybe it is. my only argument against that is if removing it changes something that was async to sync, causing other issues. although, i suppose you can make the case that this also changes behavior in a similar way... we're taking things that used to synchronously emit in whatever context they were in to ones that are (always?) queued async on the main thread.

.subscribe(onNext: { value in
sink.send(value)
})
Expand Down
48 changes: 48 additions & 0 deletions WorkflowRxSwift/Tests/Rx+ReactiveWorkers.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,54 @@ class Rx_ReactiveWorkersTests: XCTestCase {

disposable?.dispose()
}

func test_observes_on_main_queue() {
struct TestWorkflow: Workflow {
enum Action: WorkflowAction {
typealias WorkflowType = TestWorkflow
case complete

func apply(toState state: inout State) -> Output? {
switch self {
case .complete:
return .finished
}
}
}

enum Output {
case finished
}

func render(state: Void, context: RenderContext<Self>) {
Single<Void>.create { observer in
DispatchQueue.global().async {
observer(.success(()))
}
return Disposables.create()
}
.asObservable()
.running(in: context) { _ in
XCTAssert(Thread.isMainThread)
return Action.complete
}
}
}

let host = WorkflowHost(
workflow: TestWorkflow()
)

let expectation = XCTestExpectation()
let disposable = host.output.signal.observeValues { output in
if output == .finished {
expectation.fulfill()
}
}

wait(for: [expectation], timeout: 1.0)
disposable?.dispose()
}
}

struct CombinedWorkflow: Workflow {
Expand Down