ForEachReducer support for dictionary-based collections

[heyltsjay]

_ForEachReducer drops support for dictionary-based collections, which was unexpected and not documented in the migration guide. Should either support or call out the design decision to move away from it.

Our app (UIKit) currently models forms using dictionaries, mostly to help coalesce actions that come in:

struct State {
  var form: [FormFieldID: FormField.State] = [
     .email: .init(),
     .password: .init(),
  ]
}

enum Action {
  case form(id: FormFieldID, action: FormField.Action)
}

[heyltsjay]

If it's just an oversight, migrators can use this for now:

    @inlinable
    func forEach<Element: ReducerProtocol, Key>(
      _ toDictionaryState: WritableKeyPath<State, [Key: Element.State]>,
      action toKeyedAction: CasePath<Action, (Key, Element.Action)>,
      @ReducerBuilderOf<Element> _ element: @escaping () -> Element,
      file: StaticString = #file,
      fileID: StaticString = #fileID,
      line: UInt = #line
    ) -> CombineReducers<ReducerBuilder<Self.State, Self.Action>._Sequence<Reduce<Self.State, Self.Action>, Self>> {
        CombineReducers {
            Reduce { state, action in
                AnyReducer {
                    element()
                }
                .forEach(
                    state: toDictionaryState,
                    action: toKeyedAction,
                    environment: { _ in },
                    file: file,
                    fileID: fileID,
                    line: line
                )
                .callAsFunction(&state, action, ())
            }
            self
        }
    }

[stephencelis]

Thanks for bringing this up @heyltsjay! I think like the other operators, we want to take some time to re-envision the use of this one, which we haven't used ourselves in practice.

The main a few things that come to mind:

• We should flip it to be defined in the new reducer builder way (as you do in your migration sketch, above).
• I think we should probably consider requiring OrderedDictionary. Right now if you try to render a Dictionary to SwiftUI view state, you need to somehow loop over the dictionary to render it, you need to order the keys in some way to avoid getting randomness on recomputation.
• I wonder if we should also have a ForEachStore that is compatible with OrderedDictionary.

Does anyone out there have some real world experience using Reducer.forEach with dictionary state? Would love for some examples (and maybe a case study) to work with to help drive the design here.

[tgrapperon]

Here is a quick experiment with completely generic Reducer.ForEach and ForEachStore. It doesn't seems to cause additional strain on compiler.

As I'm getting it, any type conforming to the IdentifiedStatesCollection protocol could work. I've provided conformance for IdentifiedArray and OrderedDictionary, and it should also work with unstructured or lazier collections. This is a very fast proof of concept, and this protocol could likely be refined.

What do you think?

[tgrapperon]

As a byproduct of the generalization, one can ForEach reducers and stores using different IdentifiedStatesCollection on the same set of data. It means that one can non destructively and efficiently filter or sort for the view layer while preserving the existing states for example.

[iampatbrown]

@tgrapperon That looks like it could work. I'd probably require IDs be an OrderedSet and remove areIdentifiersEqual.

I'm hesitant about introducing a public protocol for this though. While the flexibility of using custom collections with ForEachStore is appealing, I'm concerned it would add constraints to the implementation.

I think adding support for OrderedDictionary would cover the majority of use cases.

[iampatbrown]

Something like this:

public protocol IdentifiedCollection {
  associatedtype ID: Hashable
  associatedtype Value

  var ids: OrderedSet<ID> { get }
  subscript(id id: ID) -> Value? { get set }
}

extension IdentifiedArray: IdentifiedCollection {
  public typealias Value = Element
}

extension OrderedDictionary: IdentifiedCollection {
  public typealias ID = Key

  public var ids: OrderedSet<ID> { keys }
  public subscript(id id: ID) -> Value? {
    _read { yield self[id] }
    _modify { yield &self[id] }
  }
}

Then in ForEachStore you might be able to use:

 let elements = store.state.value
 self.data = elements.ids.lazy.map { elements[id: $0] }

Does self.data actually get used much?

[tgrapperon]

Does self.data actually get used much?

It seems never! At least the one from ForEachStore. I've tried to delete/move/insert and it is never called. I even tried to fatalError() in a computed property, and it never touches it.

Furthermore, it seems there is an issue with the current implementation, as we capture store.state.value in init. If the store instance doesn't change but the number of views or content changes without invalidating the parent of ForEachStore, this value won't be updated.

It doesn't seem it affects edit operations.

I've managed to make TODO crash with a drop, but I suspect it is a vanilla problem. I'll try to reproduce the issue.

At worse, we can promote the ViewStore to @ObservedObject and decide that Data == IDs. This is not optimal, but it will already be better than the uncontrolled collection that is returned today. Given that content is escaping, if ForEachStore is evaluated, it will trickle down to ForEach at least, so it shouldn't impact performance. There is nothing in the doc that describes what this type represents, so it should probably work as fine with IDs that we're already keeping up-to-date anyway.

About the protocol thing, you're probably right. I'm still pushing with my LazyForEach to check if looser constraints are required, but I think it could work even with OrderedSet being fixed. I didn't use the name IdentifiedCollection as I thought it would collide with swift-identified-collections (and I don't think this protocol has its place there neither).

I don't think one can easily make the protocol internal, and there are only 2 conforming types, so maybe the safest would be to to parametrize with functions like you suggested. It that can be upgraded later to a protocol if required.

[iampatbrown]

I've managed to make TODO crash with a drop, but I suspect it is a vanilla problem. I'll try to reproduce the issue.

Interesting... let me know if you figure out how to reproduce it.

At worse, we can promote the ViewStore to @ObservedObject and decide that Data == IDs

Could probably also do something like:

public var data: Data { state.value }
let state: CurrentValueSubject<Data, Never>

I'm still pushing with my LazyForEach

Which is understandable :) Looks like it would be a powerful tool. Maybe LazyForEachStore could be an option.

I didn't use the name IdentifiedCollection as I thought it would collide with swift-identified-collections (and I don't think this protocol has its place there neither).

Yeah it isn't quite an IdentifiedCollection however I'd be leaning more towards a protocol in swift-identified-collections than here.

maybe the safest would be to to parametrize with functions like you suggested. It that can be upgraded later to a protocol if required.

Yeah that makes sense.

[tgrapperon]

Interesting... let me know if you figure out how to reproduce it.

Yes, this is Vanilla and it happens when you insert something in a List that comes from another section from the List (not a move intra-section). Not a TCA problem, and not reproducible with the current Todo without adding some code.

About DynamicViewContent, after having played a little with it, I don't think this is a thing for us to implement. The best we should do is pass wrapped ones up. It seems that SwiftUI only looks at ForEach anyway, which is understandable from their standpoint.

About my LazyForEach, I think that an Update Reducer could maybe be defined to update the EachState before passing it to EachReducer, but I don't think it's possible with the current ReducerBuilder (I didn't check yet). Or it could be a parameter of ForEachReducer, as this is currently the one that can see both ParentState and EachState. This is maybe the simplest approach. LazyForEachStore would be natural too. ForEachStore is currently held back by its deprecated forms, so having a new type would allow to make it work more efficiently.

About IdentifiedCollection, I still don't know if protocol IdentifiedCollection makes sense in swift-indentified-collection. This is a vast topic, and the OrderedSet<ID> is maybe too restrictive too. For example, what would you do with a collection similar to this one if it'd become more exposed? (EDIT: or these ones)

But in this library, I still think that extracting efficiently an OrderedDictionary from IdentifiedArray makes sense:

extension OrderedDictionary {
  public init(_ identifiedArray: IdentifiedArray<Key, Value>) {
    self = identifiedArray._dictionary
  }
}

Being an IdentifiedArray plays no different role than OrderedDictionary in ForEachStore/Reducer, so it would be natural to define these types based on OrderedDictionary and provide IdentifiedArray initializers that would downcast to OrderedDictionary. The IdentifiedArray -> OrderedDictionary transform seems harmless as long as IdentifiedArray is backed by OrderedDictionary, which seems to be the case for the foreseeable future.

[iampatbrown]

About IdentifiedCollection, I still don't know if protocol IdentifiedCollection makes sense in swift-indentified-collection

Agreed.

I still think that extracting efficiently an OrderedDictionary from IdentifiedArray makes sense

Yeah, that sounds pretty good.

About DynamicViewContent, after having played a little with it, I don't think this is a thing for us to implement.

Something to look into for sure. I haven't explored DynamicViewContent much.

[tgrapperon]

Ha, I didn't tried to implement it, but I just realized that extracting an OrderedDictionary won't play well with the setter part in ForEachReducer. So your initial parametric solution seems to be the solution with the least side effects for ForEachReducer. But ForEachStore could be implemented with OrderedDictionary if one can initialize them from IdentifiedArray.
Yesterday again I had a case where OrderedDictionary variants would have allowed me to avoid wrapping a fixed array of things into IdentifiedArray<Int, Identified<Element>> and directly work with a simpler, and yet relatively safe, OrderedDictionary<Int, Element>, so I can definitely feel the appeal!

[heyltsjay]

Any chance we can try to get this into 1.0? I feel confident that an ordered dictionary is a nice data structure to model state. Leaves shouldn't necessarily have to carry their own ID or be aware of their usage context.

[kaishin]

I second this. We use identified array most of the time but there are cases where dictionaries make more sense.