diff --git a/swift/Workflow/Sources/SubtreeManager.swift b/swift/Workflow/Sources/SubtreeManager.swift index 8584cc134..ba10e7a3a 100644 --- a/swift/Workflow/Sources/SubtreeManager.swift +++ b/swift/Workflow/Sources/SubtreeManager.swift @@ -13,6 +13,9 @@ extension WorkflowNode { /// Sinks from the outside world (i.e. UI) private var eventPipes: [EventPipe] = [] + /// Typed sinks from the previous render pass + private var previousSinks: [ObjectIdentifier:AnyTypedSink] = [:] + /// The current array of children private (set) internal var childWorkflows: [ChildKey:AnyChildWorkflow] = [:] @@ -34,6 +37,7 @@ extension WorkflowNode { /// Create a workflow context containing the existing children let context = Context( + previousSinks: previousSinks, originalChildWorkflows: childWorkflows, originalChildWorkers: childWorkers) @@ -52,8 +56,12 @@ extension WorkflowNode { /// Merge all of the signals together from the subscriptions. self.subscriptions = Subscriptions(eventSources: context.eventSources, eventPipe: EventPipe()) + /// Captured the typed sinks from this render pass to allow reuse. + self.previousSinks = context.sinkStore.usedSinks + /// Capture all the pipes to be enabled after render completes. self.eventPipes = context.eventPipes + self.eventPipes.append(contentsOf: context.sinkStore.eventPipes()) self.eventPipes.append(self.subscriptions.eventPipe) /// Set all event pipes to `pending`. @@ -119,12 +127,16 @@ extension WorkflowNode.SubtreeManager { } +// MARK: - Render Context + extension WorkflowNode.SubtreeManager { /// The workflow context implementation used by the subtree manager. fileprivate final class Context: RenderContextType { private (set) internal var eventPipes: [EventPipe] + + private (set) internal var sinkStore: SinkStore private let originalChildWorkflows: [ChildKey:AnyChildWorkflow] private (set) internal var usedChildWorkflows: [ChildKey:AnyChildWorkflow] @@ -134,9 +146,11 @@ extension WorkflowNode.SubtreeManager { private (set) internal var eventSources: [Signal, NoError>] = [] - internal init(originalChildWorkflows: [ChildKey:AnyChildWorkflow], originalChildWorkers: [AnyChildWorker]) { + internal init(previousSinks: [ObjectIdentifier:AnyTypedSink], originalChildWorkflows: [ChildKey:AnyChildWorkflow], originalChildWorkers: [AnyChildWorker]) { self.eventPipes = [] + self.sinkStore = SinkStore(previousSinks: previousSinks) + self.originalChildWorkflows = originalChildWorkflows self.usedChildWorkflows = [:] @@ -191,13 +205,12 @@ extension WorkflowNode.SubtreeManager { func makeSink(of actionType: Action.Type) -> Sink where Action : WorkflowAction, WorkflowType == Action.WorkflowType { - let eventPipe = EventPipe() + let typedSink = sinkStore.findOrCreate(actionType: Action.self) let sink = Sink { action in - let event = Output.update(AnyWorkflowAction(action), source: .external) - eventPipe.handle(event: event) + typedSink.handle(action: action) } - eventPipes.append(eventPipe) + return sink } @@ -227,6 +240,70 @@ extension WorkflowNode.SubtreeManager { } +// MARK: - Reusable Sink + +extension WorkflowNode.SubtreeManager { + fileprivate struct SinkStore { + private var previousSinks: [ObjectIdentifier:AnyTypedSink] + private (set) var usedSinks: [ObjectIdentifier:AnyTypedSink] + + init(previousSinks: [ObjectIdentifier:AnyTypedSink]) { + self.previousSinks = previousSinks + self.usedSinks = [:] + } + + mutating func findOrCreate(actionType: Action.Type) -> TypedSink { + let key = ObjectIdentifier(actionType) + + let typedSink: TypedSink + + if let previousSink = previousSinks.removeValue(forKey: key) as? TypedSink { + // Reused a previous sink, creating a new event pipe to send the action through. + previousSink.eventPipe = EventPipe() + typedSink = previousSink + } else if let usedSink = usedSinks[key] as? TypedSink { + // Multiple sinks using the same backing sink. + typedSink = usedSink + } else { + // Create a new typed sink. + typedSink = TypedSink() + } + + usedSinks[key] = typedSink + + return typedSink + } + + func eventPipes() -> [EventPipe] { + let eventPipes = usedSinks.values.map { typedSink -> EventPipe in + typedSink.eventPipe + } + + return eventPipes + } + } + + fileprivate class AnyTypedSink { + var eventPipe: EventPipe + + init() { + eventPipe = EventPipe() + } + } + + fileprivate final class TypedSink: AnyTypedSink where Action.WorkflowType == WorkflowType { + + func handle(action: Action) { + let output = Output.update(AnyWorkflowAction(action), source: .external) + + eventPipe.handle(event: output) + } + } +} + + +// MARK: - EventPipe + extension WorkflowNode.SubtreeManager { fileprivate final class EventPipe { @@ -295,6 +372,8 @@ extension WorkflowNode.SubtreeManager { } } +// MARK: - ChildKey + extension WorkflowNode.SubtreeManager { struct ChildKey: Hashable { @@ -320,6 +399,8 @@ extension WorkflowNode.SubtreeManager { } +// MARK: - Workers + extension WorkflowNode.SubtreeManager { /// Abstract base class for running children in the subtree. @@ -372,6 +453,8 @@ extension WorkflowNode.SubtreeManager { } +// MARK: - Subscriptions + extension WorkflowNode.SubtreeManager { fileprivate final class Subscriptions { private var (lifetime, token) = Lifetime.make() @@ -395,6 +478,8 @@ extension WorkflowNode.SubtreeManager { } +// MARK: - Child Workflows + extension WorkflowNode.SubtreeManager { /// Abstract base class for running children in the subtree. @@ -415,9 +500,9 @@ extension WorkflowNode.SubtreeManager { } } - + fileprivate final class ChildWorkflow: AnyChildWorkflow { - + private let node: WorkflowNode private var outputMap: (W.Output) -> AnyWorkflowAction diff --git a/swift/Workflow/Tests/ConcurrencyTests.swift b/swift/Workflow/Tests/ConcurrencyTests.swift index e23e837fc..e2d063523 100644 --- a/swift/Workflow/Tests/ConcurrencyTests.swift +++ b/swift/Workflow/Tests/ConcurrencyTests.swift @@ -13,7 +13,7 @@ final class ConcurrencyTests: XCTestCase { let expectation = XCTestExpectation() var first = true - var observedScreen: TestWorkflow.TestScreen? = nil + var observedScreen: TestScreen? = nil let disposable = host.rendering.signal.observeValues { rendering in if first { @@ -70,6 +70,98 @@ final class ConcurrencyTests: XCTestCase { disposable?.dispose() } + // A `sink` is invalidated after a single action has been received. However, if the next `render` pass uses a sink + // of the same type, actions sent to an old sink should be proxied through the new sink. + // This allows for a UI that does not synchronously update to use the new sink. + func test_old_sink_proxies_to_new_sink() { + let host = WorkflowHost(workflow: TestWorkflow()) + + // Capture the initial screen and corresponding closure that uses the original sink. + let initialScreen = host.rendering.value + XCTAssertEqual(0, initialScreen.count) + + // Send an action to the workflow. This invalidates this sink, but the next render pass declares a + // sink of the same type. + initialScreen.update() + + let secondScreen = host.rendering.value + XCTAssertEqual(1, secondScreen.count) + + // Send an action from the original screen and sink. It should be proxied through the most recent sink. + initialScreen.update() + + let thirdScreen = host.rendering.value + XCTAssertEqual(2, thirdScreen.count) + } + + // If a previous `sink` has been invalidated due to receiving an action, and a new sink of the same type + // is not redeclared on the subsequent render pass, it should be considered invalid and not allowed to send actions. + func test_invalidate_old_sink_if_not_redeclared() { + let host = WorkflowHost(workflow: OneShotWorkflow()) + + // Capture the initial screen and corresponding closure that uses the original sink. + let initialScreen = host.rendering.value + XCTAssertEqual(0, initialScreen.count) + + // Send an action to the workflow. This invalidates this sink, but the next render pass declares a + // sink of the same type. + initialScreen.update() + + let secondScreen = host.rendering.value + XCTAssertEqual(1, secondScreen.count) + + // MANUAL TEST CASE: Uncomment to validate this fatal errors. + // Calling `update` uses the original sink. This will fail with a fatalError as the sink was not redeclared. + //initialScreen.update() + + // If the sink *was* still valid, this would be correct. However, it should just fail and be `1` still. + //XCTAssertEqual(2, secondScreen.count) + // Actual expected result, if we had not fatal errored. + XCTAssertEqual(1, host.rendering.value.count) + + struct OneShotWorkflow: Workflow { + typealias Output = Never + struct State { + var count: Int + } + + func makeInitialState() -> State { + return State(count: 0) + } + + func workflowDidChange(from previousWorkflow: OneShotWorkflow, state: inout State) { + } + + enum Action: WorkflowAction { + typealias WorkflowType = OneShotWorkflow + + case updated + + func apply(toState state: inout State) -> Never? { + switch self { + case .updated: + state.count += 1 + return nil + } + } + } + + typealias Rendering = TestScreen + func render(state: State, context: RenderContext) -> Rendering { + let update: () -> Void + if state.count == 0 { + let sink = context.makeSink(of: Action.self) + update = { + sink.send(.updated) + } + } else { + update = {} + } + return TestScreen(count: state.count, update: update) + } + } + } + // When events are queued, the debug info must be received in the order the events were processed. // This is to validate that `enableEvents` is tail recursive when handled by the WorkflowHost. func test_debugEventsAreOrdered() { @@ -106,6 +198,64 @@ final class ConcurrencyTests: XCTestCase { disposable?.dispose() } + func test_childWorkflowsAreSynchronous() { + + let host = WorkflowHost(workflow: ParentWorkflow()) + + let initialScreen = host.rendering.value + XCTAssertEqual(0, initialScreen.count) + initialScreen.update() + + // This update happens immediately as a new rendering is generated synchronously. + // Both the child updates from the action (incrementing state by 1) as well as the + // parent from the output (incrementing its state by 10) + XCTAssertEqual(11, host.rendering.value.count) + + struct ParentWorkflow: Workflow { + struct State { + var count: Int + } + + func makeInitialState() -> State { + return State(count: 0) + } + + func workflowDidChange(from previousWorkflow: ParentWorkflow, state: inout State) { + } + + enum Action: WorkflowAction { + typealias WorkflowType = ParentWorkflow + + case update + + func apply(toState state: inout State) -> Output? { + switch self { + case .update: + state.count += 10 + return nil + } + } + } + + typealias Rendering = TestScreen + + func render(state: State, context: RenderContext) -> Rendering { + var childScreen = TestWorkflow(running: .idle, signal: TestSignal()) + .mapOutput({ output -> Action in + switch output { + case .emit: + return .update + } + }) + .rendered(with: context) + + childScreen.count += state.count + return childScreen + } + } + + } + // Signals are subscribed on a different scheduler than the UI scheduler, // which means that if they fire immediately, the action will be received after // `render` has completed. @@ -157,6 +307,199 @@ final class ConcurrencyTests: XCTestCase { disposable?.dispose() } + // Since event pipes are reused for the same type, validate that the `AnyWorkflowAction` + // defined event pipes still sends through the correct action. + // Because they are just backed by type, not the actual action, they send the actions appropriately. + // (Thus, there is a single backing `TypedSink` for `AnyWorkflowAction`, but the correct action is applied. + func test_multipleAnyWorkflowAction_sinksDontOverrideEachOther() { + + let host = WorkflowHost(workflow: AnyActionWorkflow()) + + let initialScreen = host.rendering.value + XCTAssertEqual(0, initialScreen.count) + + // Update using the first action. + initialScreen.updateFirst() + + let secondScreen = host.rendering.value + XCTAssertEqual(1, secondScreen.count) + + // Update using the second action. + secondScreen.updateSecond() + XCTAssertEqual(11, host.rendering.value.count) + + struct AnyActionWorkflow: Workflow { + enum Output { + case emit + } + + struct State { + var count: Int + } + + func makeInitialState() -> State { + return State(count: 0) + } + + func workflowDidChange(from previousWorkflow: AnyActionWorkflow, state: inout State) { + + } + + enum FirstAction: WorkflowAction { + + typealias WorkflowType = AnyActionWorkflow + case update + + func apply(toState state: inout State) -> Output? { + switch self { + case .update: + state.count += 1 + } + return nil + } + } + + enum SecondAction: WorkflowAction { + + typealias WorkflowType = AnyActionWorkflow + case update + + func apply(toState state: inout State) -> Output? { + switch self { + case .update: + state.count += 10 + } + return nil + } + } + + struct TestScreen { + var count: Int + var updateFirst: () -> Void + var updateSecond: () -> Void + } + typealias Rendering = TestScreen + + func render(state: State, context: RenderContext) -> Rendering { + + let firstSink = context + .makeSink( + of: AnyWorkflowAction.self) + .contraMap { (action: FirstAction) -> AnyWorkflowAction in + AnyWorkflowAction(action) + } + + let secondSink = context + .makeSink( + of: AnyWorkflowAction.self) + .contraMap { (action: SecondAction) -> AnyWorkflowAction in + AnyWorkflowAction(action) + } + + return TestScreen( + count: state.count, + updateFirst: { + firstSink.send(.update) + }, + updateSecond: { + secondSink.send(.update) + }) + } + } + + } + + /// Since event pipes are allowed to be reused, and shared for the same backing action type + /// validate that they are also only reused for the same source type - ie: a sink for an + /// action should not use the same event pipe as a worker that maps to the same action type. + /// This will likely need to be a test that will be "correct" when it fatal errors + /// since the behavior would be reusing the wrong event pipe for an old sink that was not + /// redeclared. + func test_eventPipesAreOnlyReusedForSameSource() { + let host = WorkflowHost(workflow: SourceDifferentiatingWorkflow(step: .first)) + + //let initialScreen = host.rendering.value + XCTAssertEqual(0, host.rendering.value.count) + + // Update to the second "step", which will cause a render update, with the sink not being declared. + host.update(workflow: SourceDifferentiatingWorkflow(step: .second)) + // The state should be the same, even though it rendered again. + XCTAssertEqual(0, host.rendering.value.count) + + // MANUAL TEST CASE + // This will fail, as the sink held by `initialScreen` has not be redeclared, even though the backing action is the same for the worker. + // Uncomment to validate this test fails with a fatal error. + //initialScreen.update() + XCTAssertEqual(0, host.rendering.value.count) + + struct SourceDifferentiatingWorkflow: Workflow { + + var step: Step + enum Step { + case first + case second + } + + struct State { + var count: Int + } + + func makeInitialState() -> State { + return State(count: 0) + } + + func workflowDidChange(from previousWorkflow: SourceDifferentiatingWorkflow, state: inout State) { + } + + enum Action: WorkflowAction { + typealias WorkflowType = SourceDifferentiatingWorkflow + + case update + + func apply(toState state: inout State) -> Never? { + switch self { + case .update: + state.count += 1 + return nil + } + } + } + + struct DelayWorker: Worker { + typealias Output = Action + + func run() -> SignalProducer { + return SignalProducer(value: .update).delay(0.1, on: QueueScheduler.main) + } + + func isEquivalent(to otherWorker: DelayWorker) -> Bool { + return true + } + } + + typealias Rendering = TestScreen + + func render(state: State, context: RenderContext) -> Rendering { + let update: () -> Void + switch step { + + case .first: + let sink = context.makeSink(of: Action.self) + update = { sink.send(.update) } + + case .second: + update = {} + } + + context.awaitResult(for: DelayWorker(), outputMap: { $0 }) + + return TestScreen(count: state.count, update: update) + } + } + } + + // MARK - Test Types + fileprivate class TestSignal { let (signal, observer) = Signal.pipe() var sent: Bool = false @@ -169,8 +512,19 @@ final class ConcurrencyTests: XCTestCase { } } + + fileprivate struct TestScreen { + var count: Int + var update: () -> Void + } + + fileprivate struct TestWorkflow: Workflow { + enum Output { + case emit + } + init(running: Running = .idle, signal: TestSignal = TestSignal()) { self.running = running self.signal = signal @@ -194,11 +548,11 @@ final class ConcurrencyTests: XCTestCase { } } - func makeInitialState() -> ConcurrencyTests.TestWorkflow.State { + func makeInitialState() -> State { return State(count: 0, running: self.running, signal: self.signal) } - func workflowDidChange(from previousWorkflow: ConcurrencyTests.TestWorkflow, state: inout ConcurrencyTests.TestWorkflow.State) { + func workflowDidChange(from previousWorkflow: TestWorkflow, state: inout State) { } enum Action: WorkflowAction { @@ -206,23 +560,18 @@ final class ConcurrencyTests: XCTestCase { case update - func apply(toState state: inout ConcurrencyTests.TestWorkflow.State) -> ConcurrencyTests.TestWorkflow.Output? { + func apply(toState state: inout State) -> Output? { switch self { case .update: state.count += 1 - return nil + return .emit } } } - struct TestScreen { - var count: Int - var update: () -> Void - } - typealias Rendering = TestScreen - func render(state: ConcurrencyTests.TestWorkflow.State, context: RenderContext) -> ConcurrencyTests.TestWorkflow.TestScreen { + func render(state: State, context: RenderContext) -> Rendering { switch state.running { case .idle: @@ -246,11 +595,11 @@ final class ConcurrencyTests: XCTestCase { struct TestWorker: Worker { typealias Output = TestWorkflow.Action - func run() -> SignalProducer { + func run() -> SignalProducer { return SignalProducer(value: .update) } - func isEquivalent(to otherWorker: ConcurrencyTests.TestWorkflow.TestWorker) -> Bool { + func isEquivalent(to otherWorker: TestWorker) -> Bool { return true } }