Implement keyed side effects in place of / as underpinnings to Worker (Grand Unified Workflow Theory)

[rjrjr]

There are lot of nagging questions and bugs around Workers, Workflows and their consistency between each other and the two platforms.

• Should they have identity / key properties? (Give Worker a key property instead of automatically using type + equivalence #961)
• Should Swift have SignalProducer.asWorker? (Add SignalProducer.asWorker to Swift #581)
• Should we fix the bugs in Context.subscribe, or drop that function entirely? ([iOS] If 2 signals fire in the same run loop, the second is ignored #744)
• How do we resolve their run / render APIs? (#1019, #996, Swift: change awaitResult(worker …) to running(worker …) #544)
• How do we resolve their lifecycles? (Reconcile redundant worker behavior between Swift and Kotlin #994)
• Does Swift need StatelessWorkflow? (Add stateless workflow convenience extensions to Swift #236)

After a marathon discussion this afternoon, @zach-klippenstein @bencochran @dhavalshreyas and I are pretty convinced that most of these issues can be resolved, or at least become a lot simpler to address, if we drop Worker as a separate concept. We replace it with one new Workflow feature that allows them to do the same kind of work, RenderContext.onFirstRender.

It should be possible to retrofit existing Workers as convenience implementations built on this call, allowing this to be a transparent change. And this seems like no more work than addressing all of the issues listed above in isolation — probably a lot less when you take into account the related design discussions that never seem to resolve.

I'm not going to try to capture all of the details, just enough that we don't forget the conversation. Counting on @zach-klippenstein and @bencochran to iterate here.

// Current:
override fun render(context: RenderContext<State, Output>): R {
  context.runningWorker(stream.asWorker(), “key”) {
    setOutput(it)
  }
}

// Proposed:
override fun render(
  props: RequestParameters,
  state: State,
  context: RenderContext<State, Output>
) {
  // key is required! No conveniences on this API, we use it to build simpler ones.
  context.onFirstRender(key = props + state) { onDispose ->
    val subscription = stream.subscribe {
      context.actionSink.send { action { … } }
    }
    onDispose { subscription.unsubscribe() }
  }
  context.onFirstRender(“times”) {}
}

It is not expected that anyone would call that method inline. Rather, it's the primitive we use to build various operators.

Worker could be reproduced something like this:

// <Props, State, Output, Rendering>
abstract class Worker<T> : Workflow<Worker<T>, T, Unit> {
  abstract fun run(): Flow<T>
  abstract fun isEquivalent(other: Worker<*>): Boolean

  final override asStatefulWorkflow() = WWF(this)
}

class WWF<T>(
  private val worker: Worker<T>
) : StatefulWorkflow<Worker<T>, Long, T, Unit>() {

  final override fun onPropsChanged(
    old: Worker<T>,
    new: Worker<T>,
    state: Long
  ): Long {
    if (!old.isEquivalent(new)) return state+1
    return state
  }

  final override fun render() {
    context.onFirstRender(state)
  }
}

fun <T, S,O> RenderContext<S, O>.runningWorker(
  worker: Worker<T>,
  key: String,
  mapOutput: (T) -> Action<S, O>
) {
  render(worker, worker, key, mapOutput)
}

Worker in Swift:

extension Worker: Workflow where State == UUID, Rendering == Void {

  func makeInitalState() -> UUID {
    return UUID()
  }

  func workflowDidChange(from previousWorkflow: Self, state: inout State) {
    if !isEquivalent(to: previousWorkflow) {
      state = UUID()
    }
  }

  func render(state: State, context: RenderContext<Self>) -> Void {
    context.onFirstRender(state) { onDispose in
      self.run().subscribeValue {
        context.actionSink.send($0)
      }
    }
    return ()
  }
}

extension RenderContext {
  func running<W, O>(
worker: W,
key: String = “”,
outputMap: @escaping (W.Output) -> O
  ) where
    W: AnyWorkflowConvertable,
    W.Rendering == Void,
    O: WorkflowAction,
    O.WorkflowType == WorkflowType
  {
    _ = render(workflow: worker, outputMap: outputMap)
  }
}


extension AnyWorkflowConvertable where Rendering == Void {
  func running<Parent>(
     with: RenderContext<Parent>,
key: String = “”,
outputMap: @escaping (W.Output) -> O
  ) where
    Parent: Workflow,
    O: WorkflowAction,
    O.WorkflowType == Parent
  {
    _ = rendered(with: context, outputMap: outputMap)
  }
}

Could provide a convenience for a long lived RPC request something like this:

fun <Req, Res> callWorkflow(perform: (Req) -> Res) : Workflow<Req, Res, Unit> = object : Workflow<…> {
  fun onPropsChanged(old: Req, new: Req, …) …
  fun render(req: Req) {
    context.onFirstRender(req) { … }
  }
}

val myCall: Workflow<Request, Response, Unit> = callWorkflow<Request, Response>({ request ->
  val response = service.call(request)
  return response.await()
})

[vRallev]

Not sure if this issue is open for discussion. I really like the proposal, it's a lot clearer and easier to understand. The unclear part to me is how you'd deal with states:

// Proposed:
override fun render(
  props: RequestParameters,
  state: State,
  context: RenderContext<State, Output>
) {
  when (state) {
    is Something -> context.onFirstRender(..) { .. }
  }
}

While context.onFirstRender makes sense for all states, the name is a little ambiguous for state specific workers. Does it mean the first render for this state while the Workflow stays in this state or for forever?

[rjrjr]

It's a lousy name, suggestions welcome. It basically means "I am only going to use this lambda the first time you call render with this key" -- calling it again in later renderings with the same key will keep it running, just like today's workers.

[rjrjr]

RenderContext.runSideEffect(key)?

[vRallev]

runInBackground() or runConcurrently()? Although this could be misleading. runSideEffect() sounds negative, but is probably the most accurate.

My concern with onFirstRender() was that it's not required to call it during the first render.

[zach-klippenstein]

I like to think of this change as replacing a type with a function. The type was sibling to Workflow, and copied all its semantic questions. The function is much more constrained and with a much smaller API surface, and had much fewer questions.

[zach-klippenstein]

The arguments to this function are the key that defines how long the task is kept alive, and distinguishes different calls.

One thing we didn't talk about yesterday that I would like to propose is making the parameter a vararg, so it's easy to pass in multiple values to use as the key. This addresses the concert @bencochran raised about often only wanted to use a subset of fields for equivalence in a way that doesn't require creating a wrapper explicitly. It also mirrors the Compose remember & onCommit APIs.

[zach-klippenstein]

How does the Sink handle backpressure in Swift, @bencochran @dhavalshreyas? One issue we're going with this in Kotlin is that Workers currently experience backpressure, but the RenderContext's Sink does not. I'm not sure this is even something we care about. We don't actually have backpressure in any of our internal code (everything is Observables, not Flowables).

[zach-klippenstein]

Ben's solution passes a ReactiveSwift Lifetime to runSideEffect. @rjrjr For our implementation, I think we'd want simply use a suspend function, like this:

fun runSideEffect(key: Any, sideEffect: suspend () -> Unit)

The sideEffect function gets run in a new coroutine scoped under the workflow on the Unconfined dispatcher, and cancelled when the side effect stops being run or the workflow goes away. All we need to hold onto in the WorkflowNode then is the Job.

[bencochran]

Sorry, haven’t fully caught up here, but one quick note:

Ben's solution passes a ReactiveSwift to runSideEffect

That was mostly for the prototype. I’d imagine we’d either do something like the suspend you mention or create our own notion of Lifetime to keep ReactiveSwift out of the public API.

[zach-klippenstein]

Oops, I think I meant to type Lifetime - fixed my comment.

[rjrjr]

We had a long, deep discussion on this last week, and settled on basically the original proposal:

• Workflows can declare side effects with comparison keys
• A side effect is a lambda called at startup, and which is passed a teardown hook as an argument

Alternatives we played with and rejected include:

• Tying side effects to ViewController: Too damaging to the logic / view separation we've achieved

• Adding mutable storage (based on Lifetime in Swift) to workflow state (https://coderpad.io/WD7KRJCQ)

  • Too easy to abuse
  • It's been challenging to achieve immutable state, feels like a step backward

• Giving every Workflow exactly one side effect, so that Workflow is our only abstraction / lifecycle.

  • In theory more "pure," in practice pretty hard to think about.
  • Workflow and side effects really are different things, it's good for the API to keep that explicit.
  • And there really isn't a Workflow lifecycle, that's just an emergent property of the side effect lifecycle

Let's drill into that last point a bit more. It's not something we'd really realized before, and it's pretty illuminating. Anything that looks like a Workflow lifecycle is really almost entirely about side effects (today's Workers).

In the new model, not terribly different than the current one, the Workflow tree effectively has a keyed map of running side effects that are:

• started when they are first referenced during a render pass
• ended when a render pass runs without referring to them

The keys to this side effect table are namespaced by the workflow tree. What looks like a child workflow "starting" when it is first rendered is almost entirely down to the workers that it declares when it first renders; and which are ended when it is no longer rendered, and so no longer declares them.

The Workflow / SideEffect dichotomy allows Workflow implementations to stay purely declarative and functional, and makes it clear that "real work" belongs in the SideEffect functions they define. Today's Workers and UI event ActionSinks can be built from the new SideEffect primitive. (Or maybe it's that a single sink per workflow is part of the SideEffect plumbing? I lost track there.)

[dhavalshreyas]

While working on prepping Ben’s iOS GUWT implementation for review, I kept bumping into a few things that I was trying to wrap my head around. I took some time to crystalize my concerns from the first-principles perspective. I wanted to note the concerns here and get the team’s feedback before proceeding with the implementation. Happy to disagree and commit :)

It boils down to the following issues:

Type

GUWT proposes changing the Worker API from:

awaitResult(for: Worker)

To:

runSideEffect(key: String, action: (Lifetime) -> Void)

Concern

While it looks like we’re deleting the Worker concept in the proposal, in-fact (as noted by @zach-klippenstein) we’re just switching from using a nominal type Worker to a non-nominal type (Lifetime) -> Void.

In contrast to the nominal, or named, types that comprise most of Swift, non-nominal types are defined in relation to other types.

Non-nominal types cannot be extended .... you can’t add methods or properties or conformance to protocols.

NSHipster

Switching to a non-nominal type would limit how flexible the API is. For example, to create a new kind of convenient Worker/SideEffect, we will not be able to just conform to a type and instead we’ll need to create a Workflow which calls the sideEffect.

While we can work around this, I’m not entirely sure if there’s a win by switching from a nominal type to a non-nominal type.

Key

GUWT proposes replacing Worker's isEquivalent call with a key while making the SideEffect call. The key will conform to Equatable and Hashable.

Concern

This adds additional responsibility on the caller to make sure the key is crafted correctly. Specifically, the key should represent the work that'll be done by action uniquely and comprehensively. If action does a slightly different job, the caller is responsible for making sure the key is different from the one used in the last render call. If it's not - it'll just silently assume that it's the same work and not trigger the new action.

In contrast, Worker's' isEquivalent requirement only expects us to compare the two objects (that are inputs to the corresponding run() actions).

Why?

I spent some time digging into why we need the Hashable requirement. In #961, @zach-klippenstein explains how it'll help with our internal diffing and make it a constant-time lookup vs. a linear search. While this makes sense to me, I want to make sure we're okay with adding an additional requirement on the API to solve an implementation issue.

Wins - Dependencies

I'm excited about removing the ReactiveSwift dependency from the core Workflow library. Workflow uses ReactiveSwift in a very limited way. However, having it be part of the API requires our consumers to pull in that dependency.

Proposal

From the Swift perspective, I would introduce SideEffect as a protocol very similar to the current Worker.

public protocol SideEffect {
    associatedtype Output

    func run(sink: Sink<Output>) -> Lifetime

    func isEquivalent(to otherSideEffect: Self) -> Bool
}

The two big differences being:

• Pass sink as a parameter to run() to send Actions instead of using a SignalProducer to emit Outputs.
• Use an in-house Lifetime to co-ordinate the SideEffect lifetime.

Worker can then conform to SideEffect along with RxSwiftWorker, CombinerWorker..etc

[bencochran]

The proposal as I understood it wasn’t to replace Worker with SideEffect, it was to replace Worker with Workflow. SideEffect is what gives us the mechanism to actually do that. In my understanding of the proposal, Worker still exists exactly as it does today, it’s just represented and hosted in the workflow tree as another Workflow.

[dhavalshreyas]

The proposal as I understood it wasn’t to replace Worker with SideEffect, it was to replace Worker with Workflow. SideEffect is what gives us the mechanism to actually do that. In my understanding of the proposal, Worker still exists exactly as it does today, it’s just represented and hosted in the workflow tree as another Workflow.

👍

Yes, when we had first discussed this, I was pretty excited about this idea.

However, digging into this now, it feels like all we've done is to "require" the Workflow host - even though it doesn't add as much value.

For example, by switching to using a nominal type for SideEffect, this is all we need to do to have the old Worker (or any similar async performer thingie) conform to SideEffect. Compared to here, where we need an intermediate Workflow which adds very little additive value.

Using nominal types, we can get an API very similar to our current proposal like in here and we'll retain the ability to introduce a Workflow host, if we choose to.

[rjrjr]

It seems to me that sink is a big hand wave in this proposal, and reopens a lot of the conversations that were finally resolved by the GUWT proposal. Is there one sink per workflow? Per side effect? Is it a buffer?

If it's the same sink we're already using for UI events, that's a drastic change to Worker semantics, and one we've rejected at least twice already: today each worker is effectively its own sink. If workers start sharing the same buffered sink used for UI events, things get dramatically less deterministic than they are today.

Most of the rest of the argument is about the side effects being harder to work with than workers, which was explicitly not the proposal. We consistently discussed side effects as a fundamental low level building block, upon which we would build conveniences like the Worker abstraction — one which would likely never be used directly by feature developers. So, you're working to solve a problem that we don't have, and introducing unnecessary complexity to do so.

• There is no new burden on feature developers, they continue to use the Worker API they're already used to, with ambiguities about its contract resolved, and a simpler implementation underpinning it.

• I don't see how building Workers and whatever other tools occur to us on this substrate is any harder than it is today. Seems to me it's easier, because we have a single runtime mechanism to think about, not one for Workflow and one for Side Effect.

This feels to me like a rehash of settled issues.

[rjrjr]

It is not expected that anyone would call that method inline. Rather, it's the primitive we use to build various operators.

[dhavalshreyas]

If it's the same sink we're already using for UI events, that's a drastic change to Worker semantics, and one we've rejected at least twice already:

The current GUWT proposal uses the UI sink for events, like here.

Current GUWT proposal:

let sink = context.makeSink(Action.self)
context.runSideEffect(key: "") {
  sink.send(.action)
}

My proposal creates the sink from within the infra and passes it to run().

---

It is not expected that anyone would call that method inline. Rather, it's the primitive we use to build various operators.

Got it. Since this is part of the public API, I was trying to think through all the different ways it can be used/misused.

---

So, you're working to solve a problem that we don't have, and introducing unnecessary complexity to do so.

I was positing that we're doing exactly that by switching to non-nominal types and adding a hashable requirement.

---

Overall, I think the proposal is a step forward. I wanted to make sure the concerns and it's justifications are documented. Since the team feels the proposal as it stands is good, I'll continue with the implementation as-designed.

[zach-klippenstein]

The side effect implementations are done on both platforms. Closing.