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

[mjohnson12]

This is a breaking change to Workflow that removes ReactiveSwift from the public interface of Workflow and Workflow UI.
There are 3 changes:

1. In WorkflowHost rendering is now a read only property of the latest rendering.
2. In WorkflowHost renderingPublisher is a Combine publisher that publishes renderings. In searching the register code base there were very few places we were using a Signal/SignalProducer from the rendering property. The plan would be to change those consumers to use renderingPublisher and sink.
3. In WorkflowHost output has been renamed to outputPublisher. It is now a Combine Publisher that can be used to subscribe to Workflow output. Per Andrew's suggestion I added a new protocol WorkflowOutputPubisher that exposes the outputPublisher. The output property is used in a lot of places in register. To fix those places I added an extension on WorkflowOutputPubisher in WorkflowReactiveSwift that re-exposes output as a Signal. All the places that use output will just need to import WorkflowReactiveSwift and they will continue to work.
4. WorkflowHostingController in WorkflowUI now implements WorkflowOutputPublisher. Consumers using output will just need to import WorkflowReactiveSwift to continue to work.

Note: Even though this removes ReactiveSwift as a dependency from the Workflow and WorkflowUI targets the Workflow Package has to continue to have ReactiveSwift as a dependency since it's used by WorkflowReactiveSwift. But because register uses bazel if you import Workflow/WorkflowUI in your module it does not import (directly or transitively) ReactiveSwift.

Checklist

• Unit Tests
• UI Tests
• Snapshot Tests (iOS only)
• I have made corresponding changes to the documentation

[blevasseur-block]

suggestion:

@Published private(set) var rendering: WorkflowType.Rendering`
public var renderingPublisher: AnyPublisher<WorkflowType.Rendering, Never> {
    $rendering.eraseToAnyPublisher()
}

    private let rendering: CurrentValueSubject<Rendering, Never>

    var renderingPublisher: AnyPublisher<Rendering, Never> {
        rendering.eraseToAnyPublisher()
    }

There's likely a few ways we could go about this without creating a custom subject. It's worth noting that @Published is part of Combine and use-able anywhere as well. There's a bit of utility that comes with that.

[mjohnson12]

AnyPublisher isn't backward compatible with what we have now for rendering since you can't ask for the current value.

[mjohnson12]

Oops I read private not private(set). rendering has to be Public to keep clients working.

[blevasseur-block]

Yup, the above with @Published should make it essentially a public read only version of CurrentValueSubject.

I sort of figured with the AnyPublisher but figured I'd post it anyways just in case.

[mjohnson12]

From what I've gathered reading in the swift forums setting the value on CurrentValueSubject is thread safe. I don't about @Published. I'd have to check on that.

[mjohnson12]

Pushed up a branch off of this branch to try using @Published for render. https://github.com/square/workflow-swift/compare/markj/reactive_swift_removal...markj/published_rendering?expand=1

[jamieQ]

definitely prefer hiding the implementation details of the 'observable' thing that is exposed publicly. what does the property wrapper buy us exactly (vs, say, just exposing rendering as a read-only public property, and managing the publicly-observable value separately)? do we want to lean into that as the binding mechanism, or implement the logic manually so we could change the guts if needed.

also, doesn't @Published have some specific behaviors about when observers are notified vs when the stored value is changed (like if you read the stored value in a sink callback it won't yet be updated or something)? is that something that matters?

[mjohnson12]

Originally I was trying to maintain the same behavior we had before where there was a rendering property you could ask the value of. This led to the issue that CurrentValueSubject would expose setting the value which we did not want to do. That's why I came up with ReadOnlyCurrentValueSubject.
With that api getting the rendering value would be identical to now so none of that code would need to change only where you subscribe to changes of the rendering which is actually not very many places.

The @Published scenario basically had the same behavior as a CurrentValueSubject but you can easily make the setter private.

I agree @Published isn't buying us much over a read only property for rendering and a renderingPublisher AnyPublisher property that are implemented internally by something like a CurrentValueSubject. I'll explore that scenario.

[mjohnson12]

Since we have consensus that we don't like the current ReadOnlySubject approach I will push up changes on this branch.

[mjohnson12]

Updated the implementation to have a read only rendering and a renderingPublisher property.

[blevasseur-block]

question: Is this just a duplicated version of

import Combine
import Foundation
import ReactiveSwift

extension Publisher {
    /// Create a ReactiveSignal
    public func toSignal() -> Signal<Output, Failure> {
        Signal.unserialized { observer, lifetime in
            let cancellable = self.sink(
                receiveCompletion: { completion in
                    switch completion {
                    case .finished:
                        observer.sendCompleted()

                    case .failure(let error):
                        observer.send(error: error)
                    }
                },
                receiveValue: { value in
                    observer.send(value: value)
                }
            )
            lifetime.observeEnded {
                cancellable.cancel()
            }
        }
    }
}

In our ReactiveSwiftCombineBridging library?

Do we have a way of collapsing these if so - or is this more of a temporary transitional item needed?

[mjohnson12]

We can't reference anything in register in Workflow. I don't think it would be worth adding a new public library just to share that.

[blevasseur-block]

No it wouldn't, but we also need to be careful about fragmented/duplicated public utilities. If this was internal and guaranteed to be 100% unique, I might be more ok with it. Both are public and almost 1-1 duplications meaning fragmentation and potential collisions at some point.

Is this just temporary though as ReactiveSwift is being removed from Workflow or is this a more or less long term duplicated utility?

For the consuming app, I do imagine outputPublisher.toSignal() would work all the same leaving us with outputPublisher.toSignal() and/or outputPublisher.output essentially producing the same thing. I would imagine ouptuPublisher.output could be internal to Workflow and outputPublisher.toSignal() would be the application utility.

[mjohnson12]

We have a lot of usage of the current output signal in the codebase so this keeps all those working by just adding an import statement.
I guess it's as temporary as we have consumers wanting a ReactiveSwift Signal. After we have the combine publisher in the consumers can definitely switch to using that and we could remove this extension.
The overall goal of this change is remove ReactiveSwift without breaking or having to change all the workflows in register.

[blevasseur-block]

Cool - totally get it given it's a migration path strategy.

[jamieQ]

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

[mjohnson12]

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.

[jamieQ]

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).

[mjohnson12]

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.

[jamieQ]

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...

[mjohnson12]

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.

[jamieQ]

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.

[mjohnson12]

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.

[jamieQ]

let's plan on testing this fairly thoroughly. i'm never certain what the exact lifetime semantics are for the ReactiveSwift stuff personally...

[mjohnson12]

Agreed. This has been in use in the ReactiveSwift bridging module for a while.

[jamieQ]

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).

[jamieQ]

let's also include in the commit message something that clearly indicates this is a breaking change to the public API, a la the guidelines specified via 'conventional commits'

[mjohnson12]

let's also include in the commit message something that clearly indicates this is a breaking change to the public API, a la the guidelines specified via 'conventional commits'

I've updated the commit message