[Woo POS] Avoid initial view model unwanted recreation #12799
Conversation
Generated by 🚫 Danger |
|
|
|
|
||
| return viewModel | ||
| }() | ||
| @ObservedObject private var viewModel: PointOfSaleDashboardViewModel |
There was a problem hiding this comment.
Just wondering, would @StateObject here fix the issue? Personally, I feel like owning the view model within the POS boundary (as if an invisible framework) would be preferable, and this seems to be the goal of StateObject in the doc. The dependencies can be DI'ed through the view's initializer, and the view just does a simple initialization of the view model with the depenencies.
@ObservedObject view model works in the HubMenu because it's being held by the UIKit hosting controller which doesn't get recreated like in a SwiftUI view.
If @StateObject doesn't work out and the POS view model needs to be DI'ed to the POS view, maybe having HubMenuViewModel own it would be more consistent with the existing setup like InPersonPaymentsMenuViewModel.
There was a problem hiding this comment.
Just wondering, would @StateObject here fix the issue? Personally, I feel like owning the view model within the POS boundary (as if an invisible framework) would be preferable, and this seems to be the goal of StateObject in the doc. The dependencies can be DI'ed through the view's initializer, and the view just does a simple initialization of the view model with the depenencies.
You mean the View declaring its own dependencies via a @StateObject rather than DI it from outside? So:
| @ObservedObject private var viewModel: PointOfSaleDashboardViewModel | |
| @StateObject private var viewModel = PointOfSaleDashboardViewModel(products: POSProductFactory.makeFakeProducts(), | |
| cardReaderConnectionViewModel: .init(state: .connectingToReader)) |
It does seem to fix the problem as well, I'm guessing because of:
SwiftUI creates a new instance of the model object only once during the lifetime of the container that declares the state object. For example, SwiftUI doesn’t create a new instance if a view’s inputs change, but does create a new instance if the identity of a view changes
I like the idea of setting a POS boundary via using StateObject, and the benefits of dependency injection for testing purposes are not lost in this case since is just the initial POS state. I don't really know if there's any other trade-offs we should be aware of?
There was a problem hiding this comment.
Yup could be just @StateObject private var viewModel: PointOfSaleDashboardViewModel and setting the view model in the view init given the dependencies.
I don't really know if there's any other trade-offs we should be aware of?
Good point, I can look more into this tomorrow. We've been using @StateObject in the code base in views that own a view model throughout the UI's lifetime.
There was a problem hiding this comment.
After some more reading on the official doc and other posts, StateObject feels to me like the most appropriate place for the view to own the view model that's only created once. With a few notes:
- The view model's dependencies shouldn't change, which I think is the case for POS - site ID, some data layer service implementation, analytics implementation (singleton in the app), etc.
Explicit state object initialization works well when the external data that the object depends on doesn’t change for a given instance of the object’s container.
If we do want the view model to be recreated again when some dependency changes, it looks like we can achieve this by binding the identity of the view that owns the StateObject view model to one or more parameters.
- StateObject should be private in all cases, and devs need to be aware not to re-initialize it again after the first initialization because any following inits wouldn't work silently and could result in unexpected behavior
Lemme know what you think!
There was a problem hiding this comment.
Sounds good to me! Updated on 4a5ded6 . Now the Hub doesn't know anything about the pos viewmodel, but is entirely driven internally by the point of entry.
This is not needed to be explicitely public anymore since we’ve removed the pos framework.
|
For some reason it failed CI in the |
| }() | ||
| @StateObject private var viewModel: PointOfSaleDashboardViewModel = PointOfSaleDashboardViewModel( | ||
| products: POSProductFactory.makeFakeProducts(), | ||
| cardReaderConnectionViewModel: .init(state: .connectingToReader) |
There was a problem hiding this comment.
super nit: is the change of the cardReaderConnectionViewModel initial state from scanningForReader to connectingToReader intentional? really just a minor non-blocking observation as it's just a simulated flow, feel free to leave it.
There was a problem hiding this comment.
Oh, not intentional at all, just a random connecting initial state :D
This PR addresses the issue discussed in slack, here: p1715934814114829-slack-C025A8VV728 . #12761 should be merged first
Description
The initial view model that drives the pos dashboard is currently created inside the
PointOfSaleEntryPointView, which is recreated multiple times, triggering a view model recreation with it.This PR addresses this problem by extracting the view model initialization a few levels up the chain, to the
HubMenuViewController, to assure it's only created once through the app's lifecycle, and then we inject it into the proper view.Question
I'm not quite sure if we should compose the view model and its dependencies on a different manner. Doing so in the
HubMenuViewControllerdoesn't feel quite right, but does follow the same pattern we already use for creating theHubMenuViewModel.Perhaps moving its composition to the
HubMenuCoordinatoror theMainTabBarControllerwould make sense? Despite POS not being a "tab", it is intended to be a different mode.Testing instructions
Filterbutton and observe that the modal is not automatically dismissed anymore.debugprinton view model init and observe that is only created once.