-
Notifications
You must be signed in to change notification settings - Fork 22
Add a recipe for presenter integration with SwiftUI "renderers" #157
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
base: main
Are you sure you want to change the base?
Conversation
9589dbf to
e7dd43d
Compare
…nd how we can render the models using SwiftUI rather than renderers. See #154
e7dd43d to
718ec3e
Compare
| import SwiftUI | ||
|
|
||
| /// A protocol for view models that create their own SwiftUI view representation. | ||
| protocol SelfRenderingViewModel { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I originally implemented this stuff, the original idea was to make a Renderer registry, which is probably more similar to how things work in KMP. A registry requires a registration step though, and I don't like registration steps (in Kotlin, the DI framework handles this more automatically). But really a registry wasn't required. In Swift, you can extend any object and make it conform to a protocol, so I made SelfRenderingViewModel. Any Kotlin view model can be extended in Swift to conform to SelfRenderingViewModel and make its own Renderer.
Some minor tweaks I would make moving forward:
- Get rid of the registry (actually, I don't think you are using the registry in this change, so that is good)
- Use SelfRenderingViewModel everywhere a ViewModel is expected. In other words, use
any SelfRenderingViewModelinstead ofAny. - Maybe rename
SelfRenderingViewModelbecause all view models should be self-rendering, so maybe name itPlatformViewModelorViewModelProtocolor something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I also felt like it was unnecessary so I omitted the registry and factory in this change and tried to keep it simple. I'll do some more refactor here
| /// Note that `PresenterView` should not be used often. `Presenters` are hierarchical, with parent `Presenters` containing the models of their children. | ||
| /// The view for a parent `Presenter` model should also present the model of its children, so `PresenterView` is only be needed for the root | ||
| /// parent `Presenter`. | ||
| struct PresenterView<Model>: View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my comment below on SelfRenderingViewModel, I think you should change <Model> to <Model: SelfRenderingViewModel>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!
Re-evaluating for the same reasons as other comments
| /// This view is a lightweight wrapper that delegates view creation to the `Models` themselves. `Models` must conform | ||
| /// to `SelfRenderingViewModel` and provide their view in `makeViewRenderer()`. This is a simple API that | ||
| /// enables view creation for models regardless of type. If needed this implementation can be changed to follow a factory pattern. | ||
| struct PresenterModelView<Model>: View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my comment below on SelfRenderingViewModel, I think you should change <Model> to <Model: SelfRenderingViewModel>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to put a type constraint on this because some parents that render children models might be agnostic to what that model actually is. For example, in SwiftUiHomePresenterView we render the backstack, which can contain a multitude of different model types. Thus we only know that our models are of type BaseModel.
I suppose we could extend BaseModel to conform to SelfRenderingViewModel and always render some default UI, but I'm not sure that's more preferable to crashing intentionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing this a bit more, going to enforce the constraint with this API and push the conformance requirement to "features" to make it explicit
| if let selfRenderingModel = model as? (any SelfRenderingViewModel) { | ||
| return AnyView(selfRenderingModel.makeViewRenderer()) | ||
| } | ||
|
|
||
| fatalError("Could not find view builder for \(type). Make \(type) conform to `SelfRenderingViewModel` protocol.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By changing <Model> to <Model: SelfRenderingViewModel>, you won't need a fatalError here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd still like to do this
|
|
||
| var body: some View { | ||
| if let viewModel = viewModelObserver.viewModel { | ||
| PresenterModelView(model: viewModel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, without a registry, you really don't need PresenterModelView at all. Just call viewModel.makeViewRenderer() here.
| /// The view for a parent `Presenter` model should also present the model of its children, so `PresenterView` is only be needed for the root | ||
| /// parent `Presenter`. | ||
| struct PresenterView<Model>: View { | ||
| struct PresenterView<Model: BaseModel>: View { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I run into issues with existential types if I try to add a constraint on PresenterViewModel, so I used the similar pattern to SwiftUiHomePresenterView. It doesn't push out responsibility to the leaf nodes as much as I would like, open to other ideas.
At the same time, this class should not be used often and if it's failing it should be pretty obvious to the user, since it's the entry point of their view hierarchy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I would prefer to use fatalError(...) in the BaseModel extension then to crash in all build types?
Add a recipe that demonstrates some APIs that can be used to:
StateFlowNavigationStackWith this change the recipe app in iOS has a different landing screen where users are able to choose the CMP-rendered recipes or the SwiftUI one
swiftui_recipe.mp4
See #154