swift-composable-architecture icon indicating copy to clipboard operation
swift-composable-architecture copied to clipboard

Property wrapper for shared state

Open hibernat opened this issue 3 years ago • 6 comments
trafficstars

The code in this PR is just partially documented. Purpose of this PR is: What do you think about that?

Managing shared state properties across more states is quite frequent, and should have native support by the Composable Architecture. This PR contains new property wrapper that keeps the property mappings between states, so the shared state is much easier to handle and maintain:

Before:

var profile: ProfileState {
    get {
      ProfileState(
        currentTab: self.currentTab,
        count: self.counter.count,
        maxCount: self.counter.maxCount,
        minCount: self.counter.minCount,
        numberOfCounts: self.counter.numberOfCounts
      )
    }
    set {
      self.currentTab = newValue.currentTab
      self.counter.count = newValue.count
      self.counter.maxCount = newValue.maxCount
      self.counter.minCount = newValue.minCount
      self.counter.numberOfCounts = newValue.numberOfCounts
    }
  }

Using the property wrapper:

@StateToDerivedStatePropertyMapping(
        (\SharedState.currentTab, \ProfileState.currentTab),
        (\.counter.count, \.count),
        (\.counter.maxCount, \.maxCount),
        (\.counter.minCount, \.minCount),
        (\.counter.numberOfCounts, \.numberOfCounts)
    ) var profileStateMapping
    var profile: ProfileState {
        get { profileStateMapping.derivedState(self) }
        set { profileStateMapping.mutate(&self, newValue) }
    }
struct ProfileState: Equatable, DerivedState {
    init(by valueForKeyPath: [PartialKeyPath<Self> : Any]) {
        currentTab = valueForKeyPath[\.currentTab] as! SharedState.Tab
        count = valueForKeyPath[\.count] as! Int
        maxCount = valueForKeyPath[\.maxCount] as! Int
        minCount = valueForKeyPath[\.minCount] as! Int
        numberOfCounts = valueForKeyPath[\.numberOfCounts] as! Int
    }

The newly proposed way is not shorter, but the code is more maintainable:

  • The derived state should have no default property values, as it is always newly created by the parent state. State conformace to the DerivedState protocol makes clear that the state is dependent on some other state, and the required initializer allows customization, if needed
  • Property mappings are just parameters to the property wrapper
  • Supports optional derived state

Final notes:

  • The code supports up to 16 properties that can mapped between states. I just cannot write it better while keeping type safety, without "cheating" the WritableKeyPath.
  • I also expect that the state is a value type, classes needs another (modified) property wrapper.
  • There is a new example within the SwiftUICaseStudies

Happy New Year!

hibernat avatar Dec 27 '21 17:12 hibernat

Hi @hibernat,

Thanks for putting together this PR. I do like the idea of using key paths to make managing shared state a bit easier, however, there are a few things I'm unsure about with this solution:

  • Sometimes my derived state doesn't have a 1 to 1 mapping.
  • I would typically be deriving state from other modules so the state isn't always initialized by a parent.
  • Initializing state by force unwrapping dictionary values could lead to run time crashes.
  • The property wrapper doesn't ensure all properties are mapped.
  • I find the computed property approach easier to understand and is likely going to perform better.

Your property wrapper could also be useful in projects not using TCA, so it might work better as a standalone library.

Anyway just wanted to mention a few things.

Happy New Year too!

iampatbrown avatar Dec 30 '21 22:12 iampatbrown

Hi @iampatbrown,

Thanks for your response, please, help me to understand you comments:

  • Sometimes my derived state doesn't have a 1 to 1 mapping. - Do you mean a case when some property is being computed from more other (source) properties? Or any other case? If the local (derived) state has its own properties, then these have to be anyhow "mapped" to some properties in its parent state, because the computed local state is always newly initialized on any get. Or, do I miss anything?
  • I would typically be deriving state from other modules so the state isn't always initialized by a parent. - This is something that I (probably) do not understand fully. Local state (derived state) should always have its "source of truth" state, and I do not see any other option than state that is a "parent" of the local state. The local (derived) state is always newly created in the parent's getter of the derived property, and I see no option how to access a property that is not accessible by the parent.

Happy New New Year!!!

hibernat avatar Dec 31 '21 08:12 hibernat

Sometimes my derived state doesn't have a 1 to 1 mapping.

As in, sometimes I don't set all the state from the child state back to the parent. There is an example of this in the ReusableFavoriting case study: https://github.com/pointfreeco/swift-composable-architecture/blob/313dd217dcd1d0478118ec5d15225fd473c1564a/Examples/CaseStudies/SwiftUICaseStudies/04-HigherOrderReducers-ReusableFavoriting.swift#L114-L117

It can also get more complicated than this when working with subsets of the parent state, eg. a filtered collection.

I would typically be deriving state from other modules so the state isn't always initialized by a parent.

I mean the state must be able to be initialized by itself without knowing about its parent. The module would work in isolation then get combined with other modules/features to make larger features. The SharedState case study could be broken up into 3 modules:

  • CounterFeature
  • ProfileFeature
  • AppFeature

The CounterFeature and ProfileFeature would would work without any knowledge of the AppFeature. So making ProfileState conform to DerivedState kind of breaks this modulariztion.

iampatbrown avatar Jan 03 '22 21:01 iampatbrown

I'm also curious about the performance of such derived states with deeply nested collections of complex substates 🤔

I've been using something like that for current element access, but not sure about whole collections

struct DerivedState {
  var value: Int
}

struct ParentState {
  var items: [DerivedState]
  var currentIndex: Int?
  
  var current: IndexedElementProvider<DerivedState, Int>? {
    get { IndexedElementProvider(for: self, \.items, at: \.currentIndex) }
    set { newValue.assign(to: \.items, at: \.currentIndex, on: &self) }
  }
}

// var state = ParentState()
// state.current?.element // DerivedState?
// state.current?.element.value = 1

public struct IndexedElementProvider<Element, Index> {
  public static func element(_ element: Element, at index: Index) -> Self {
    .init(element: element, index: index)
  }

  public init(element: Element, index: Index) {
    self.element = element
    self.index = index
  }

  public var element: Element
  public let index: Index
}

extension IndexedElementProvider: Equatable where Element: Equatable, Index: Equatable {
  public static func == (lhs: IndexedElementProvider, rhs: IndexedElementProvider) -> Bool {
    lhs.element == rhs.element && lhs.index == rhs.index
  }
}

extension IndexedElementProvider {
  public init?<Parent, Collection: Swift.Collection>(
    for parent: Parent,
    _ collectionKeyPath: KeyPath<Parent, Collection>,
    at indexKeyPath: KeyPath<Parent, Index?>
  ) where Index == Collection.Index, Element == Collection.Element {
    let index: Index? = parent[keyPath: indexKeyPath]
    let element: Element? = index.map { index in
      let collection = parent[keyPath: collectionKeyPath]
      return collection[index]
    }
    if let _index = index, let _element = element {
      self = .init(element: _element, index: _index)
    } else {
      return nil
    }
  }

  public func assign<Parent, Collection: Swift.MutableCollection>(
    to collectionKeyPath: WritableKeyPath<Parent, Collection>,
    at indexKeyPath: KeyPath<Parent, Index?>,
    on parent: inout Parent
  ) where Index == Collection.Index, Element == Collection.Element {
    guard let index = parent[keyPath: indexKeyPath] else { return }
    parent[keyPath: collectionKeyPath][index] = element
  }

  public init<Parent, Collection: Swift.Collection>(
    for parent: Parent,
    _ collectionKeyPath: KeyPath<Parent, Collection>,
    at indexKeyPath: KeyPath<Parent, Index>
  ) where Index == Collection.Index, Element == Collection.Element {
    let index: Index = parent[keyPath: indexKeyPath]
    let collection = parent[keyPath: collectionKeyPath]
    let element = collection[index]

    self.index = index
    self.element = element
  }

  public func assign<Parent, Collection: Swift.MutableCollection>(
    to collectionKeyPath: WritableKeyPath<Parent, Collection>,
    at indexKeyPath: KeyPath<Parent, Index>,
    on parent: inout Parent
  ) where Index == Collection.Index, Element == Collection.Element {
    let index = parent[keyPath: indexKeyPath]
    parent[keyPath: collectionKeyPath][index] = element
  }
}

maximkrouk avatar Jan 05 '22 10:01 maximkrouk

Hi @hibernat! Thanks for taking the time to PR and share. We're just getting around to looking this stuff over from the holidays. Happy new year to you, too! :smile:

I think this is a really fun exploration of how one might approach derived shared state. I always love to see how folks are using some of these advanced Swift features, like property wrappers and key paths, and you've employed them in interesting ways!

@iampatbrown has done a good job voicing the concerns I have with this approach over the more manual approach we show in the case studies, especially when it comes to modularity and the safety of the dictionary initializer. While we are always open to shipping a feature that would fully improve upon the manual approach, I think we'd want such a solution to address those concerns.

Pat also mentioned the potential ability to ship this as a library (that would work even outside TCA), so that's definitely an idea if you've found this approach useful in your applications and would like to give others an opportunity to explore, as well! One thing we'd really like to do in the future is provide a better way of sharing these kinds of explorations with the community, so @mbrandonw and I will be discussing some ideas soon.

stephencelis avatar Jan 05 '22 22:01 stephencelis

Hi @stephencelis , thanks for your response! Glad to read it. FYI: @iampatbrown (Thanks for your time spent on response above!)

  • I agree with your comments, my proposed solution is not an ideal one, and I know it, and I knew it when I submitted the PR.
  • I like TCA, however handling shared state is not ideal, and I still think how to make it easier, at least for me
  • These are my concerns:
  1. Any state can use default values for its properties - this is troubled when the state is always initialized by the computed property of the parent state. Specially dangerous, when someone is not aware that such a state is a derived state, and modifies the state by adding a new property with default value. Nothing stops him/her to do so, nothing warns him/her to add the property also to the parent state. I do not think that the required initializer with dictionary with forced unwrapping is so bad idea. Main purpose was to have crashes in a moment when developer makes any change that affects deriving the state, but he/she is not aware of that. (I also do not say that that is a great idea...)
  2. Creating derived state manually by computed property is something that makes the code harder to read/understand, with too many manual effort. The fact that I cannot do it significantly better does not change by meaning about that :)
  3. Performance of the mapper is definitely lower than in case of manual computed property, but for most of cases it should be neglectable, I mean.
  4. I have not faced any case of derived state combined with indexed collection yet, and, as I can see from the example above @maximkrouk , this is something that makes all this topic even more complicated.

I have not abandoned this topic, I just have to do also some other work...

hibernat avatar Jan 13 '22 20:01 hibernat

Hi @hibernat, we are going to close this without merging. Since this functionality can be defined outside the library it is perfect for packaging up as an external library. Also, the new dependency management system that ships with 0.41 can solve some of these same problems.

If you end up exploring this idea more please feel free to open a discussion to share!

mbrandonw avatar Oct 11 '22 18:10 mbrandonw