pointfreeco / swift-composable-architecture

A library for building applications in a consistent and understandable way, with composition, testing, and ergonomics in mind.
https://www.pointfree.co/collections/composable-architecture
MIT License
12.35k stars 1.44k forks source link

Combine enum case reducers #69

Closed slashmo closed 4 years ago

slashmo commented 4 years ago

In my project, I have a Content enum with the cases .group and .request. I also have both GroupAction and RequestAction. Here is the contentReducer I came up with:

let contentReducer = Reducer<RequestCollection.Content, ContentAction, Void> { state, action, _ in
  switch state {
  case var .group(group):
    guard case let .group(groupAction) = action else { return .none }
    switch groupAction {
    case let .nameChanged(name):
      group.name = name
      state = .group(group)
    }
  case var .request(request):
    guard case let .request(requestAction) = action else { return .none }
    switch requestAction {
    case let .nameChanged(name):
      request.name = name
      state = .request(request)
    }
  }
  return .none
}

What I don't like about this is that I have to guard case let on the ContentAction where a .group case should only be possible to be sent to a Group. In an ideal scenario I'd like to compose this reducer from both groupReducer and requestReducer. I'll try and prototype a higher order reducer for that purpose.

Did someone else already encounter something similar?

slashmo commented 4 years ago

I could "reduce" the contentReducer by calling out to groupReducer and requestReducer manually:

let contentReducer = Reducer<RequestCollection.Content, ContentAction, Void> { state, action, _ in
  switch state {
  case var .group(group):
    guard case let .group(groupAction) = action else { return .none }
    let effect = groupReducer(&group, groupAction).map(ContentAction.group)
    state = .group(group)
    return effect
  case var .request(request):
    guard case let .request(requestAction) = action else { return .none }
    let effect = requestReducer(&request, requestAction).map(ContentAction.request)
    state = .request(request)
    return effect
  }
}

This somehow feels similar to the index juggling before the .forEach operator on Reducer became a thing. A lot of boilerplate, especially the extracting of the ContentAction that has to only vary slightly for each case.

boudavid commented 4 years ago

If I understood correctly, your implementation of actions looks like this :

enum ContentAction {
  case group(GroupAction)
  case request(RequestAction)
}

enum GroupAction {
  case nameChanged(String)
}

enum RequestAction {
  case nameChanged(String)
}

Is it correct ? Can you also explain why the switch is on the state and not the action ? I'm just trying to understand what you want to achieve. 😃

In an ideal scenario I'd like to compose this reducer from both groupReducer and requestReducer.

Maybe the Reducer.combine method is what you're looking for ? Typically, it's used like this :

let someReducer = Reducer.combine(
  Reducer<SomeState, SomeAction, SomeEnvironment> { state, action, environment in
    switch action {
      // ...
    }
  },
  anotherReducer.pullback(
    state: \.another,
    action: /SomeAction.another,
    environment: { _ in AnotherEnvironment() } 
  ),
  // Other reducers...
)

So in your case, you could combine the contentReducer with the groupReducer and requestReducer that are pulled back.

slashmo commented 4 years ago

Yes, those are the actions I use. (Sorry for not including it myself :v:). I have basically something like a file system. Both Groups and Requests may be nested in Groups. That's why I have the Content enum. Soon, GroupAction would also get one additional action to send a ContentAction to one specific Content it holds.

So in your case, you could combine the contentReducer with the groupReducer and requestReducer that are pulled back

Do you mean something like this?

let contentReducer = Reducer<RequestCollection.Content, ContentAction, Void>.combine(
  groupReducer.pullback(state: \RequestCollection.Content.group, action: /ContentAction.group, environment: { $0 }),
  requestReducer.pullback(state: \RequestCollection.Content.request, action: /ContentAction.request, environment: { $0 })
)

This would only work if there were WritableKeyPaths for each case. I could work around that by adding a get/set Property for each case to the Content struct. Maybe an API similar to pullback that pulls the state along a CasePath instead of KeyPath could help with that.

What I don't quite like about this approach is that for each new case I'd have to remember to add a new pulled back reducer to the contentReducer without the compiler warning me that not all cases have been covered.

stephencelis commented 4 years ago

This would only work if there were WritableKeyPaths for each case. I could work around that by adding a get/set Property for each case to the Content struct. Maybe an API similar to pullback that pulls the state along a CasePath instead of KeyPath could help with that.

It's definitely possible to write a pullback against enum state using enum properties or by defining a new one to work with state case paths. It's a problem we're actively exploring solutions for and will hopefully have something that comes with the library soon.

What I don't quite like about this approach is that for each new case I'd have to remember to add a new pulled back reducer to the contentReducer without the compiler warning me that not all cases have been covered.

This is already the unfortunate reality of pullback along action enums, so state enums won't be an exception. At the very least this glue code is easily tested, but it would be nice if a language-level affordance allowed us to recapture this exhaustiveness in some way.

slashmo commented 4 years ago

It's a problem we're actively exploring solutions for and will hopefully have something that comes with the library soon.

Awesome, let me know if I can help out with a PR.

This is already the unfortunate reality of pullback along action enums, so state enums won't be an exception. At the very least this glue code is easily tested, but it would be nice if a language-level affordance allowed us to recapture this exhaustiveness in some way.

Makes sense, thanks for the explanation.

fabfelici commented 4 years ago

It's definitely possible to write a pullback against enum state using enum properties or by defining a new one to work with state case paths.

I also use to model the state with an enum sometimes so I have been playing with the code and I implemented a new pullback that seems to work with enum states by using CasePath.

public func pullback<GlobalState, GlobalAction, GlobalEnvironment>(
    state toLocalState: CasePath<GlobalState, State>,
    action toLocalAction: CasePath<GlobalAction, Action>,
    environment toLocalEnvironment: @escaping (GlobalEnvironment) -> Environment
) -> Reducer<GlobalState, GlobalAction, GlobalEnvironment> {
    .init { globalState, globalAction, globalEnvironment in
        guard let localAction = toLocalAction.extract(from: globalAction),
            var localState = toLocalState.extract(from: globalState) else { return .none }
        let effect = self.reducer(
            &localState,
            localAction,
            toLocalEnvironment(globalEnvironment)
        )
        .map(toLocalAction.embed)
        globalState = toLocalState.embed(localState)
        return effect
    }
}

and here is a quick test:

func testEnumStateCasePathPullback() {

    enum State: Equatable {
        case incrementing(Int)
        case doubling(Int)
    }

    enum StateAction: Equatable {
        case apply
        case toggle
    }

    let incrementReducer = Reducer<Int, Void, Void> { state, action, env in
        state += 1
        return .none
    }

    let doubleReducer = Reducer<Int, Void, Void> { state, action, env in
        state *= 2
        return .none
    }

    let reducer = Reducer.combine(
        incrementReducer.pullback(
            state: /State.incrementing,
            action: /StateAction.apply,
            environment: { $0 }
        ),
        doubleReducer.pullback(
            state: /State.doubling,
            action: /StateAction.apply,
            environment: { $0 }
        ),
        .init { state, action, env in
            switch action {
                case .toggle:
                    switch state {
                        case .doubling(let value):
                            state = .incrementing(value)
                        case .incrementing(let value):
                            state = .doubling(value)
                    }
                default:
                    break
            }
            return .none
        }
    )

    let store = TestStore(
        initialState: .incrementing(1),
        reducer: reducer,
        environment: ()
    )

    store.assert(
        .send(.apply) { $0 = .incrementing(2) },
        .send(.apply) { $0 = .incrementing(3) },
        .send(.toggle) { $0 = .doubling(3) },
        .send(.apply) { $0 = .doubling(6) },
        .send(.apply) { $0 = .doubling(12) },
        .send(.apply) { $0 = .doubling(24) },
        .send(.toggle) { $0 = .incrementing(24) },
        .send(.apply) { $0 = .incrementing(25) }
    )
}

What do you guys think?

stephencelis commented 4 years ago

@fabfelici Looks great! What we've been experimenting with is a bit of a layer on top of this, which I've opened up a draft PR for here: https://github.com/pointfreeco/swift-composable-architecture/pull/82

It's still very much a work-in-progress exploration, but we're eager to get some feedback with some real world use.

stephencelis commented 4 years ago

We also just got a Swift forum that should be nice for discussions like this and allow us to keep GitHub issues focused on bugs. @slashmo since you started the discussion, would you like to move things there so that we can close this out?

slashmo commented 4 years ago

Awesome! 👍 Yes, I'll move it over there and close this issue afterwards.

slashmo commented 4 years ago

This has been moved to https://forums.swift.org/t/pullback-along-enum-state/36416