Skip to content

Add StateMutationSink #167

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Add StateMutationSink #167

merged 1 commit into from
Dec 13, 2022

Conversation

n8chur
Copy link
Collaborator

@n8chur n8chur commented Dec 12, 2022

Background

From @dhavalshreyas's proposal in Notion (with slightly updated examples):

Sinks are used to process Actions, either from SideEffects or from user-interaction events.

We currently have makeSink to process any Action and makeOutputSink, which provides a Sink that sends the Output.

The proposal here is to introduce makeStateMutationSink, which would provide a Sink that automatically mutates State without an intermediate Action.

Before:

struct MyWorkflow: Workflow {
    enum Action: WorkflowAction {
        typealias WorkflowType = MyWorkflow

        case updateTapCount(Int)

        func apply(toState state: inout Int) -> MyWorkflow.Output? {
            switch self {
            case .updateTapCount(let count):
                state.tapCount = count
                return nil
            }
            return nil
        }
    }

    func render(state: State, context: RenderContext<Self>) -> Rendering {
        let sink = context.makeSink(of: Action.self)
        return Screen(onTap: { _ in
            sink.send(.updateTapCount(state.tapCount + 1))
        })
    }
}

After:

struct MyWorkflow: Workflow {
    func render(state: State, context: RenderContext<Self>) -> Rendering {
        let stateMutationSink = context.makeStateMutationSink()
        return Screen(onTap: { _ in
            stateMutationSink.send(\.tapCount, value: state.tapCount + 1)
        })
    }
}

This convenience has been baking in ios-register's WorkflowExperimental framework for quite some time now and has proven useful to many.

Additions to existing convenience

These changes area a direct port of that code (including the associated tests) with one additional convenience which would allow for mutating state with a closure rather than just the KeyPath/Value syntax:

struct MyWorkflow: Workflow {
    func render(state: State, context: RenderContext<Self>) -> Rendering {
        let stateMutationSink = context.makeStateMutationSink()
        return Screen(onTap: { _ in
            stateMutationSink.send { state in
               state.tapCount += 1
               state.tapped = true
            }
        })
    }
}

Checklist

  • Unit Tests
  • UI Tests
  • Snapshot Tests (iOS only)
  • I have made corresponding changes to the documentation

@n8chur n8chur force-pushed the westin/state-mutation-sink branch from d2fa8bd to 336b984 Compare December 12, 2022 22:05
@n8chur
Copy link
Collaborator Author

n8chur commented Dec 12, 2022

Let me know if you feel this deserves documentation outside of the code doc comment, and if so, where it should go!

@n8chur n8chur force-pushed the westin/state-mutation-sink branch from 336b984 to 778a642 Compare December 12, 2022 22:08
@n8chur n8chur marked this pull request as ready for review December 12, 2022 22:09
@n8chur n8chur requested a review from a team as a code owner December 12, 2022 22:09
@n8chur n8chur changed the title [WIP DNR] Add StateMutationSink Add StateMutationSink Dec 12, 2022
func test_singleUpdate() {
let host = WorkflowHost(workflow: TestWorkflow(value: 100, signal: output))

let gotValueExpectation = expectation(description: "Got expected value")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't really affect the semantics of the tests at all, but i think the expectation machinery isn't strictly needed in this instance. the processing should happen synchronously as soon as the values are received.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you prefer that I try removing it?

I just directly ported the test from ios-register and didn't dig into how I might be able to improve these tests much...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, no it's fine as-is, just noticed this. i didn't realize initially that this code was just being moved.

@n8chur n8chur merged commit 0d11788 into main Dec 13, 2022
@n8chur n8chur deleted the westin/state-mutation-sink branch December 13, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants