[RFC] [DNM] SideEffect as Type

[dhavalshreyas]

Context

In #1174, we introduce the sideEffect API on RenderContext:

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

action is executed the first time a key is encountered and Lifetime.onEnded is called after it's no longer needed.

Example usage:

sink = context.makeSink(Action.self)
context.runSideEffect("entity-" + entityId) { lifetime in 
  fetchEntity(entityId) { sink.send(.gotEntity) }
}

Proposal

Make SideEffect a type:

protocol SideEffect: Hashable {
  func run(sink: Sink<Action>) -> Lifetime
}

and the API becomes:

func run<S: SideEffect>(sideEffect: S)

Why?

Harder to mess-up

The Hashable contract implies that the hashValue will be calculated based on hashing all the contents of the object (SideEffect). If the hashValues are the same, we then compare the two objects to make sure they're the same. This contract gives us a strong guarantee that the block of work that will be executed by the SideEffect will be the same.

If we instead put the responsibility on the consumer of the API to formulate the right key, we will be unnecessarily burdening the caller to carefully generate the right key and the impact of doing this incorrectly will be subtle.

In other words, if we want the key to represent the same action AND the action is a function of (a, b, c) I believe that the the key should be f(a, b, c)

Flexible

Making SideEffect a protocol (instead of having the key and an independent action closure) will make this more flexible. We will then be able to get any of the Workers to conform to the SideEffect. (Instead of having an intermediate Workflow to do the runSideEffect call.)

[dhavalshreyas]

Actually, thinking through this..since we'll be using the sink to dispatch changes - this model might not make as much sense. :(

Back to the drawing board.

This has now been addressed.

[JustinDSN]

Why did we close / reopen?

Did something change?

[AquaGeek]

Squinting, this looks a lot like Worker.

[dhavalshreyas]

It pretty much is - except we're moving the ReactiveSwift specific stuff out of Worker. So instead of it returning SignalProducer<Output>, we'll be passing in a Sink and use an internal concept for Lifetime instead.

Oh, we're also adding a hash value to it.

[dhavalshreyas]

Why did we close / reopen?

Did something change?

Yes, we're now passing the sink on run.

[dhavalshreyas]

This was discussed in #1021, the team has decided to stick with the API as-designed.