Sharing some ideas #1
Conversation
|
Hey, nice job!!! I'm sharing some ideas, really nitpicking a lot not necessarily because there's a right or wrong way of doing these things, but to expose a different perspective about a large variety of things that will allow you to pick the ideas that work better for you, in each scenario. I'm gonna put some inline comments to explain and justify the change, feel free to add questions on those and, once this PR is clear, please feel free to delete it together with the branch. |
|
|
||
| // MARK: - PRISM | ||
|
|
||
| extension AppAction { |
There was a problem hiding this comment.
I used this Xcode Code Snippet to create these extensions:
https://github.com/SwiftRex/SwiftRex/blob/develop/docs/markdown/CodeSnippet/PrismAssociatedValue.codesnippet
But a Sourcery Template is also available if you like it more:
https://github.com/SwiftRex/SwiftRex/blob/develop/docs/markdown/SourceryTemplates/Prism.stencil
|
|
||
| static var empty = State(id: UUID().uuidString, | ||
| title: "Type to create a new task...", | ||
| static var empty = State(title: "Type to create a new task...", |
There was a problem hiding this comment.
In this case I removed the mocks from the View Models because, as I'm using the real app reducer, I need to go through the full store. Benefit is that I don't have to rewrite Reducers.
|
|
||
| // MARK: - STATE | ||
| struct State: Equatable { | ||
| let id: String |
There was a problem hiding this comment.
It's funny but now id is no longer needed. On the place where we receive the view action we do have the id already. This is a nice way to avoid inconsistent state and bugs.
| case .toggle: break | ||
| } | ||
| return state | ||
| case toggle |
There was a problem hiding this comment.
No need for id anymore, as seen in the previous comment
| case add(String) | ||
| case remove(IndexSet) | ||
| case move(IndexSet, Int) | ||
| case toggle(String) |
There was a problem hiding this comment.
In the "model" level of toggle action we still need the id. But we remove the responsibility of keeping track of this id from the view, making it more "dumb".
| struct AppState: Equatable { | ||
| var appLifecycle: AppLifecycle | ||
| var task: TaskList.State | ||
| var tasks: [Task] |
There was a problem hiding this comment.
I recommend that your AppState tree (and AppAction tree) don't point to View Models / View Events. Usually the View Model is not source-of-truth but a state derived from Model/State, therefore you can easily calculate it out of the State ([Task]). This separation can have benefits in the future, when you need to share the property tasks among different screens and sometimes between different modules. Then your State/Model [Task] is the source-of-truth and the TaskList.State is calculated from that.
|
|
||
| static func viewModel<S: StoreType>(store: S) -> ObservableViewModel<TaskList.Action, TaskList.State> | ||
| where S.ActionType == AppAction, S.StateType == AppState { | ||
| where S.ActionType == TaskAction, S.StateType == [Task] { |
There was a problem hiding this comment.
It could be AppAction/AppState here. But if you plan to eventually move this view to another module, you can use strictly the essential to build a view and handle its actions, ignoring things like AppLifecycle for example. This is totally optional.
| title: state.task.title, | ||
| tasks: state.task.tasks | ||
| title: "Tasks", | ||
| tasks: state.map { State.Item(id: $0.id) } |
There was a problem hiding this comment.
All our rows need now is id. It could even be an array of String, but because String is not identifiable you would need to use id: .self. I personally like this more.
| case add(String) | ||
| case remove(IndexSet) | ||
| case move(IndexSet, Int) | ||
| case toggle(CheckmarkCellView.Action) |
There was a problem hiding this comment.
Because the action "toggle" is handled completed by the cell, and because now I'm creating the cell from outside and injecting it here, we don't have to be the proxy for this action any more.
| // MARK: - REDUCERS | ||
|
|
||
| extension Reducer where ActionType == TaskList.Action, StateType == TaskList.State { | ||
| extension Reducer where ActionType == TaskAction, StateType == [Task] { |
There was a problem hiding this comment.
Reducing View State or View Event directly feels a bit wrong. I always prefer isolating Reducers/Middlewares as much as possible from View things. If tomorrow you want to reuse the same logic for an Apple Watch app, where your views have completely different distribution due to the small screen, or if you want to write a console app for example, having the logic tied to View elements will hold you back. Yes, there's an extra step to convert things when you "plug" your views (store projection), but I believe that in this case you could have some weird situations in the future as your app has more screens and share the same model with them. So instead of reducing views, you reduce the model. Views only react to that.
There was a problem hiding this comment.
So in this case, since this is really a "TaskAction / [Task]" concern, wouldn't it make more sense to move this out of the TaskListViewModel ? It doesn't carry any specificity at this level, imo, and seems like a good candidate to move (back!) to ReduxFramework.swift.
There was a problem hiding this comment.
Exactly... I didn't move things to make easier the Git Diff, but some things I would put in different folders. Nowadays what I found best for me is organising folders by topics, not by layer. So TaskList would be a folder with TaskListView, TaskListViewModel, TaskListViewProducer and TaskListHandler (that has reducer and middleware). Then another folder TaskDetails...
| static func viewModel<S: StoreType>(store: S, taskId: String) -> ObservableViewModel<CheckmarkCellView.Action, CheckmarkCellView.State> | ||
| where S.ActionType == TaskList.Action, S.StateType == TaskList.State { | ||
| let task = store.mapState { state in state.tasks.first { $0.id == taskId } } | ||
| where S.ActionType == TaskAction, S.StateType == [Task] { |
There was a problem hiding this comment.
Here we are coming from AppState/AppAction objects, instead of coming from View State / View Events. This is because I'm projecting not from the list view, but from outside where I do have the full Store available for me.
| let task = store.mapState { state in state.first { $0.id == taskId } } | ||
| return task | ||
| .projection(action: Self.transform, state: Self.transform) | ||
| .projection(action: Self.transform(taskId: taskId), state: Self.transform) |
There was a problem hiding this comment.
Here is interesting, we already have the task id in this context, we just supply our transform with it.
| static func transform(taskId: String) -> (CheckmarkCellView.Action) -> TaskAction? { | ||
| return { viewAction in | ||
| switch viewAction { | ||
| case .toggle: return .toggle(taskId) |
There was a problem hiding this comment.
And suddenly your View Event doesn't need the id any more. We can create a AppAction.TaskAction (that needs the id) out of this View Event that doesn't hold the id. So the row no longer needs the id for anything.
| where S.StateType == AppState, S.ActionType == AppAction { | ||
| TaskList( | ||
| viewModel: TaskList.viewModel(store: store.projection(action: AppAction.task, state: \AppState.tasks)), | ||
| rowView: { id in taskListRowView(store: store, taskId: id) } |
There was a problem hiding this comment.
This is the view producer injection. This is useful because now we have the full store available when creating any view, without having to send it to the view itself. The closure environment receives the full store, but the view itself doesn't need it. This is specially useful when using views from different modules or when using NavigationLink to push views with completely different requirements.
This also decouples more the parent and child view, as the parent doesn't have to either hold more state than necessary (we were able to have an row that only needs Task Id and nothing else) nor handle more events than necessary (we were able to remove "toggle" action from the parent, and the proxying of it).
| static let mockStore = ObservableViewModel<AppAction, AppState>.mock( | ||
| state: stateMock, | ||
| action: { action, _, state in | ||
| state = Reducer.app.reduce(action, state) |
There was a problem hiding this comment.
We can use all reducers here! The preview will behave very close to the real app, but without the Middleware events or side-effects. If you want to emulate the middleware side-effects callbacks you still can do it in this closure here.
There was a problem hiding this comment.
Note that this is one of my least favorite change 🤷♂️. While it seems great at first sight to be using the AppState mock, it's pushing down to our "independent" View some of the App level constructs.
There was a problem hiding this comment.
That's fair. We could pick only the reducers that are relevant and that way use a "store" from that level.
| CheckmarkCellView( | ||
| viewModel: | ||
| CheckmarkCellView.viewModel( | ||
| store: mockStore.projection(action: AppAction.task, state: \AppState.tasks), |
There was a problem hiding this comment.
You can even use these previews for Snapshot Testing.
There was a problem hiding this comment.
https://www.youtube.com/watch?v=tk0HzScvW2M
She uses this library: https://github.com/pointfreeco/swift-snapshot-testing
While you're there, you may wanna check that Point-free also has a redux framework that shares some similarities with SwiftRex, but with some different concepts and completely different implementation. The benefit is that they probably have a larger community ready to give support. But there are some significant differences and you may like more one or another. Since you're comparing libraries, I think you'd like to know.
|
🎉 Awesome ! Thanks for the feedback. I will go over all the comments. Since I was trying to use the ForEach extension as my next step today, this will play well into that refactor I believe. Oh, btw, I am (obviously) able to use CombineRextensions now. |
|
Pick the ones you like, discard the ones you don't like. I tried to give a completely different perspective as much as I could, to offer more alternatives for you. Now it's up to you to pick what you like. Then you can discard this PR. |
| Reducer<TaskAction, [Task]>.task.lift( | ||
| action: \AppAction.task, | ||
| state: \AppState.tasks | ||
| ) <> Reducer<AppLifecycleAction, AppLifecycle>.lifecycle.lift( |
There was a problem hiding this comment.
Great, you read my mind ! Was gonna ask about some examples of combining reducers with the <> operator. Nice.
There was a problem hiding this comment.
Something like
[
reducer1, reducer2
].reduce(.identity, <>)
also works. And it's fun to reduce reducers. :D
|
I opted to merge your pull request given the amount of great information / comment there. Will follow up with an additional PR on some more "cosmetic" changes. |
…n PR #1. Mostly trying to keep the potential for "modularity" intact. So the CheckmarkCellView projection and the Reducer were moved away from TaskListViewModel and under the ReduxFramework. This leaves the actual View Models pretty bare bone.
Added contexts for the new iteration that was triggered from PR #1 .
2 participants
No description provided.