RFC: Require WithViewStore to specify observed view state

[stephencelis]

👋 Hi all!

We just opened #1339 and would love your feedback. It's a big change to a widely-used API, but we think a good one in the long run.

[tgrapperon]

I'm not a native speaker, but I would have rather leaned toward gerunds instead of imperative/infinitive forms:

WithViewStore(store, observing: ViewState.init, sending: ViewAction.init) {}

I didn't pull the branch to try but I'm wondering if \.self is working as a (State) -> State function when State: Equatable, or does it needs honest KeyPath arguments instead of functions for this to work?

[stephencelis]

I'm not a native speaker, but I would have rather leaned toward gerunds instead of imperative/infinitive forms:

WithViewStore(store, observing: ViewState.init, sending: ViewAction.init) {}

We sketched that out initially but we actually use get:send: in the binding APIs already, so observe:send: was not only a bit shorter but also consistent with other APIs.

It also reads kinda like English: "With a view store of store, observe X and send Y."

We're down to formalize how we do some of this naming, though!

I didn't pulled the branch to try but I'm wondering if \.self is working as a (State) -> State function when State: Equatable, or does it needs honest KeyPath arguments instead of functions for this to work?

Unfortunately not. This is a long-standing Swift bug.

[tgrapperon]

I personally feel that the trailing closure is creating a step that makes it different from a binding (which is completely defined after send). I'm reading:

"With a view store of store that is observing X and sending Y, do/make { … }"

[stephencelis]

One further complication with -ing is it'd clash with the existing removeDuplicates.

WithViewStore(
  self.store
  observing: \.view,
  sending: { .view($0) },
  removeDuplicates: isDuplicate
)

We could also change this to removingDuplicates, though this is a bit of a change from Apple's APIs (even if usage is different enough to justify).

[stephencelis]

We've gone ahead and merged the API as-is, but are open to more feedback before the next release, which will likely be soon, and also with use!

[jshier]

Looking at this a bit I think making it more ergonomic will make it more acceptable.

First, I feel like many users will see observe: { $0 } as boilerplate, not a prompt to optimize anything, especially when it requires the creation of an additional type that requires a custom initializer for easiest usage.

Second, this pattern is fairly painful if you only need a single property. Creating a whole type with a custom initializer to extract a single property feels fairly wasteful. I don't have any ideas yet, but some sort of convenience here would help.

Third, this optimization is really only relevant for views which are still loaded, right? For instance, if I have a login view that only needs a single property and single action while observing the whole application's state, but the view goes away once the user logs in, there's no harm in observing everything, right? If so this optimization doesn't seem generally applicable.

Fourth, it seems that, by defaulting to this behavior, it makes it somewhat harder to refactor screens and flows, as you need to be very particular about the state you prune and changing that state leads to more extensive refactor than it seems like simply accessing an additional property on a screen should. Especially if there are several layers that the state must travel through.

So while this may be worthwhile as an optimization point, I'm not sure it makes a good default. Perhaps some guidance around how to figure out you need this optimization might be a good starting point?

[stephencelis]

@jshier Thanks for taking the time to provide some feedback!

Looking at this a bit I think making it more ergonomic will make it more acceptable.

Definitely open to ideas and suggestions here!

First, I feel like many users will see observe: { $0 } as boilerplate, not a prompt to optimize anything, especially when it requires the creation of an additional type that requires a custom initializer for easiest usage.

In the three years of productions use TCA has seen, observing too much state is probably the biggest gotcha folks encounter, so we definitely think a change is needed here. We're no fans of boilerplate that can be avoided, but we think the trade-off is worth it here, especially when it's not super heavyweight boilerplate.

By making state observation explicit and providing a required parameter, we hope to surface the behavior of WithViewStore in a more front-and-center way. And we think folks will still encounter the issue, but it'll be much easier for them to find if they have any observe: { $0 }s at the root of their application, and explain why this can cause performance issues.

Second, this pattern is fairly painful if you only need a single property. Creating a whole type with a custom initializer to extract a single property feels fairly wasteful. I don't have any ideas yet, but some sort of convenience here would help.

Can you explain why you think it's painful for a single property? For a single property you can pluck the value out directly with a transform function, no extra struct needed:

WithViewStore(self.store, observe: \.count) { viewStore in
  // ...
}

If it's unlikely this view will need additional view state in the future, you can stop here, no additional lines of code.

You can technically even avoid view state structs by plucking out tuples of properties, but until Swift finally ships equatable tuples, you will have to pass the == sign along:

WithViewStore(
  self.store,
  observe: { (count: $0.count, fact: $0.fact) },
  removeDuplicates: ==
) { viewStore in
  // ...
}

At this point, though, we personally find the returns to be diminishing, and a ViewState struct to be appropriate.

Third, this optimization is really only relevant for views which are still loaded, right? For instance, if I have a login view that only needs a single property and single action while observing the whole application's state, but the view goes away once the user logs in, there's no harm in observing everything, right? If so this optimization doesn't seem generally applicable.

This optimization is relevant for really all non-leaf TCA features in your application. This includes all parent domains that contain child domains, e.g. those that present child sheets, navigation stacks that are drilled many layers deep, etc.

Accidental observation of unrelated state in these parent nodes can cause performance issues and even buggy behavior in navigation stacks as SwiftUI mistakenly pops/pushes screens during mutations, etc.

Fourth, it seems that, by defaulting to this behavior, it makes it somewhat harder to refactor screens and flows, as you need to be very particular about the state you prune and changing that state leads to more extensive refactor than it seems like simply accessing an additional property on a screen should. Especially if there are several layers that the state must travel through.

For what it's worth we are not suggesting users immediately make ViewState struct before building out views. You are free to get things working with { $0 }, and then only if/when you encounter performance issues and navigation bugs, you can identify where too much state is being listened to and refactor.

The one nice thing about ViewState refactors are they're pretty mechanical:

1. You add a ViewState struct that might be empty:

   struct ViewState: Equatable {
     init(state: Feature.State) {
     }
   }

2. You pass the initializer to WithViewStore:

   -WithViewStore(self.store, observe: { $0 }) { viewStore in
   +WithViewStore(self.store, observe: ViewState.init) { viewStore in
      // ...
    }

3. You identify each property being accessed as a compiler error, and map it to a ViewState property that is assigned in the initializer.

So while this may be worthwhile as an optimization point, I'm not sure it makes a good default. Perhaps some guidance around how to figure out you need this optimization might be a good starting point?

Again, we are definitely open to other ideas. Do you have any concrete (or even non-concrete) ideas you'd like to share?

At the end of the day nothing will prevent you from defining helpers that hide this default { $0 } away in your application, we just no longer think that it's appropriate to ship such a tool by default anymore.

That said, we haven't hard-deprecated the existing APIs yet, so we have time to explore alternatives!

[jshier]

In the three years of productions use TCA has seen, observing too much state is probably the biggest gotcha folks encounter, so we definitely think a change is needed here. We're no fans of boilerplate that can be avoided, but we think the trade-off is worth it here, especially when it's not super heavyweight boilerplate.

By making state observation explicit and providing a required parameter, we hope to surface the behavior of WithViewStore in a more front-and-center way. And we think folks will still encounter the issue, but it'll be much easier for them to find if they have any observe: { $0 }s at the root of their application, and explain why this can cause performance issues.

Sure, I just don't think many users will care since they don't see the benefit. Requiring this optimization just to use the framework is just another source of complexity until you actually see the issues you describe, which may be never. For those users this will be boilerplate they either remove with their own extension or gloss over, leaving { $0 } scattered throughout the codebase. This doesn't seem like a good way to introduce the framework nor a good way to leave a user's codebase.

And as an aside, as project maintainers you largely talk to users with problems, so of course they seem obvious and common.

Can you explain why you think it's painful for a single property?

I forgot you could just use viewStore.state 🥴. You do lose the readability of a real property name in that, but it's not hard to make. I can certainly see codebases requiring proper names though, and having to choose between creating a type for a single property or using a tuple with a label and having to manually provide == isn't great.

For what it's worth we are not suggesting users immediately make ViewState struct before building out views. You are free to get things working with { $0 }, and then only if/when you encounter performance issues and navigation bugs, you can identify where too much state is being listened to and refactor.

If it's not important right away and may never be important, that says to me it shouldn't be required by default.

The one nice thing about ViewState refactors are they're pretty mechanical:

That it's a simple change doesn't really change my point. Fundamentally this is the same issue you just got rid of by dropping Environment values and moving to @Dependency: adding even a single value could affect many use sites. Like Environment the updates are rote but repetitive and the ergonomic issue is the same.

Again, we are definitely open to other ideas. Do you have any concrete (or even non-concrete) ideas you'd like to share?

First, add docs and tools to help users identify when using these APIs makes sense given their ergonomic cost. Allow the users to optionally make their code more complex if they find value in the optimization. Don't require it by default.

Second, add additional APIs to make dealing with scoped state easier. Or perhaps just recommend different default patterns. Once ReducerProtocol is out and pullback is largely removed, state scoping will be the biggest remaining ergonomic issue for the library, as far as I can tell. Users have to learn about the Store / ViewStore separation, Store scoping which occurs alongside ViewStore usage, and with this change, slicing out ViewState for every view they use. There are a lot of questions that have to be answered when using view hierarchies with TCA and adding another question which must be answered just doesn't seem worthwhile to me.

As I understand it, to use TCA with a SwiftUI view and these changes you have to:

1. Take a Store of some kind. This could be the root store or an already scoped store.
2. Transform that store into a ViewStore in some way.
3. Create some representation of just the state needed by my view, and use that to create the view store. While you could just use { $0 } you have to create something.
4. If I have subviews, reusable views, or later flow, I may need to take my local Store and scope it again, repeating this process for virtually every larger view in my app.

Do we really need all of this? Could we trade some of this work for the required state slicing? Perhaps instead of scoping Stores everywhere we could just take a single reference to @AppStore var store in the view and slice it locally? If we're manually slicing state anyway, is ViewStore even really necessary anymore?

On a related note, how does this change work with .binding and BindableState. If I have this very simple view that modifies a single piece of state through a binding, how would that work:

private struct ExpenseTypeView: View {
    let store: StoreOf<NewExpenseFlow.InputScreen>

    var body: some View {
        WithViewStore(store) { viewStore in
            VStack(alignment: .leading) {
                Text("What are you submitting?")
                    .font(.mediumTitle)
                    .padding(.horizontal)
                ExclusiveChoiceView(selectedOption: viewStore.binding(\.$expenseType))
                    .padding(.horizontal)
                Spacer()
            }
        }
        .navigationTitle("Type")
        .navigationBarTitleDisplayMode(.inline)
    }
}

This is also one of the examples where it seem like explicit state slicing would really look like boilerplate.

[jshier]

That it's a simple change doesn't really change my point. Fundamentally this is the same issue you just got rid of by dropping Environment values and moving to @dependency: adding even a single value could affect many use sites. Like Environment the updates are rote but repetitive and the ergonomic issue is the same.

You can largely ignore my point here, as this is just local state that won't be passed much. The issue isn't gone, just much lower impact.

[mbrandonw]

Sure, I just don't think many users will care since they don't see the benefit.

Many users may not care, but at least now there is something public in the API whose docs are just one click away (though I now realize we still need to write those docs 😬. Edit: and now we have! #1368).

Previously there was no indication in the API that you had to think about this, and hence probably no reason to look at the docs.

Requiring this optimization just to use the framework is just another source of complexity until you actually see the issues you describe, which may be never.

Many/most of the people who bring up the issue we are trying to address are new to TCA and ran into the issue pretty quickly due to various bugs in SwiftUI. For example, if you ever wrap a NavigationView inside a WithViewStore, then you need to chisel away the state otherwise you will run into problems. This is even true of vanilla SwiftUI. Environment objects and NavigationView can easily lead to weird popping bugs. And unfortunately this seems true of NavigationStack too.

Also worth mentioning that in the past day this issue has come up yet again: #1366.

First, add docs and tools to help users identify when using these APIs makes sense given their ergonomic cost. Allow the users to optionally make their code more complex if they find value in the optimization. Don't require it by default.

What you describe here is actually been the status quo now for the past month. It has made our job a bit easier in that we can just point people to the performance when people bring up the issue, but also we're only two people. Maybe we need a group of ambassadors to help spread the word too.

Do we really need all of this? Could we trade some of this work for the required state slicing? Perhaps instead of scoping Stores everywhere we could just take a single reference to @appstore var store in the view and slice it locally? If we're manually slicing state anyway, is ViewStore even really necessary anymore?

We would love to simplify this stuff, and we've spent quite a bit of time already with no success yet. As with a bunch of evolutions to TCA over the years (including ReducerProtocol), it is something we will keep chipping away at (with the help of suggestions from the community) until eventually the solution reveals itself to us. But we haven't cracked it yet.

Some of your ideas could work for certain types of applications. For example, the idea of just having a root app store that every view immediately has access to and does the feature slicing itself. This could work perfectly fine, but it does preclude many powerful patterns.

For example, you can no longer modularize since every view needs access to the full domain. You can also no longer use optionals, enums and identified arrays as state modeling tools because then that pushes the burden of complex scoping and IfLetStore'ing to the child view. You would essentially need to hold all of state as a big, flat struct.

On a related note, how does this change work with .binding and BindableState. If I have this very simple view that modifies a single piece of state through a binding, how would that work:

The code snippet you posted would continue working the exact same with the API, you would just have an additional observe: { $0 } in the WithViewStore. But it is true that BindableState doesn't place nicely with the concept of view state, and so perhaps by pushing people towards view state we are exacerbating that problem.

We do have ideas to fix this, and so maybe we need to prioritize if we are pushing people into this new WithViewStore(_:observe:) style.

…as this is just local state that won't be passed much. The issue isn't gone, just much lower impact.

I believe all ViewState structs can be private (or internal if you want to test them). I don't think views ever need to know about other views' ViewState.

[mbrandonw]

I forgot you could just use viewStore.state 🥴. You do lose the readability of a real property name in that, but it's not hard to make.

Coincidentally there is now an evo proposal to lift the 1-tuple restriction, so this may not be an issue in the future.

[rcarver]

I think it's a good direction, but thinking about the problem of usability while bringing the API in line with other recent updates to TCA, here's a sketch of a different direction.

ObserveStore(store, state: \.isVisible) { state, send in 
  if state.isVisible {
    Text("Hello World")
      .onAppear { send(.viewAppeared) }
  }
}

Rationale being:

1. Lead with what's happening: 'observe'
2. Observing what? 'state'. Excluding this arg would more communicate that the entire store is being observed.
3. Further separating ViewStore into two args, 1) the observed state 2) send that's similar to the .run Effect.

[Updated] Also drawing inspiration from the _Observe reducer discussed here

ObserveStore(store) // observe the whole store
ObserveStore(store, state: \.someField) // observe a single field
ObserveStore(store, state: ViewState.init) // construct a view state

Rationale (3) being fairly separate from the rest but I'd want to rename closure arg anyway?

ObserveStore(store) { viewStore in } // confusing: ObserveStore gives me a ViewStore? 
ObserveStore(store) { observedStore in } // long and awkward

Special consideration for stateless stores?

ObserveStore(store.stateless) { send in }
ObservedStore(store, state: ()) { send in }

I'm sure this raises plenty of questions and problems, just thought I'd throw the idea out there!

[mbrandonw]

Hi @rcarver, thanks for the feedback!

It's definitely interesting to put the "observe" part more upfront, and I personally would love to do that, but the tricky part is that the thing you want to get in the trailing closure is definitely a viewStore. There is more that you can do with a view store than just sending actions. You can derive bindings, you can yield while a predicate holds true on a piece of state, and you can derive a publisher, which may be handy when used in conjunction with SwiftUI's onReceive.

So, I think if we do put "observe" more upfront, the naming must play nicely with the fact that you will get a view store inside the closure.

But, having said that, I'm not sure that it's a showstopper to have a syntax like:

ObserveStore(self.store) { viewStore in 
  …
}

This is all still theoretical, and I haven't thought too much about the implications or even discussed it with Stephen, but thanks for bringing it up. 🙂

[rcarver]

All good points! I knew I was forgetting plenty of use cases. But, the reason I forgot them is because 99% of my WithViewStore simply reads state and/or sends actions. Maybe it's worth two variations?

// If you simply need state and actions
ObserveStore(store, state: \.isVisible) { state, send in }

// If you need the full power of a ViewStore
WithViewStore(store, observe: \.isVisible) { viewStore in }

[tgrapperon]

As an aside, please note that you can build this API out of an aptly named function in some View extension, without having to define a new type:

extension View {
  @MainActor
  public func ObserveStore<State, Action, ViewState: Equatable, ViewAction, Content: View>(
    store: Store<State, Action>,
    observe toViewState: @escaping (State) -> ViewState,
    send fromViewAction: @escaping (ViewAction) -> Action,
    @ViewBuilder content: @escaping (ViewState, (ViewAction) -> Void) -> Content
  ) -> some View {
    WithViewStore(store, observe: toViewState, send: fromViewAction) { viewStore in
      let state = viewStore.state
      let send: (ViewAction) -> Void = { viewStore.send($0) }
      content(state, send)
    }
  }
  @MainActor
  public func ObserveStore<State, Action, ViewState: Equatable, Content: View>(
    store: Store<State, Action>,
    observe toViewState: @escaping (State) -> ViewState,
    @ViewBuilder content: @escaping (ViewState, (Action) -> Void) -> Content
  ) -> some View {
    WithViewStore(store, observe: toViewState) { viewStore in
      let state = viewStore.state
      let send: (Action) -> Void = { viewStore.send($0) }
      content(state, send)
    }
  }
}