-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Introduce WithViewStore.init(_:observe:send:...)
#1339
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Looks nice! I'm curious, what sort of performance problem is easier to introduce with the older initializers? |
@square-tomb We have some information written up about this in our performance article. In short, doing |
I guess I'm too late on this, but I have a hard time seeing the logic here. Surely the right way to use TCA is to scope your store before sending it to a child view? That means that when you do the right thing, you are punished with having to write a lot of // MARK: Content
struct Content: ReducerProtocol {
struct State: Equatable {
var profile: Profile.State
}
enum Action: Equatable {
case profile(Profile.Action)
}
func reduce(into state: inout State, action: Action) -> Effect<Action, Never> {
// ...
return .none
}
}
struct ContentView: View {
let store: StoreOf<Content>
var body: some View {
ProfileView(store: store.scope(state: \.profile, action: Content.Action.profile))
}
}
// MARK: Profile
enum Profile {
typealias Store = ComposableArchitecture.Store<State, Action>
struct State: Equatable {
var name: String
}
enum Action: Equatable {
case changeName(String)
}
}
struct ProfileView: View {
let store: Profile.Store
var body: some View {
WithViewStore(store, observe: { $0 }) { viewStore in
Button {
viewStore.send(.changeName("..."))
} label: {
Text(viewStore.name)
}
}
}
} |
@GreatApe Your If you haven't already, read the view store section of the performance article. It should answer your question. |
Sure, I understand that stores should be scoped before you make a viewstore for them, that's what I do in my example. And of course there are situations where you may want to scope down the store within the view itself, for example when you need to both pass down a scoped store to a child view and use another scope in your own body.* But there are also plenty cases where you don't need to do that, cases where you are following all the rules and being 100% efficient. So it's hard to understand that you would have to add
struct ContentView: View {
let store: StoreOf<Content>
var body: some View {
ProfileView(store: store.scope(state: \.profile, action: Content.Action.profile))
BodyView(store: store.scope(state: \.body, action: body))
}
} |
As an aside both scope operations (to form the child domain, or to form the |
What is the difference? Except that in the |
This isn't true, it cannot always be handled like that. As the performance article shows, if the root view of a tab-based application needs access to the WithViewStore(self.store, observe: { $0 }) { viewStore in
TabView(selection: viewStore.binding(state: \.selectedTab, send: AppAction.tabSelected)) {
…
}
} However, this is not optimal at all. It is observing all of app state just to pluck out a single field. Instead what should be done is the surrounding WithViewStore(self.store, observe: \.selectedTab) { viewStore in
TabView(selection: viewStore.binding(send: AppAction.tabSelected)) {
…
}
} In general, the state held in a |
So I guess there are cases where you can't avoid it. But again, I completely understand that stores should be scoped before making view stores, and that there are cases where you should use I'm just saying that since for child views you will typically do the scoping before passing it to the child, there are many situations where you don't need to do any further scoping. Two very common cases, off the top of my head:
So my point is that since there are plenty of situations where legitimately don't need to observe, it should not be a mandatory parameter. |
@GreatApe Can you quantify just how many features you think do need to further scope view state versus those that do not? Are there more leaf features than root/intermediate features? Is it a 50/50 split? The fact is this is a problem 100% of people will run into if they don't know to properly scope view state. It is by far the most common problem we help people with in all the various outlets (forums, discussions, twitter, slack), and even in just the month since this PR was opened we have responded to this exact problem on 3 occasions. You might be interested in the RFC discussion for this change here #1340. We discuss at length how pervasive this problem is and why we think some visibility in the API is a step in the right direction. |
I was just looking at my own app, it has 50-60 views. It's only really close to the root that I use extension Profile.State {
var viewState: ProfileView.ViewState { fatalError() }
}
struct ProfileView: View {
struct ViewState: Equatable {
let name: String
}
let store: Store<Profile.State, Profile.Action>
var body: some View {
WithViewStore(store, observe: \.viewState) { viewStore in
Text(viewStore.name)
ChildView(store: store.scope(state: \.child, action: Profile.Action.child))
}
}
} Close to the leafs I often jump to vanilla SwiftUI: struct JustAboveLeafView: View {
let store: Store<AboveLeaf.State, AboveLeaf.Action>
var body: some View {
WithViewStore(store) { viewStore in
LeafView1(name: viewStore.name)
LeafView2(age: viewStore.age)
LeafView3(sex: viewStore.sex)
}
}
} Of course this means that the body of Again, I'm definitely not arguing against the existence of |
I'm curious, in this example, if we had stuff in the TabViews that used state directly, would it be more efficient with several viewstores, or just to observe something that has both the WithViewStore(self.store, observe: \.viewState) { viewStore in
TabView(selection: viewStore.binding(send: AppAction.tabSelected)) {
Text(viewStore.string)
Rectangle
.fill(color)
ChildView(store: store.scope(...))
…
}
} |
@tgrapperon |
Let's break this down a bit more, as I don't think the statement "stores should be scoped before making view stores" is quite accurate. Instead:
As @tgrapperon said here these both uses the /// (1) A view in the A domain. It doesn't use any state, just passes the store along
/// to views of domain A and B
struct ContentA: View {
let store: Store<StateA, ActionA>
var body: some View {
// Another view in the A domain. We don't care what state it actually uses.
NestedContentA(store: store)
// A view the A domain. We alter it shape to B, but again, don't care what state it uses.
ContentB(store: store.scope(state: \.b, action: ActionA.b)
}
}
/// (2) Observing state using `WithViewStore`
struct NestedContentA: View {
let store: Store<StateA, ActionA>
var body: some View {
// Observing just the `name` field to render this view, then passing the whole store to
// another nested view of the A domain
WithViewStore(store, observe: \.name) { viewStore in
Text(viewStore.state)
MyLeafView(name: viewStore.state)
MoreNestedContentA(store: store)
}
}
}
Yes this is true. But, when I see an un-scoped ViewStore it's an indication that something may be less efficient than it could be
What do you mean by this? |
Not making it mandatory would be equivalent to the status quo before the change. No one would use
The library is meant for people beyond the Point-Free community who may have never watched a video. And so far history has shown that |
And to share more stats, isowords currently has 23 instances of creating a view store with a scope to view state, and 26 uses of constructing a view store un-scoped. So nearly 50/50. I wonder how other people's code bases fare. |
Calls to
|
In my WIP app, I'm currently at 132 |
This is what I meant. I am not and have never disputed this. I feel that I repeat that in every post.
By state I mean information passed in from the outside, ultimately coming from the root. Views are either system views or custom views. Both types can either use outside state or be stateless. We can ignore the stateless ones here. So now most (all?) views have one of these forms, focusing on stateful views: A. No stateful views in the body Examples: struct ViewA: View {
var body: some View {
Text("Hello world")
}
}
struct ViewB: View {
let store: Store<B.State, B.Action>
var body: some View {
WithViewStore(store) { viewStore in
Text(viewStore.title)
Text(viewStore.subtitle)
}
}
}
struct ViewC: View {
let store: Store<C.State, C.Action>
var body: some View {
VStack {
Text("Children:")
Child1(store.scope(state: \.child1, action: C.Action.child1))
Child2(store.scope(state: \.child2, action: C.Action.child2))
}
}
}
struct ViewD: View {
let store: Store<D.State, D.Action>
var body: some View {
WithViewStore(store, observe: \.viewStateD) { viewStore in
VStack {
Text(viewStore.title)
Text(viewStore.subtitle)
Child1(store.scope(state: \.child1, action: C.Action.child1))
Child2(store.scope(state: \.child2, action: C.Action.child2))
}
}
}
} It seems to me that D is the only case where we need to use struct ViewDasC: View {
let store: Store<D.State, D.Action>
var body: some View {
VStack {
Titles(store.actionless.scope(state: \.titles))
Child1(store.scope(state: \.child1, action: C.Action.child1))
Child2(store.scope(state: \.child2, action: C.Action.child2))
}
}
} Perhaps the rule is that you can't live without |
Your stats seem to strongly support my argument. Surely it's not nice to have 30% of your usages of |
Tangentially related. Is there any difference in rendering here: struct ViewB1: View {
let store: Store<B.State, B.Action>
var body: some View {
WithViewStore(store) { viewStore in
Text(viewStore.title)
Text(viewStore.subtitle)
}
}
}
struct ViewB2: View {
let store: Store<B.State, B.Action>
var body: some View {
WithViewStore(store, observe: \.title) { viewStore in
Text(viewStore.state)
}
WithViewStore(store, observe: \.subtitle) { viewStore in
Text(viewStore.state)
}
}
} |
I do understand that you are not arguing against scoping for view stores, and it does not need further repeating for me. But I'm not sure you understand all the situations in which un-scoped In your examples,
Yes because it is not set in stone that Now I would never suggest making views like |
It depends if you put the 50% of wrong uses by hundreds (thousands?) of inexperienced users in the balance. This is what is at the core of the PR. Experienced users will know how to define themselves a |
B is a leaf view though.
Well, if it changes into a D then it's no longer a B. Then it's a different situation. I'm trying to understand what different cases there are. Obviously one situation can turn into another.
I'm talking about the way B is now, not what it could become. Assume that the only state here is |
SwiftUI is too much of a black box to know what To back up a bit, I think we should re-evaluate whether this conversation is being productive, or if a closed PR is the proper place to have it. There are multiple lines of thought happening but no way to thread the discussions. If we want to keep discussing this then I think a dedicated discussion should be started. But we will need to see something more substantial than a slight annoyance of having to specify So I think this conversation can be most fruitful if we are finding solutions to that problem and not trying to remove the only thing that publicly lets people know that they are observing state when constructing view stores. If we find a better solution than |
And I should mention that @stephencelis and I have spent a substantial amount of time looking for alternatives to view stores in general (with extensive help from @tgrapperon) so that this whole conversation becomes moot. But sadly it has alluded us thus far. We are not suggesting that |
I think the The fact that new ViewStores are created all the time always makes me uneasy, being reference types. And an ObservedObject to boot. It feels like it can't be good, that SwiftUI will get tripped up from re-subscribing all the time, or something. But it seems to work without issue. Without knowing anything about how it works, wouldn't one solution be to have a permanent, private property on Store that exposes some kind of observing entity, and then the Just one more random idea, pretty different from today: struct FeatureView: View {
@Observe(\.title, \.subtitle) var store: StoreOf<Feature>
var body: some View {
VStack {
Text(store.title)
Text(store.subtitle)
)
}
}
Feature.State here would contain lots of other state, but we would only observe title and subtitle, they would trigger the redrawing. This is a less dynamic solution than WithViewStore but it seems to correspond to having ViewStore as a property at least. Maybe it can be expanded. If it can even be made to work. Speaking of Stores, most of mine have no corresponding reducer, they are just used for scoping. I don't know if this is the intended usage, but it does cause some confusion since the two types of stores are logically pretty different, but they have the same name. Similarly I found it hard to wrap my head around scoping existing both in the Store and the Reducer domain, sort of in parallel. Btw it's not just leaf views, it's 90% of my views, because I really made an effort to be efficient. |
While sounding great theory, it would mean that I recommend going through the exercise of exploring that problem space so that you can see the tradeoffs yourself, and if you find something interesting we'd love to see it.
I suggest you try implementing this too to see what goes wrong.
I would say no, this is not the intended usage. Maybe another topic for a discussion. I'd like to iterate that I think a new discussion is best for continuing these explorations, especially for considering things like |
Sorry for writing this here, so feel free to ignore it. I have a possibly foolish idea to solve the problem: Can the Store/ViewStore record what values got accessed with the last rendering and emit a re-render only when these values change? I feel like this is something SwiftUI should do, but maybe I am missing something. Maybe SwiftUI does not have the structure the values are based upon, and so cannot put itself between the structure and the value access to record that, but rather only gets the data and so is unable to do any such thing. edit: Small poc posted here. |
The existing
WithViewStore
initializers make it far too easy to introduce performance problems into an application as it grows, and not easy to understand what's going wrong without more intimate knowledge of the framework and its documentation.Because of this, we'd like to deprecate the existing initializers and introduce new initializers that make view state observation very explicit:
It is now very explicit what state a view is observing, and we think it will stand out more when users encounter performance issues. It is something that is easier to teach and learn.
Instead of scoping stores and then handing them to
WithViewStore
, you can pass state and action transformations directly, which even saves a few keystrokes:So in short, instead of reusing
Store.scope
for two different kinds of transformations, we have separated them out to be more specific. The general rules:Store.scope
to pass a child store to a child viewWithViewStore(_:observe:)
from a child view to observe view stateThe deprecations in this PR are soft and should not cause warnings or churn in your applications, but you will start to be able to use the new APIs as soon as they are released. Then in the future we will deprecate the old APIs with a little more fanfare.
Please let us know if you have any feedback!