Skip to content

Swift UI mvi enhancements #47

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

Open
wants to merge 5 commits into
base: swift_ui_mvi
Choose a base branch
from

Conversation

Fonix-za
Copy link
Contributor

Main changes:

  • Composition over inheritance, custom viewmodels dont inherit but rather are combined with journey viewmodels
  • Journey state becomes part of the custom state, not the other way round
  • State is now @observable for better performance, each property publishes individually instead of the entire state
  • Use of some generics to remove some boilerplate code for the IntentHandler, and just combine it into the viewmodel for simplicity
  • Added some DI just to see how it will look

@tibor-is-back
Copy link
Collaborator

tibor-is-back commented Apr 19, 2025

  • Journey state becomes part of the custom state, not the other way round
    To me this is more cleaner, but it comes with the downside that if you want to add an additional state, you need to change it everywhere in the new custom view, while with the original one, the original view-state handling logic stays intact in the view since it doesn't care about the extension, which is an advantage for the customer.
  • State is now https://github.com/observable for better performance, each property publishes individually instead of the entire state
    It's not yet clear whether we can use @observable or not. If we can that has other positive implications elsewhere, however this also has one disadvantage. With enums it super easy and with structs it's easier to constrain the customers to valid states, however going the @observable route then it will be harder because the properties needs to be declared public, and the customers can just change it creating invalid states to their liking. I definitely want to run some benchmarks around this.
  • Use of some generics to remove some boilerplate code for the IntentHandler, and just combine it into the viewmodel for simplicity
    This gets boiled down to the fact what is the view model? Why do we even have it if it's so empty, right? Which is a valid question. While your solution is good, since we don't need the waltz with the closure (or delegate or duplicating the state in the Intent Handler). However, it's more complex, and has all the problems why it shouldn't be combined in theory imho:

1. Separation of concerns: ViewModel has more responsibility then storing the view state
2. Testing will get unnecessarily more complex
3. I should be be able to use more intent handlers for my complex view logic, now I need to use the ViewModes, while I don't need the other features it provides and my logic will be also more complex, especially handling all the states and combining them (of course with closures it's also ugly 😓).

I can imagine that in the future we want to have some kind of middleware before the intent handler (for the observability events for example) for that we need a separate view model and it needs to be open for inheritance).

  • Added some DI just to see how it will look

@Fonix-za
Copy link
Contributor Author

  • Journey state becomes part of the custom state, not the other way round
    To me this is more cleaner, but it comes with the downside that if you want to change the state, you need to change it everywhere in the custom view, while with the original one, the original view-state handling logic stays intact in view since it doesn't care about the extension.

Im not sure i follow, do you mean in the case that a customer copies the code for the journey view and wants to add their own state? i guess so, but i guess it depends, as an implementer do i want my custom state to be the first class state, or the journey state?

  • State is now https://github.com/observable for better performance, each property publishes individually instead of the entire state
    It's not yet clear whether we can use https://github.com/observable or not. If we can that has other positive implications elsewhere, however this also has one disadvantage. With enums it super easy and with structs it's easier to constrain the customers to valid states, however going this route it will be harder because the properties needs to be declared public, and the customers can just change it creating invalid states to their liking. I definitely want to run some benchmarks around this.

yeah this is just wishful thinking if we can use iOS 17. im just wondering also with the enum state way, even though it may guide you into not making invalid states, it also can limit the person customising the journey. what may be an invalid in a OOTB journey, may not be after adding some custom state on top of it, which would make you have to ditch all the screens built in stuff probably. but maybe that is an unlikely scenario. Im also wondering that some customers may want to combine viewmodels potentially, and then 2 viewmodels may want to interact in the same screen, the enum scenario might make this difficult to do as the valid states would be totally different. it does require the implementer to have a deeper understanding of whats going on though, but they would have more freedom to do the changes they need.

Use of some generics to remove some boilerplate code for the IntentHandler, and just combine it into the viewmodel for simplicity
_This gets boiled down to the fact what is the view model? Why do we even have it if it's so empty, right? Which is a valid question. While your solution is good, since we don't need the waltz with the closure (or delegate or duplicating the state in the Intent Handler). However, it's more complex, and has all the problems why it shouldn't be combined in theory imho:

  1. Separation of concerns: ViewModel has will get more responsibility then storing the view state
  2. Testing will get unnecessarily more complex
  3. In theory I can use more intent handlers for my complex view logic, now I need to use the ViewModes, while I don't need the other features it provides and my logic will be also more complex, especially handling all the states and combining them (of course with closures it's also ugly 😓)._

for 1, the previous viewmodel implementation wasnt even a viewmodel really, it didnt do much besides forwarding intents to the intent handler and had a property for state. so for sepration of concerns i felt like the viewmodel had no concerns to begin with really.
2. i think its really the same as before, i have just moved things basically to a super class so you dont have to write the boilerplate code each time you create one, nothing has really changed with the implementation overall.
3. the intent handlers were tightly coupled with the viewmodel as you cant reuse intent handles for different viewmodels, and the intent handler when you boil it down was just a function to handle intents coming in, so now thats all it is in the viewmodel. if you have complex logic you should break up the function into functions with more specific purposes (ideally one function for each intent, which makes it more testable), much like you would have created separate intent handlers before. just because they are part of the viewmodel now doesnt mean you cant split it up over different functions/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants