Skip to content
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

RFC: Unsafe access to store state and actions. #1946

Closed
wants to merge 3 commits into from

Conversation

mbrandonw
Copy link
Member

A few months ago we moved to start making view store observations a lot more explicit. We now encourage people to do WithViewStore(store: observe: { … }) and ViewStore(store, observe: { … }) in order to chisel away at the state to the bare essentials the store needs.

There's one big ergonomic problem with this that we never dealt with, and that's a quick way to send actions when you only have a Store. Technically you would have to do something like this:

ViewStore(store, observe: { $0 }).send()

That's awkward, and gets even more awkward if the state is not equatable.

Really, view stores are just kind of awkward in general. We introduced them out of necessity but it's been a constant struggle to have good ergonomics. Luckily we think view stores aren't long for this world thanks to some of the new Observation stuff coming soon.

So, we're looking to add some new API surface area to improve the view store ergonomics today, knowing that hopefully someday in the future we can completely remove the concept, or at least make it necessary only in a small number of situations.

We are suggesting adding a withUnsafeStore API that will give you instant access to the store's state and sending actions for a well defined scope:

store.withUnsafeStore { $0.users }
store.withUnsafeStore { $0.send(.addButtonTapped) }

It's named "unsafe" because it does not perform any observation at all. You are only getting a snapshot of state at a specific time. And it's a little verbose, but a lot nicer than the alternative today.

What are people's thoughts on this?

Copy link
Member

@stephencelis stephencelis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the general direction! Will let others weigh in on any API ideas.

@jshier
Copy link
Contributor

jshier commented Feb 27, 2023

"Safe" has a very particular meaning in Swift, namely "memory safe". It doesn't seem like a good idea to conflate the safety of Swift itself with a TCA-specific construct. Perhaps we can think of a TCA-specific term for this sort of inefficient Store. It could be as simple as withInefficientStore, withUnscopedStore, withSnapshotStore, or withEscapedStore.

@mbrandonw
Copy link
Member Author

@jshier that's definitely true, but we took inspiration for this API from Swift's withUnsafeContinuation, which as far as we known only checks that the continuation is resumed exactly one time, and doesn't have anything to do with memory safety.

@jshier
Copy link
Contributor

jshier commented Feb 27, 2023

Eh, I'd guess unchecked continuations could lead to memory corruption if their resumption wasn't monitored, since you could have a thread safety issue. I suppose the definition from the language can now be expanded to "thread safe" as well.

In either case, this API doesn't seem unsafe by any definition. At worst it's inefficient. You could really stretch the term and consider it unsafe because large, unscoped observation may trigger latent SwiftUI bugs, but you can't really use it for that here, can you?

Additionally, since the "create local Store to send one off action" pattern is actually recommended when you don't need state at all, it doesn't seem like a good idea to dissuade users from using by calling it unsafe.

@rcarver
Copy link
Contributor

rcarver commented Feb 27, 2023

Perhaps "unobserved" instead of "unsafe"?

Out of curiosity why do we still need a ViewStore to send?

store.withUnsafeStore { $0.send(.addButtonTapped) } // 😭
store.send(.addButtonTapped) // 🤷‍♂️

@jshier
Copy link
Contributor

jshier commented Feb 27, 2023

Yeah, if this converts to a ViewStore, then that should be in the name. withUnobservantViewStore is a bit pedantic but rather self explanatory.

@jshier
Copy link
Contributor

jshier commented Feb 28, 2023

Or is this a first step towards eliminating ViewStore altogether? So this would be the first API to allow sending actions directly to a Store instance?

@mbrandonw
Copy link
Member Author

Out of curiosity why do we still need a ViewStore to send?

It's a reasonable question, and honestly it's funny how difficult it is to answer directly. Here's my best shot.

If we step back a bit, the main reason for ViewStore to exist is to give us a scratch object to construct that focuses on state and actions just for a view. View stores are't meant to be passed around to a bunch of views, they are highly specific to one view.

This is in stark contrast to stores which are meant to be passed along to child views, although they are usually first scoped. This is what makes it hard to combine both of these concepts into one package.

For example, many people want scope actions specifically for a view so that the view can't send "internal" or "effect" actions. How would you do that without a view store? If you scope the store the view holds onto then you can't pass it to a child because no child is going to care about those view actions. So then are you going to hold onto two stores just so that you can send view actions? That is bizarre and doesn't even solve the problem.

All of that, and more, is what led us to create the view store as the central object that holds onto domain specific to a view. Both state and actions. A nice side effect of this also is that it comes with a performance boost if you do it correctly.

Or is this a first step towards eliminating ViewStore altogether? So this would be the first API to allow sending actions directly to a Store instance?

It is a very tiny step. We do want to eliminate ViewStore eventually, or at least severely diminish its use case, and so we consider "withUnsafeStore" a temporary tool to use until that day. But also to be used sparingly.

We could be persuaded by withUnobservedStore, but it does seem verbose.

@jshier
Copy link
Contributor

jshier commented Feb 28, 2023

For example, many people want scope actions specifically for a view so that the view can't send "internal" or "effect" actions. How would you do that without a view store? If you scope the store the view holds onto then you can't pass it to a child because no child is going to care about those view actions. So then are you going to hold onto two stores just so that you can send view actions? That is bizarre and doesn't even solve the problem.

I really don't know what you mean here. Assuming we could send to Stores directly, couldn't anyone who wanted limited actions available to some child view just scope like normal? I supposed that, if you didn't want to force everyone to take Stores in every child view, you could end up with a very similar structure to what we have now, where instead of a ViewStore, we simply have a .scope method on Store that creates something that could be represented in a view builder. Or perhaps a send method that takes a scoping path?

Getting back to the naming question, perhaps something that indicates the value is only for sending would be appropriate? Or maybe just create a type that can only send, so there's no other access to the Store in the first place. It could even be scoped. store.withSender(of: /Child.action) { $0.send(.childAction) }.

@tgrapperon
Copy link
Contributor

When I saw the commit before spotting this discussion, I immediately thought that it should be named StoreSnapshot, in line with @jshier comment about unsafe. I then realized that unsafe may deter folks from using this API, and that was maybe the reason this prefix was chosen, but it doesn't seem to be the case. The problem with SnapshotStore is that it doesn't really work with the idea of sending things to it, but on the other hand UnobservedStore doesn't make a lot of sense given that ViewStore is not called ObservedStore. So UnsafeStore may indeed work, as imperfect as it is.

I personally think that the concept of ViewStore is fundamental on the State part, not because it observes it, but because it provides a real-life projection (a measure) of something that is otherwise opaque (the store's state). There is a bunch of stuff in the store that corresponds to its "state" (in the general sense), but which is not observable (for example, the current stack of Effects being performed). We can also imagine that the Store can live at its own async pace at some point, and that the ViewStore will provide a projection of its state into the MainActor world. In a similar fashion, projecting an identified array into its simple count is very similar to what happens in physics where we project the state of a system (which can be a combination of complex functions) into some observable value (i.e., real numbers). But I also agree that the actual ergonomics is far from ideal, and if Observation allows to use Store directly, so be it. But these ideas will always lurk in the shadows.

I'm more reserved about the ViewAction aspects in its current form, and I think that we could have probably sent actions directly to the Store from the get-go. I get the safeguards/boundaries it can provide to the developer, but I feel that they're more artificial than anything else right now. In an async Store context, however, I can imagine ViewAction being actions that are sent from a MainActor context, and this could maybe be exploited somewhere. Right now, all actions are received on the MainActor, but we can maybe imagine something so that in the future, only ViewAction will be reduced synchronously.

In other words, I don't know if the concept of ViewStore will completely disappear, but it is indeed likely that its current Combine-based ObservableObject form is not long for this world. I'm not sure how the upcoming changes to SwiftUI will allow to perform the transition, so UnsafeStore is maybe generic enough to allow deferring a more specific commitment to a later time.

That's a lot of words to say that I'm OK with the change in the context of a transitory solution to a problem we have today.

@rcarver
Copy link
Contributor

rcarver commented Feb 28, 2023

Thanks @mbrandonw @tgrapperon for the insights.

I'll just say that purely as a user, without deep insight into the complexities or transition plans. Something like this would be pretty convenient at times:

store.snapshot { $0.users }
store.send(.addButtonTapped)

@acosmicflamingo
Copy link
Contributor

"Snapshot" also came to mind but didn't think too much of it since I associate it with diffable data in UICollectionView. Seeing others bringing up "snapshot" makes me think there is something to it :)

@mbrandonw
Copy link
Member Author

I really don't know what you mean here. Assuming we could send to Stores directly, couldn't anyone who wanted limited actions available to some child view just scope like normal? I supposed that, if you didn't want to force everyone to take Stores in every child view, you could end up with a very similar structure to what we have now, where instead of a ViewStore, we simply have a .scope method on Store that creates something that could be represented in a view builder. Or perhaps a send method that takes a scoping path?

@jshier I think you misunderstood my statement. Suppose you had a view like this:

struct ParentView: View {
  let store: StoreOf<Parent>
  var body: some View {
    HStack {
      ChildView(store: store.scope(state: \.child, action: Parent.Action.child))
      Button("Tap") { self.store.send(.tap) }
    }
  }
}

If you could send actions directly to Store then you would be able to send any action in the parent domain. This includes internal actions, child actions, effect actions, etc.

So you might try restricting the actions that ParentView can send, but how would you do that? If you tried this:

struct ParentView: View {
  let store: Store<Parent.State, Parent.Action.View>
  var body: some View {
    HStack {
      ChildView(store: store.scope(state: \.child, action: Parent.Action.child)) 
      Button("Tap") { self.store.send(.tap) }
    }
  }
}

…then you are out of luck. You can't possible scope the store to pass to the child because the store no longer holds onto any child actions. That was kind of its whole point too, to remove the non-view actions.

So what you really need to do is hold onto two stores, one for the true domain of the feature (and it's child domains) and one for the view domain:

struct ParentView: View {
  let store: StoreOf<Parent>
  let viewStore: Store<Parent.State, Parent.Action.View>
  var body: some View {
    HStack {
      ChildView(store: store.scope(state: \.child, action: Parent.Action.child))
      Button("Tap") { self.viewStore.send(.tap) }
    }
  }
}

And so now we have re-invented the concept of view stores, but just in a weirder way. We have to hold onto two stores and nothing is stopping you from still sending non-view actions to the store.

I want to stress that the situation is far more nuanced and delicate than just letting one send actions to the store. The view store is there for a reason (both for state and actions) and the alternative is not ergonomic. It really does serve as a scratch object just for the view where it is not appropriate to reuse the store, and that's the best way of thinking about it.

I'm more reserved about the ViewAction aspects in its current form, and I think that we could have probably sent actions directly to the Store from the get-go.

We went through lots of iterations for ViewStore in the early days of TCA (before open sourcing and even before episodes) and tried out lots of these idea. We thought it would be weird if there was a concept of ViewStore for reading state yet you would still send actions to a Store.

And we can't ignore the fact that there are an increasing number of people who are separating "view" actions from the rest of their actions. Really it is quite strange that you can technically send "effect" actions and child actions from a view when that is not intended at all.

I'll just say that purely as a user, without deep insight into the complexities or transition plans. Something like this would be pretty convenient at times:

store.snapshot { $0.users }
store.send(.addButtonTapped)

@rcarver I understand the sentiment, but I think we need some concrete details of how this could actually work in practice and still serve the various use cases of the library.

@JacksonUtsch
Copy link
Contributor

The idea of removing ViewStore is interesting. I think its purpose seems much more clear when thought in light of Store being an actor (although this is not the case in TCA). You basically need a ViewStore to observe changes. It allows the SwiftUI implementation of a ViewStore be separate and allow you to build the architecture for all platforms. ViewStores then have various purposes.

I'm not entirely sure what Observation brings to the table besides a uniform way of observing change. I'm sure it can help in some areas.

@stephencelis
Copy link
Member

Thanks all for your input! We've rethought things and would like to propose something else. Please join the discussion here: #2223

Closing in favor of #2222

@stephencelis stephencelis deleted the with-unsafe-store branch June 21, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants