Require Signals to get wrapped in a Worker #1025
Conversation
AquaGeek
changed the title
Signals to get wrapped in a Worker
AquaGeek
force-pushed
the
stromberg/wrap-signals-in-worker
branch
from
590e677
to
90178f3
Compare
There was a problem hiding this comment.
Did you consider making SignalWorker abstract and having its own run() method that returns a Signal, mirroring Worker? Seems odd that the api for creating Signal workers vs SignalProducer workers is different, although I'm sure there's a good reason (I'm just curious what it is).
I'm not entirely sure I follow what you're suggesting. |
This causes test failures due to unexpected Workers getting encountered. Instead, subscribe to the signal like we used to in `subscribe()`.
AquaGeek
force-pushed
the
stromberg/wrap-signals-in-worker
branch
from
90178f3
to
220e9d8
Compare
| @@ -270,16 +270,14 @@ fileprivate final class RenderTestContext<T: Workflow>: RenderContextType { | |||
| let sink = Sink<Action> { action in | |||
| observer.send(value: AnyWorkflowAction(action)) | |||
| } | |||
| subscribe(signal: signal) | |||
There was a problem hiding this comment.
Because subscribe has been removed I moved its logic up into this function and am subscribing directly — i.e. I'm not wrapping the Signal here in a worker (it creates test failures due to unexpected workers being encountered).
|
I don't see what the temperature of the stream has to do with it. Making it an extendable type means implementors can implement |
| } | ||
|
|
||
| public func isEquivalent(to otherWorker: SignalWorker) -> Bool { | ||
| return key == otherWorker.key |
There was a problem hiding this comment.
@bencochran Is Key comparison enough here? Do we run into the "need consumers to understand that Key needs to be differentiated enough" problem?
|
We're going to split this PR into two:
That way, we can land the first, integrate it (incrementally), and fix any issues that come out of that. |
|
Sounds great, incremental is better and less painful! |
This should address #402. The aesthetics of the
SignalWorkerare very much open for feedback. We could also leavecontext.subscribein place and create the worker in the infrastructure layer instead.