Struggling with lack of encapsulation and APIs

[kanevP]

Hi all, I've been using TCA on my personal projects since the early episodes and and successfully pushing for it at work and I feel like the code produced might be the most sophisticated I have ever written. Sometimes I catch myself thinking "I can't believe I'm being paid to write this", but there's something I'm concerned about myself and worried how I'd defend if brought up by colleagues that don't share my excitement of the library:

• Encapsulation (separating private and public states) - vars of states feel exposed for higher level reducers to mutate
• APIs of components to notify concerned parties of meaningful events

I'll share how approximately I'd write a View Model of a Dropdown view with comments describing my struggle to find alternatives for good things about our View Model pattern:

// The protocol is internal and describes
// the interface that will be exposed to the view
// This I find an alternative to by using `ViewState`s & `ViewStore`
protocol DropDownViewModelType: ObservableObject {
  associatedtype Item
  var isLoading: Bool { get }
  var items: [Item] { get }
  var selectedItem: Item? { get }
  
  func viewDidAppear()
  func didSelect(item: Item)
}

@available(iOS 13.0, *)
public class DropDownViewModel<Item>: ObservableObject {
  
  public struct Environment {
    let retrieve: () -> AnyPublisher<[Item], Never>
    // let's say an item selection can be rejected
    let itemIsSelectable: (Item) -> Bool
  }
  
  // MARK: -  Public API
  
  // ⚠️ Events of interest to others
  // Notice that we can't make the mistake of
  // subscribing to the selection attempt that would be rejected
  // by the `itemIsSelectable` method if the `Environment`
  // where in TCA we'd have something like:
  // enum Actions<Item> {
  //   case didAppear
  //   case didSelect(Item) // ❌ reacting to this action would be an error
  //   case itemSelectionDidPassValidation(Item) // ✅ this is the correct action to react to
  //   ...
  //
  // Additionally the `itemSelectionDidPassValidation` is not needed for the
  // reducer to be functional. It'd look like this:
  // ...
  // case let didSelect(item):
  //   guard environment.itemIsSelectable(item) else { return .none }
  //   state.selectedItem = item
  //   ...
  // But instead we need to declare a `case itemSelectionDidPassValidation` only to
  // notify concerned parties of the event.
  public var didSelect: AnyPublisher<Item, Never> {
    mutableDidSelect.eraseToAnyPublisher()
  }
  
  // MARK: - DropDownViewModelType vars
  // ⚠️ No mutable vars
  // Protocol conformance vars are immutable from outside
  // that way nobody can accidentally break the internal logic
  // like if you toggle `isLoading` to `true` the retrieval
  // will never kick off (see the `viewDidAppear` method)
  @Published private(set) var isLoading: Bool = false
  @Published private(set) var items: [Item] = []
  // ⚠️ Notice that we cannot assign an item from outside
  // and circumvent the `itemIsSelectable` described in the `Environment`
  @Published private(set) var selectedItem: Item? = nil
  
  // MARK: - Other
  private let environment: Environment
  private let mutableDidSelect = PassthroughSubject<Item, Never>()
  private var cancelables: Set<AnyCancellable> = []
  
  public init(environment: DropDownViewModel<Item>.Environment) {
    self.environment = environment
  }
  
  // MARK: - DropDownViewModelType methods
  func viewDidAppear() {
    guard !isLoading, items.isEmpty else {
      return
    }
    
    isLoading = true
    environment.retrieve()
      .sink(receiveValue: { [weak self] items in
        self?.isLoading = false
        self?.items = items
      })
      .store(in: &cancelables)
  }
  
  func didSelect(item: Item) {
    guard !isLoading, environment.itemIsSelectable(item) else {
      return
    }
    mutableDidSelect.send(item)
    selectedItem = item
  }
}

Any help/input is highly appreciated!

[iampatbrown]

Any chance you could include a view with this example as well? I was starting to test it out to better understand your question and I wasn't exactly sure how the selection logic is implemented form a view standpoint.

I think your initial intuition is right regarding ViewState and ViewStore. The protocol doesn't necessary ensure conforming classes will use @Published or rely on environment.itemIsSelectable(item) though. Also, is var didSelect: AnyPublisher<Item, Never> meant to be part of the protocol? and who are the concerned parties in this scenario? didSelect looks like it would be similar to using something like viewStore.publisher.selectedItem

I think some of the main things are going to be testability and composition.

How would your concerns be phrased if they were in the form of a question from your colleagues? This will help me provide more targeted input :)

[iampatbrown]

Thanks for this. I think part of the issue is because there is another layer with SandwichFormViewModel compared to the SandwichForm.reducer. The reducer is explicitly being given access to those values. Where in the ViewModel example they are explicitly being restricted access.

There would be ways to achieve this with TCA as well. Depending on what your needs are it will change. A example could look something like this (this is just to demonstrate what I mean, I'm not suggesting you actually do this :P)

 enum SandwichForm {
  ...
  struct State: Equatable {
    var proteinDropdownState = DropDown<Proteins>.State(items: Proteins.allCases)
    var veggiesDropdownState = DropDown<Veggies>.State(items: Veggies.allCases)
    var stateless: Void { get {} set {} } // You would make this the state you want to allow access to
  }
  ...
  // This reducer only has access to the state you allow it to, you could change the actions etc as well
  static let formReducer = Reducer<Void, Actions, Environment> { state, action, environment in
    switch action {
    case .didAppear:
      // state....
      return .none
    default:
      return .none
    }
  }

  static var reducer: Reducer<State, Actions, Environment> {
    .combine(
      DropDown<Proteins>.reducer.pullback(...),
      DropDown<Veggies>.reducer.pullback(...),
      formReducer.pullback(
        state: \.stateless,
        action: /Actions.self,
        environment: { $0 }
      )
    )
  }
}

I'm not exactly sure what logic the form is responsible for but this is similar to a higher order reducer.

Also the VMs inside SandwichFormViewModel probably don't need to be @Published

[surajbarkale]

⚠️ In the VM version of this form I can't break the internal mechanism of the dropdown
with this code even if I was trying

You can declare fields as fileprivate(set) (assuming you have different files for DropDown and SandwichForm) to get the encapsulation at minimal expense. All you have to do is make sure that the state and reducer are in the same file.

For encapsulation of actions, you can separate ViewAction as you indicated below. However, this will be a weaker form of encapsulation.

[iampatbrown]

@surajbarkale great point. If you needed, you could probably make the reducer a static on State and use private(set). eg.

enum DropDown<Item: Hashable> {
  struct State: Equatable {
    private(set) var isLoading: Bool = false
    private(set) var items: [Item]
    private(set) var selectedItem: Item?

    static var reducer: Reducer<State, Actions, Environment> {
      return .init { state, action, environment in
        ...
        state.isLoading = false
        ...
        return .none
        }
      }
    }
  }
  enum Actions: Equatable { ... }
  public struct Environment { ... }
  static var reducer: Reducer<State, Actions, Environment> { State.reducer }
}

Not something I've ever done before... but I'm guessing it would work...

[kanevP]

@surajbarkale, not going to lie, I kind of facepalmed for "Why didn't I think about that sooner" when I read your suggestion about utilising fileprivate(set), but that would break testability through the TestStore, where you need to mutate those values. This reminded me of something else that feels off. As a user (let's say I'm building the form) of the Dropdown component (let's say I didn't write that), I need to know its whole internal machinery to write a full unit test for the form (what state mutates on every dropdown action). Now scale that to complex form that has dropdown, text fields, and all kinds of other fields. You have to know how all of the form fields work so you can unit test what happens on an action.

Once I had to write a TCA textfield that worked with a lot of custom logic: attributed strings, characters that were not allowed, custom cursor positioning and having just one of those in a form made it so noisy to test (the form) I just skipped those scenarios. That was for a component that I had written the day before so the internal logic was still in my head, I can't imagine my frustration for somebody else's component that I'm not familiar with.

[surajbarkale]

Rant

In my personal experience, module level encapsulation is the right choice for almost all scenarios. All languages (even C) have support for it. When using SPM, cost of creating a separate module is so low that I believe this is the right way to go. There should not be any secrets for the code within the module. Just had to get that off my chest :)

Well we can solve it by adding even more code :) If you still wish to use private fields, you can create a method to modify these fields wrapped in #if DEBUG. Similar to how TestStore is defined.

If you really need to make the actions private, just wrap them in a struct:

public enum Action {
  internal enum View {
    case didSelectItem
  }
  public struct Internal {
    internal let action: View  
  }

  case  itemSelected
  case internal(Internal)
}

For the consumers of the component, Action.Internal is opaque since there are no public fields. When writing the reducer, you have to probably nest the switch statement for Action.View.

I am not sure if this is what you were looking for.

[surajbarkale]

In TCA all actions are effectively public. This is required to ensure that all actions are handled in the reducer switch statement. So always treat the Action enum as the public API of your component. Which means naming the actions in relation to the domain instead of in relation to the view.

I would change the Actions<Item> enum as follows:

enum DropdownAction<Item: Equatable> {
  case loadItems // previously didAppear
  case itemsLoaded([Item])
  case validateSelection(Item) // previously didSelect(Item)
  case didSelect(Item) // previously itemSelectionDidPassValidation(Item)
}

[kanevP]

@surajbarkale thank you for you reply, to me didSelect(Item) looks like an action that's very inviting to be send from inside the view, which would be a bug (validateSelection(Item) should be sent instead) that won't even be caught by unit tests.
I was thinking of splitting the actions like this:

  enum ViewAction: Equatable {
    case didAppear
    case viewDidSelectItem(Item)
  }
  
  enum Actions: Equatable {
    case viewAction(ViewAction)
    case itemSelectionDidPassValidation(Item)
    case retrievalResult([Item])
  }
 

Which looks like solves the API concerns, but my encapsulation ones still stand.