square / workflow

A Swift and Kotlin library for making composable state machines, and UIs driven by those state machines.
https://square.github.io/workflow
Apache License 2.0
1.12k stars 80 forks source link

Sinks not working for child workflows #1212

Closed AlexandruIstrate closed 4 years ago

AlexandruIstrate commented 4 years ago

I have a problem when creating and using a sink in a child workflow.

I needed to have a compossible node structure so I defined a workflow with children. I named it GraphWorkflow. Inside of it I defined a list of workflows that output a String and Render a GraphScreen (see the use of AnyWorkflow<GraphScreen, String>).

GraphWorkflow.swift

// MARK: Data

public struct WorkflowData {
    var key: String
    var workflows: [AnyWorkflow<GraphScreen, String>] = []
}

// MARK: Inititialization and Output

public struct GraphWorkflow: Workflow {
    public typealias Output = String

    var data: WorkflowData
    var baseScreen: AnyScreen?

    init(data: WorkflowData, baseScreen: AnyScreen? = nil) {
        self.data = data
        self.baseScreen = baseScreen
    }
}

// MARK: State

extension GraphWorkflow {
    public struct State { }

    public func makeInitialState() -> GraphWorkflow.State {
        return State()
    }

    public func workflowDidChange(from previousWorkflow: GraphWorkflow, state: inout GraphWorkflow.State) {
        print("Changed")
    }
}

// MARK: Actions

extension GraphWorkflow {
    public enum Action: WorkflowAction {
        public typealias WorkflowType = GraphWorkflow

        case run(String)

        public func apply(toState state: inout GraphWorkflow.State) -> String? {
            switch self {
            case .run(let value):
                print("Flow output \(value)")

            return nil
        }
    }
}

// MARK: Rendering

extension GraphWorkflow {
    public typealias Rendering = GraphScreen

    public func render(state: GraphWorkflow.State, context: RenderContext<GraphWorkflow>) -> GraphScreen {

        let screen = GraphScreen(baseScreen: baseScreen, key: UUID().description, title: data.key, icon: UIImage(systemName: "square.and.arrow.up.on.square")!, background: .red)

        // Render all of the children
        for child in data.workflows {
            let output = child.mapOutput({ output -> GraphWorkflow.Action in
                return .run(output)
            }).rendered(with: context, key: UUID().description)
            screen.children.append(output)
        }

        return screen
    }
}

GraphScreen.swift

public struct GraphScreen: Screen {
    var baseScreen: AnyScreen
    var key: AnyHashable

    var children: [GraphScreen] = []

    public init<ScreenType: Screen, Key: Hashable>(baseScreen screen: ScreenType, key: Key?) {
        self.baseScreen = AnyScreen(screen)

        if let key = key {
            self.key = AnyHashable(key)
        } else {
            self.key = AnyHashable(ObjectIdentifier(ScreenType.self))
        }
    }

    public init<ScreenType: Screen>(baseScreen screen: ScreenType) {
        let key = Optional<AnyHashable>.none
        self.init(baseScreen: screen, key: key)
    }

    public func viewControllerDescription(environment: ViewEnvironment) -> ViewControllerDescription {
        return GraphViewController.description(for: self, environment: environment)
    }
}

private final class GraphViewController: ScreenViewController<GraphScreen> {

    override func viewDidLoad() {
        super.viewDidLoad()

        // Add the children views
        for child in screen.children {
            let describedChild = DescribedViewController(screen: child.baseScreen, environment: environment)
            addChild(describedChild)
            view.addSubview(describedChild.view)
            describedChild.didMove(toParent: self)
        }
    }
}

Now, to allow for workflows with different types to become children of GraphWorkflow, I have defined my other workflows as normal, isolated workflows, with their own rendering type but with a String output. To make them compatible with AnyWorkflow<GraphScreen, String>, I used the handy conversion method mapRendering.

This is an example of such workflow:

// MARK: Initialization and Output

public struct TextWorkflow: Workflow {
    public typealias Output = String

    var data: WorkflowData

    public init(data: WorkflowData) {
        self.data = data
    }
}

// MARK: State

extension TextWorkflow {
    public struct State {
        var text: String
    }

    public func makeInitialState() -> State {
        return State(text: "")
    }

    public func workflowDidChange(from previousWorkflow: TextWorkflow, state: inout TextWorkflow.State) {
        print("Changed")
    }
}

// MARK: Actions

extension TextWorkflow {
    enum Action: WorkflowAction {
        typealias WorkflowType = TextWorkflow

        case textChanged(String)
        case editingBegan
        case editingEnded

        func apply(toState state: inout TextWorkflow.State) -> String? {
            switch self {
            case .editingBegan:
                break
            case .editingEnded:
                break
            case .textChanged(let text):
                print("New text \(text)")
            }

            return nil
        }
    }
}

// MARK: Rendering

extension TextWorkflow {
    public typealias Rendering = TextScreen

    public func render(state: State, context: RenderContext<TextWorkflow>) -> TextScreen {
        let sink = context.makeSink(of: TextWorkflow.Action.self)
        return TextScreen(text: state.text, onEditingBegan: {
            sink.send(.editingBegan)
        }, onTextChanged: { text in
            sink.send(.textChanged(text))
        }, onEditingEnded: { text in
            // Firstly, inform the callback using the last text value
            sink.send(.textChanged(text))

            // Send the editing ended event
            sink.send(.editingEnded)
        })
    }
}

NOTE: The rendering part for the TextWorkflow is not that important. It's just a ScreenViewController<TextScreen> with a text field and some callbacks.

Then, creating a composite workflow is as simple as:

let wf = GraphWorkflow(data: WorkflowData(key: "Root"))
wf.data.workflows.append(TextWorkflow(data: WorkflowData(key: "Text Workflow"))
    .mapRendering({ rendering in
        return GraphScreen(baseScreen: rendering)
    })
)

// Put the root workflow inside a ContainerViewController

The problem is with the sink inside of render in TextWorkflow. The callbacks from the screen get called, but the sink doesn't trigger the apply method in Action. Because of this, I can't update the flow using the UI. Is there any reason for the sink not to work? Or is it perhaps that I did something wrong in my node composition?

Thanks for the help! Alexandru

zach-klippenstein commented 4 years ago

I'm not very familiar with Swift so this is just a guess, but this line looks suspicious to me:

.rendered(with: context, key: UUID().description)

Is that generating a new UUID every time? If so, all your child workflows will be restarted on every render pass, and the ones from the previous render pass will be stopped, which is why their sinks stop working.

AlexandruIstrate commented 4 years ago

You are indeed correct. UUID() generates a new universally unique identifier every time. That might be the problem. If I am correct, the key must be unique between all of the children. So I could use, for example, the name of the workflow, such as TextWorkflow or Child 1, Child 2, etc? I'll give this a try and let you know if it works. Thank you for the quick reply!

zach-klippenstein commented 4 years ago

That should work. As long as the key is derived from something that identifies the child workflow consistently across render passes.

bencochran commented 4 years ago

Yeah, you’ll need stable (and unique) keys between renders. I’d consider making some kind of identifier part of the items held in WorkflowData (like maybe var workflows: [(identifier: String, workflow: AnyWorkflow<…>], or a struct to wrap that up).

One other thing you’re going to need is to implement screenDidChange in your GraphViewController. When your workflow tree rerenders, the new root screen will be handed to that method to update itself and its children (calling update(screen:environment:) on each child DescribedViewController with its appropriate screen). Which also means you’ll need to keep those child view controllers around for use somewhere (another good use for the stable identifiers defined above).

AlexandruIstrate commented 4 years ago

Also, if I want to render children, should I use child.rendered(with:key:) or context.render(workflow:key:outputMap:)? What is the difference between the two?

AlexandruIstrate commented 4 years ago

I finally figured it out. It turns out the sink wasn't firing because of the way I was presenting my ContainerViewController. Instead of using present(_:animated:completion:), I was hosting the view controller as a child like this:

let container = ContainerViewController(
    workflow: GraphWorkflow(data: WorkflowData(key: "Root"))
)

let containerView = container.view!
self.view.addSubview(containerView)
containerView.translatesAutoresizingMaskIntoConstraints = false

NSLayoutConstraint.activate([
    containerView.leadingAnchor.constraint(equalTo: self.view.leadingAnchor),
    containerView.trailingAnchor.constraint(equalTo: self.view.trailingAnchor),
    containerView.topAnchor.constraint(equalTo: self.view.topAnchor),
    containerView.bottomAnchor.constraint(equalTo: self.view.bottomAnchor)
])

What I should have used was as simple as:

let container = ContainerViewController(
    workflow: GraphWorkflow(data: WorkflowData(key: "Root"))
)
present(container, animated: true, completion: nil)

Once I swapped out the initial code with the new version, everything started working. However, I would like to have the ContainerViewController embedded in another UIViewController, via an embed segue or just programatically. Is there any way to make that work and still have the sink operational?

dhavalshreyas commented 4 years ago

Is there a sample project where this bug repros? Happy to look into it if there is.

Is the block that's calling sink.send() getting called? Is the Action.apply() getting called?

AlexandruIstrate commented 4 years ago

Is there a sample project where this bug repros? Happy to look into it if there is.

Is the block that's calling sink.send() getting called? Is the Action.apply() getting called?

Currently, I am using Square Workflow in a private project. I'm afraid I can't share the source code of that project. I do, however, have a test/demo project that consists of some simple code that essentially reproduces the bug in question. I can upload the demo project to GitHub if you would like to experiment with it. I would also make sure to mark the problem areas with comments and explanations.

dhavalshreyas commented 4 years ago

Thanks @AlexandruIstrate! That'll be perfect.

AlexandruIstrate commented 4 years ago

I have created a repository with the demo project. Essentially, in the file ViewController.swift you have the choice between two methods (1 and 2). They are marked so that you know what each does. The difference between the two is that in one, the ViewController is getting presented modally over the top and in the other it gets embedded as a child.

Link to the repo: SquareWorkflowDemo

bencochran commented 4 years ago

It looks like your ViewController is missing the addChild(container) and container.didMove(toParent:), resulting in UI events not being correctly delivered to the text field and the text field delegate methods in HelloViewController never being called. Put up a PR that appears to fix the issue on my end.

AlexandruIstrate commented 4 years ago

It seems like the PR by @bencochran fixes my problem. I'm happy to close the issue if that's alright with you. Thanks for the help!