Skip to content

RF-9688 - [refactor]: Remove ReactiveSwift from Workflow public interface #360

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+279 −235
Open

RF-9688 - [refactor]: Remove ReactiveSwift from Workflow public interface #360

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
2 changes: 1 addition & 1 deletion Package.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ let package = Package(

.target(
name: "Workflow",
dependencies: ["ReactiveSwift"],
path: "Workflow/Sources"
),

.target(
name: "WorkflowTesting",
dependencies: [
Expand Down
5 changes: 4 additions & 1 deletion Samples/Project.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,10 @@ let project = Project(
.unitTest(
for: "Workflow",
sources: "../Workflow/Tests/**",
dependencies: [.external(name: "Workflow")]
dependencies: [
.external(name: "ReactiveSwift"),
.external(name: "Workflow"),
]
),
.unitTest(
for: "WorkflowTesting",
Expand Down
12 changes: 6 additions & 6 deletions Samples/Tutorial/Frameworks/Tutorial5Complete/Tests/RootWorkflowTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class RootWorkflowTests: XCTestCase {

// First rendering is just the welcome screen. Update the name.
do {
let backStack = workflowHost.rendering.value
let backStack = workflowHost.rendering
XCTAssertEqual(1, backStack.items.count)

guard let welcomeScreen = backStack.items[0].screen.wrappedScreen as? WelcomeScreen else {
Expand All @@ -100,7 +100,7 @@ class RootWorkflowTests: XCTestCase {

// Log in and go to the todo list.
do {
let backStack = workflowHost.rendering.value
let backStack = workflowHost.rendering
XCTAssertEqual(1, backStack.items.count)

guard let welcomeScreen = backStack.items[0].screen.wrappedScreen as? WelcomeScreen else {
Expand All @@ -113,7 +113,7 @@ class RootWorkflowTests: XCTestCase {

// Expect the todo list to be rendered. Edit the first todo.
do {
let backStack = workflowHost.rendering.value
let backStack = workflowHost.rendering
XCTAssertEqual(2, backStack.items.count)

guard let _ = backStack.items[0].screen.wrappedScreen as? WelcomeScreen else {
Expand All @@ -134,7 +134,7 @@ class RootWorkflowTests: XCTestCase {

// Selected a todo to edit. Expect the todo edit screen.
do {
let backStack = workflowHost.rendering.value
let backStack = workflowHost.rendering
XCTAssertEqual(3, backStack.items.count)

guard let _ = backStack.items[0].screen.wrappedScreen as? WelcomeScreen else {
Expand All @@ -158,7 +158,7 @@ class RootWorkflowTests: XCTestCase {

// Save the selected todo.
do {
let backStack = workflowHost.rendering.value
let backStack = workflowHost.rendering
XCTAssertEqual(3, backStack.items.count)

guard let _ = backStack.items[0].screen.wrappedScreen as? WelcomeScreen else {
Expand Down Expand Up @@ -204,7 +204,7 @@ class RootWorkflowTests: XCTestCase {

// Expect the todo list. Validate the title was updated.
do {
let backStack = workflowHost.rendering.value
let backStack = workflowHost.rendering
XCTAssertEqual(2, backStack.items.count)

guard let _ = backStack.items[0].screen.wrappedScreen as? WelcomeScreen else {
Expand Down
38 changes: 25 additions & 13 deletions Workflow/Sources/WorkflowHost.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import ReactiveSwift
import Combine

/// Defines a type that receives debug information about a running workflow hierarchy.
public protocol WorkflowDebugger {
Expand All @@ -30,18 +30,30 @@ public protocol WorkflowDebugger {
func didUpdate(snapshot: WorkflowHierarchyDebugSnapshot, updateInfo: WorkflowUpdateDebugInfo)
}

/// Manages an active workflow hierarchy.
public final class WorkflowHost<WorkflowType: Workflow> {
private let (outputEvent, outputEventObserver) = Signal<WorkflowType.Output, Never>.pipe()
public protocol WorkflowOutputPublisher {
associatedtype Output

var outputPublisher: AnyPublisher<Output, Never> { get }
}
Comment on lines +33 to +37
Copy link
Contributor

Choose a reason for hiding this comment

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

is this protocol necessary? what's the benefit over just extending the concrete type directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It allows us to add the output signal on both WorkflowHost and WorkflowHostingController by just extending the protocol in WorkflowReactiveSwift. This was a suggestion in Andrew's feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah gotcha (sorry i missed the existing convo). so the tradeoff here is adding a 'public' protocol to which we really only want 2 specific things to conform so that we don't need to make Workflow or WorkflowUI depend on ReactiveSwift – is that right? mostly out of curiosity – is it possible to define the protocol with package visibility? that seems like it might be a slightly more accurate definition (assuming it works and integrates successfully into the monorepo).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before I had extensions on WorkflowHost and WorkflowHostingController which required a new module just to put the extension on WorkflowHostingController since I could not put it in WorkflowReactiveSwift since it does not know about WorkflowUI. Andrew's idea was use a protocol and drop the extension in WorkflowReactiveSwift so we don't need to have an additional module and the reactive swift stuff stays in WorkflowReactiveSwift.
I can look into the package visibility to see if that is possible.


/// Manages an active workflow hierarchy.
public final class WorkflowHost<WorkflowType: Workflow>: WorkflowOutputPublisher {
// @testable
let rootNode: WorkflowNode<WorkflowType>

private let mutableRendering: MutableProperty<WorkflowType.Rendering>
private let renderingSubject: CurrentValueSubject<WorkflowType.Rendering, Never>
Copy link
Contributor

Choose a reason for hiding this comment

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

slightly inclined to split the storage for the property vs the observer, since that will give us the most control over when the 'outside world' sees the update. e.g.

var rendering: Rendering
let renderingSubject = PassThroughSubject<Rendering, Never>()

any thoughts on that? i guess one thing that would be different is that the old way (and presumably using a CVS) would have some baked-in synchronization mechanism over the underlying value. in theory this stuff should be main-thread-only though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We currently are using a ReactiveSwift MutableProperty which does have an internal lock. Yes CurrentValueSubject does have some baked in serialization mechanism albeit without Apple documentation on what that is. Since we can't enforce someone reading the rendering value on the main thread I'm inclined to use the CurrentValueSubject since it's about as close as we can get as a Combine version of what we have now.
If we split the storage for the property out we could always lock around getting/setting it so I'm not against doing that but I think CurrentValueSubject should work for how we are using it.

private let outputSubject = PassthroughSubject<WorkflowType.Output, Never>()

/// Represents the `Rendering` produced by the root workflow in the hierarchy. New `Rendering` values are produced
/// as state transitions occur within the hierarchy.
public let rendering: Property<WorkflowType.Rendering>
public var rendering: WorkflowType.Rendering {
renderingSubject.value
}

/// A Publisher containing rendering events produced by the root workflow in the hierarchy.
public var renderingPublisher: AnyPublisher<WorkflowType.Rendering, Never> {
Copy link
Contributor

Choose a reason for hiding this comment

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

any thoughts on using some Publisher<Rendering, Never> vs AnyPublisher here (and in general where we return Publishers)? in theory the former lets us not have to do the erasure (though then client code could in theory cast it to recover the concrete type in some cases... unless we still erased it before returning). if we don't return AnyPublisher then i guess any client code that wishes to store the value as a concrete type would have the burden of doing that...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer AnyPublisher as it hides the type better and it's the standard Combine way of returning a type erased publisher. I know any Publisher doesn't work as you can't call most of operators on it. some Publisher does not seem to have that issue though.

renderingSubject.eraseToAnyPublisher()
}

/// Context object to pass down to descendant nodes in the tree.
let context: HostContext
Expand Down Expand Up @@ -78,8 +90,8 @@ public final class WorkflowHost<WorkflowType: Workflow> {
parentSession: nil
)

self.mutableRendering = MutableProperty(rootNode.render())
self.rendering = Property(mutableRendering)
self.renderingSubject = CurrentValueSubject(rootNode.render())

rootNode.enableEvents()

debugger?.didEnterInitialState(snapshot: rootNode.makeDebugSnapshot())
Expand Down Expand Up @@ -110,12 +122,12 @@ public final class WorkflowHost<WorkflowType: Workflow> {
private func handle(output: WorkflowNode<WorkflowType>.Output) {
let shouldRender = !shouldSkipRenderForOutput(output)
if shouldRender {
mutableRendering.value = rootNode.render()
renderingSubject.send(rootNode.render())
}

// Always emit an output, regardless of whether a render occurs
if let outputEvent = output.outputEvent {
outputEventObserver.send(value: outputEvent)
outputSubject.send(outputEvent)
}

debugger?.didUpdate(
Expand All @@ -129,9 +141,9 @@ public final class WorkflowHost<WorkflowType: Workflow> {
}
}

/// A signal containing output events emitted by the root workflow in the hierarchy.
public var output: Signal<WorkflowType.Output, Never> {
outputEvent
/// A publisher containing output events emitted by the root workflow in the hierarchy.
public var outputPublisher: AnyPublisher<WorkflowType.Output, Never> {
outputSubject.eraseToAnyPublisher()
}
}

Expand Down
8 changes: 5 additions & 3 deletions Workflow/Tests/AnyWorkflowTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import ReactiveSwift
import Combine
import XCTest
@testable import Workflow

Expand All @@ -40,19 +40,21 @@ public class AnyWorkflowTests: XCTestCase {
let host = WorkflowHost(workflow: OnOutputWorkflow())

let renderingExpectation = expectation(description: "Waiting for rendering")
host.rendering.producer.startWithValues { rendering in
let cancellable = host.renderingPublisher.sink { rendering in
if rendering {
renderingExpectation.fulfill()
}
}

let outputExpectation = expectation(description: "Waiting for output")
host.output.observeValues { output in
let outputCancellable = host.outputPublisher.sink { output in
if output {
outputExpectation.fulfill()
}
}
wait(for: [renderingExpectation, outputExpectation], timeout: 1)
cancellable.cancel()
outputCancellable.cancel()
}

func testOnlyWrapsOnce() {
Expand Down
Loading
Loading