npvisual / ToDoList-Redux

Redux based ToDo / Task application
4 stars 2 forks source link

Error when Instantiating view models #2

Closed rustproofFish closed 3 years ago

rustproofFish commented 3 years ago

Hi - firstly, thanks for this example app and the documentation. I've been picking it apart as I learn about SwiftRex and rebuilding it as part of the process and hit upon a rather interesting complier error relating to the way that the view models are instantiated.

As far as I can see, my code is identical to yours (aside from a change of name for the Views) and yet my code won't compile due to the dual errors "cannot call value of non-function type" and "instance member 'viewModel' cannot be used on type 'TaskListView'; did you mean to use a value of this type instead?" I actually agree with the compiler here - I've never seen this syntax before and I would have expected the viewModel property definition to prevent its use in this manner. Initially I thought there might be recursion as well but then realised it couldn't be as we're working with structs.

Your example (unmodified from the original) compiles and runs fine. Mine doesn't... I've checked dependencies (all match), imports, etc, etc. The only difference between the code bases is that you've separated the view extensions into their own files whereas I've kept the extensions in the same file as the view they refer to. I can't see how this would make a difference though. I am going slowly mad trying to figure out a) how and why this actually works (in your code) when it doesn't fit with my understanding of Swift and, b) why mine doesn't. Can you help?

Your example:

//
//  TaskList.swift
//  ToDoList-Redux
//
//  Created by Nicolas Philippe on 7/30/20.
//

import SwiftUI

import SwiftRex
import CombineRex

struct TaskList: View {
    @ObservedObject var viewModel: ObservableViewModel<Action, State>
    let rowView: (String) -> CheckmarkCellView

    var body: some View {
        NavigationView {
            VStack(alignment: .leading) {
                List {
                    ForEach(self.viewModel.state.tasks) { rowView($0.id) }
                        .onDelete { viewModel.dispatch(.remove($0)) }
                        .onMove { viewModel.dispatch(.move($0, $1)) }
                }
                Button(action: { viewModel.dispatch(.add) }) {
                    HStack {
                        Image(systemName: "plus.circle.fill")
                            .resizable()
                            .frame(width: 20, height: 20)
                        Text("New Task")
                    }
                }
                .padding()
                .accentColor(Color(UIColor.systemRed))
            }
            .navigationBarTitle(viewModel.state.title)
            .navigationBarItems(trailing: EditButton())
        }
    }
}

#if DEBUG
struct TaskList_Previews: PreviewProvider {
    static let stateMock = AppState.mock
    static let mockStore = ObservableViewModel<AppAction, AppState>.mock(
        state: stateMock,
        action: { action, _, state in
            state = Reducer.app.reduce(action, state)
        }
    )
    static var previews: some View {
        Group {
            TaskList(
                viewModel: TaskList.viewModel(
                    store: mockStore.projection(action: AppAction.list, state: \AppState.tasks)
                ),
                rowView: { Router.taskListRowView(store: mockStore, taskId: $0) }
            )
        }
    }
}
#endif

My version:

import SwiftUI

import SwiftRex
import CombineRex

// MARK: - View
struct TaskListView: View {    
    @ObservedObject var viewModel: ObservableViewModel<Action, State>

    var rowView: (String) -> TaskCellView

    var body: some View {
        NavigationView {
            VStack(alignment: .leading) {
                List {
                    ForEach(self.viewModel.state.tasks) { rowView($0.id) }
                        .onDelete { viewModel.dispatch(.remove($0)) }
                        .onMove { viewModel.dispatch(.move($0, $1)) }
                }
                Button(action: { viewModel.dispatch(.add) }) {
                    HStack {
                        Image(systemName: "plus.circle.fill")
                            .resizable()
                            .frame(width: 20, height: 20)
                        Text("New Task")
                    }
                }
                .padding()
                .accentColor(Color(UIColor.systemRed))
            }
            .navigationBarTitle(viewModel.state.title)
            .navigationBarItems(trailing: EditButton())
        }
    }
}

// MARK: - Define view model for this view
extension TaskListView {
    struct State: Equatable {
        var title: String
        var tasks: [Item]

        struct Item: Equatable, Identifiable {
            let id: String
        }

        static var empty = State(title: "Tasks", tasks: [])
    }

    enum Action {
        case add
        case remove(IndexSet)
        case move(IndexSet, Int)
    }
}

#if DEBUG
struct TaskList_Previews: PreviewProvider {
    static let stateMock = AppState.mock
    static let mockStore = ObservableViewModel<AppAction, AppState>.mock(
        state: stateMock,
        action: { action, _, state in
            state = Reducer.app.reduce(action, state)
        }
    )
    static var previews: some View {
        Group {
            TaskListView(
                viewModel: TaskListView.viewModel(store: mockStore.projection(action: AppAction.list, state: \AppState.tasks)),
                rowView: { Router.taskCellView(store: mockStore, taskId: $0) }
            )
        }
    }
}
#endif

Error message (my code):

Screenshot 2020-08-19 at 22 38 11

npvisual commented 3 years ago

@rustproofFish, have you defined the projection :

// MARK: - PROJECTIONS

extension TaskList {
    static func viewModel<S: StoreType>(store: S) -> ObservableViewModel<TaskList.Action, TaskList.State>
    where S.ActionType == ListAction, S.StateType == [Task] {
        store
            .projection(action: Self.transform, state: Self.transform)
            .asObservableViewModel(initialState: .empty)
    }

    private static func transform(_ viewAction: TaskList.Action) -> ListAction? {
        switch viewAction {
            case .add: return .add
            case let .remove(offset): return .remove(offset)
            case let .move(offset, index): return .move(offset, index)
        }
    }

    private static func transform(from state: [Task]) -> TaskList.State {
        TaskList.State(
            title: "Tasks",
            tasks: state.map { State.Item(id: $0.id) }
        )
    }
}

That's where the static function is defined.

I copy & pasted your code and without defining the projection, I get the same error you described. Let me know if that doesn't solve it.

rustproofFish commented 3 years ago

Thanks @npvisual - that was what I was missing. Development isn't my day job (as you may have guessed) so I can often miss things that might be obvious to experienced devs ;-) Code style is very individual as well - part of the process for me is reorganising examples in a way that fits inside my head! I'll have a poke around some more but ultimately I'm going to try and add persistence middleware to your example. Thanks again

rustproofFish commented 3 years ago

Everything is up and running but I was wondering if I could ask a question regarding architecture?

There's a significant use of static variables to instantiate objects (like the view model that gave me so many difficulties above) and structs/classes are broken into distinct chunks using extensions to delineate areas of functionality. Is there a specific reason this design is to be preferred? I did have a brief try at creating a standalone ViewModel for each of the views but ran into difficulties with the definition due to the generic components which might be the explanation.

npvisual commented 3 years ago

@rustproofFish, not sure I have a great answer for this and maybe @luizmb can add some more wisdom.

The quick answer is that this is what Redux is about and what using a framework brings to the table. We're mostly dealing with States, Actions and Reducers :

The Reducers we provide in our apps are just new functions that take (Action, State) and return State. In their core, they do not have any side-effect (i.e. they don't modify other variables) -- they are pure functions. So they are very static (!) and because they are wrapped in a struct, the best way to declare them is as a static var or, I would argue a static let (they are constant throughout, after all).

Because we want to leverage the operations provided by the framework, namely reduce, <> (compose) and lift , we just extend the Reducer type.

Now for some of the other use of extensions, this is pure syntactic sugar. I like to be able to use State and Action in the view models. That allows people reading the code to immediately recognize the pattern that we're trying to infer as part of the framework (remember there's no pre-defined State or Action types in SwiftRex).

So if makes it easier if the State and Action declarations are wrapped inside their respective View declarations (to avoid name collisions) and what better way to do that than putting those in extension chunks ? You can move those chunks wherever (different file, same file but at the bottom, etc.) to make your code more readable and they don't interfere with the core definition.

The latter is just preference. I wouldn't dare say it's "the preferred way". All depends what you're comfortable with. But you need to recognize what's form and what's required by the framework (i.e. the Reducer definitions).

But overall just remember that we're trying to be as close to a functional model as possible, without side-effects (except for the Middleware, but they don't modify state). So it might be "static" all the way down !

I did have a brief try at creating a standalone ViewModel for each of the views but ran into difficulties with the definition due to the generic components which might be the explanation.

I don't see a problem with a standalone ViewModel. Maybe if you share your code I can see what's going on.

luizmb commented 3 years ago

i really like the answer from @npvisual, it captures the spirit of redux and functional programming. I may try to give my point-of-view on that, but I would probably be saying the same things in a different manner, which still can be helpful perhaps.

My goal when I write some app is doing it in a way that writing bugs is harder, and writing tests is easier. That's why I like to rely a lot on Functional Programming and strong generics programming, because it's usually very testable and small mistakes will cause compilation error, not runtime error. This can be a bit frustrating in the beginning, you might feel like struggling the compiler and making not much progress as before, but it'll certainly save you debugging time, manual testing time and unreproducible bugs in the wild. So from my experience, overcoming this initial difficulties will be worthy and you'll be rewarded by your invested time.

Most of the things you pointed out, are completely optional and you're actually free to decide the best way for you to go. SwiftRex is flexible enough to have an easier implementation where everything is AppState and AppAction, so you don't even need to use things like lift, projection, view producers etc. The whole app could even be a single Reducer and a single Middleware.

However... More you get used to these patterns you'll realise that you can do more, you can actually write views that are totally unable to trigger actions that they are not supposed to, and this validated by the compiler. You can write Reducers that work in a very very narrow scope, and that way you don't use those infamous "default" in switch/case that could make you forget handling certain action.

Having the separation between: state mutation (Reducer), side-effects (Middleware) and data presentation (Views) is only the first step toward that goal. Writing the whole app solely with pure functions, except for your middleware, will allow you to test everything, to compose parts much easier, to use SwiftUI Previews with a lot of flexibility, to perform time travel or crash replay and much more. That's the spirit of so many static func. They are only static to put them under a common namespace, for example, extension of Reducer would have lots of reducers in form of static funcs, or ViewProducer will be the umbrella for all known view producers in your module, etc. It's just a matter of namespacing things, but this is totally optional. We could have them as free functions in the global namespace, that way they wouldn't need to be static. Why not an instance func, then? Well... You still can do it if you want. The problem is that once you start having instances of things floating around, it's simply too easy to put a variable in that struct or class and start having parallel state untracked by redux. You'll also will start thinking about lifecycle of that instance, copy/reference pass, etc. And usually this is not needed at all, those instances would only be a dangerous place where you could start adding things that hurt your app purity.

So, although you can have as many objects and instances as you want, I recommend you to check whether or not this is necessary. And remember, an "init" is only a special kind of "static" func, so if you don't plan to have properties in those objects, maybe static funcs (or static let pointing to closures, which is the same) should be fine.

Regarding ViewModels, you could have them created for you in your SwiftUI View init, if you pass the full Store to it, then it would be able to create a store projection focused on the state/action needed for that view. I usually don't do that because I like decoupling View and model completely, so I like a "router" layer to glue them. This "router" layer should (in my case) be stateless, and in fact I use View Producers as such. View Producers are only closures (State) -> View that have a slight "impurity": they capture the full Store from each other. I find this acceptable (as Store is a singleton holding the source-of-truth anyway), but you may want to make this better. The static constructor for them only helps me avoiding variables, properties and parallel state.

Please feel free to adapt to your needs and we will be very glad with new ideas how to make this architecture even better. The SwiftRex itself won't enforce these rules (ViewProducer is not even part of the library, for example), so this architecture we implemented here is more or less a suggestion based on years trying to find a good way to glue these things :)

PS: With the structure proposed here, you will be able to drive navigation from App State. This is very useful for UI Test and Screenshot automation. You dump your App State to a json, reload it in a UI Test and suddenly you're deep in the app navigation tree. This is another reason why stateless routers are good, and no @State in SwiftUI views will also help you.

rustproofFish commented 3 years ago

@npvisual, @luizmb - thank you for your very comprehensive answers which are really very helpful. Functional programming appeals to me conceptually but, until now, my experience has been limited to using frameworks such as RxSwift and Combine to transform data as it flows through the app without utilising the more formalised code structures associated with functional programming. As a result I've been used to my code being organised in a manner more consistent with the imperative styles. I've managed the transition from MVC to MVVM without too much difficulty so I'm hoping I can get my head around this as I think the benefits (testability, bug prevention, etc) make the effort worth it. I think coming to SwiftRex cold, the problem has been keeping track of the moving parts as there's more than I've been used to. For the moment, I'm very happy to defer to the expertise of those far more knowledgable than me in such matters. I can always revise structure once I've got the operational concepts straight in my head.

Although it's not strictly related to the original issue, as it's been mentioned I'll pick up on it here. As far as state mutation and middleware is concerned, is the following sequence correct?

luizmb commented 3 years ago

Migrating from imperative to declarative is never easy, or obvious. But the benefit will pay off. Seeing the data as a moving thing and not as a static thing makes difference between keeping a state (and possible inconsistencies, race conditions, etc) or simply creating a chain of transformation from THE single source-of-truth until to the screen, with no intermediate state in between. Combine, RxSwift and other Reactive frameworks are great first steps for that. If you're familiar with MVVM, I propose you to start thinking about your VMs no longer as a class or struct, but as a pair of functions with no state. One of these functions will handle the user's input and convert it into model actions, that eventually mutate your single source-of-truth. In response to state mutations, your VM reacts to that and simply converts state into screen instructions (flat struct with Strings for textfields, Booleans for toggles, all Locale and L10n applied, etc). If you manage to do that without keeping intermediate state, you have a lightweight redux. If you check what those "store projections" are, you're gonna realise that they are 2 functions exactly as I described. Data is only moving through the ViewModel or View, never stored.

Regarding your further questions, you're mostly correct:

Your last point is the only misconception. There's no callback for the actions you dispatch from Middleware. They are basically fire-and-forget. AfterReducer or before reducer (default one) is to enable you to check, from the middleware, how was the state before and after the CURRENT action to be handled by this middlware. So if middleware is intercepting startMonitoringFavourites, it's very likely that before or after reducer the state will be the same (unless startMonitoringFavourites itself changes some boolean in main state, like isMonitoringFavourites). But this is not typically the case. However you may want to check the response for actions triggered by your middleware, right? So if your middleware dispatched newFavouriteAdded, the same middleware can also observe newFavouriteAdded and AfterReducer confirm that the newFavourite was indeed added to the state. Sometimes this is useful, sometimes not.

SwiftRex has an option that no other redux framework has (at least not to my knowledge), that your Middleware is generic over 2 Action Types: Input and Output. The actions it dispatches, the actions it intercepts. They can relate to the same Action type, or to different one, this is totally up to you.

luizmb commented 3 years ago

To be clear, imagine you have ReducerA, ReducerB, ReducerC, MiddlewareA and MiddlewareB. Action newFavouriteAdded arrives. It will:

If MiddlewareA or B dispatches new actions, regardless if this happens before or after the reducer, these new actions will arrive at the store only on the next RunLoop, so these new actions won't race against newFavouriteAdded. They are FIFO, so somehow the order of your Middlewares matter, but not that much because they are probably dispatching in other queues anyway.

rustproofFish commented 3 years ago

@nvisual - I've stolen your app (sorry!) and added a persistence layer with associated Middleware. It's a work in progress (CRUD almost done...) but it's here ,https://github.com/rustproofFish/SwiftRex-ToDo-Persisted/tree/main> if you want to have a peek. Comments, etc are very welcome. Thanks for your help to date.

npvisual commented 3 years ago

@Nvisual - I've stolen your app (sorry!) and added a persistence layer with associated Middleware. It's a work in progress (CRUD almost done...) but it's here ,https://github.com/rustproofFish/SwiftRex-ToDo-Persisted/tree/main> if you want to have a peek. Comments, etc are very welcome. Thanks for your help to date.

Great ! That's what it's for.

I took a quick look at the PersistentStoreMiddleware. Here are a few comments (no particular order) :

InputActionType : if you want to make this Middleware a little more generic you might want to avoid using AppAction and rather use some input signal that's specific to your middleware, and then translate your AppAction into the Middleware's own InputActionType via the inputActionMap (see example).

This way you can map your .appLifecycle actions (which could potentially change from one app to the next) into your Middleware-specific actions.

Redundant translation : if you use the above suggestion you can avoid the translation you're doing here (by passing actions around). And you can directly call your .connectToStore and .subscribeToChanges without having to interpret the AppLifecycle actions.

Protocol definition in PersistentStoreMiddleware : this might be purely a personal preference, but I would stick the PersistenceService definition with the persistence middleware (and maybe provide a default implementation with it ?) so you can extract that middleware out eventually and reuse it down the road somewhere else.

@luizmb might have a different perspective on this, so I would rely on his input !

HTH.

luizmb commented 3 years ago

@Nvisual - I've stolen your app (sorry!) and added a persistence layer with associated Middleware. It's a work in progress (CRUD almost done...) but it's here ,https://github.com/rustproofFish/SwiftRex-ToDo-Persisted/tree/main> if you want to have a peek. Comments, etc are very welcome. Thanks for your help to date.

Great work! That's really nice!

I agree with the points that @npvisual mentioned, and if you want me to do a lot of nitpicking I may add some other comments, nothing really critical, it's not about right or wrong, it's more about personal preferences so please feel free to ignore if you disagree. :)

(it's not clear to me what's from the original code and what's your change because I don't see clearly the git diff now, so it's gonna be an overview review).

So, these are some of the points I could complain about, but nothing is wrong, just some tips that could help you in 1) tests or 2) replacing Realm for other providers in the future. Once I heard: "people say your should do DI so you can replace your database, but who ever does that?" as an argument against Dependency Injection. Well, I'm old enough in this business to have replaced many databases (remember Parse?) and see apps dying because they were not prepared to do it, and second you want to replace your database in tests, always, and a TDD mindset only happens with full DI, so that's why I'm hitting too much on this topic. :) Although you could replace whole middleware, and this itself is a testable way, testing the middleware itself is really important because a lot of your business logic will be there, and separate from side-effects makes it testable.

luizmb commented 3 years ago

Re-reading maybe some things are not totally clear, I'm writing in a hurry (lately I'm always in a hurry), so please tell me the bullet points that are not very clear and I improve my writing on them.

rustproofFish commented 3 years ago

@npvisual - thanks (again). That's really helpful and you've address a few queries I had regarding architecture:

InputActionType : if you want to make this Middleware a little more generic you might want to avoid using AppAction and rather use some input signal that's specific to your middleware, and then translate your AppAction into the Middleware's own InputActionType via the inputActionMap (see example).

Thanks for the example. I was worried about allowing the Middleware to access AppAction in it's entirety. @luizmb made the same point. Your example should take care of this issue nicely.

Protocol definition in PersistentStoreMiddleware : this might be purely a personal preference, but I would stick the PersistenceService definition with the persistence middleware (and maybe provide a default implementation with it ?) so you can extract that middleware out eventually and reuse it down the road somewhere else.

Fair point. My code is a bit scrappy and needs a clean up. This is on the to-do list ;-)

rustproofFish commented 3 years ago

@luizmb...

Great work! That's really nice!

I think you are being too kind :-) It works but it's not exactly elegant...

  • There's a Persistence Reducer that performs NSLog. Being very very veeeeery annoying person, of those types that are not very popular in parties, NSLog is a side-effect and putting this in the Reducer makes the Reducer impure. :D

Ah yes. That was left over from when I was debugging something. Must have missed it...

  • PersistentStoreMiddleware needs Input to be AppAction because of the Lifecycle stuff. This is a common problem I also face from time to time. To avoid that PersistentStoreMiddleware becomes coupled to your AppAction because of that, what I usually do is keeping PersistentStoreMiddleware only working with specific action and state types...

I was going to implement the suggestion made by @npvisual which should resolve this.

  • publisher.assertNoFailure() should probably become a publisher.replaceError(with:) or catchErrors, and then you dispatch an action of type .gotError(Date(), error: ..., whilePerforming: originalInputAction)....

I've been very lazy by using .assertNoFailure as I wanted to get the core functionality in place. For production I do need to make sure error cases are handled appropriately. That will likely be in the next iteration when I've had a think about how I want to handle this (graceful recovery and/or fallback strategy is going to be the fun bit I think)

  • I don't see where you call cancelSubscription. But in any case I recommend that you cancel everytime you go backgrounded...

That is the plan - not implemented yet but very good to know I'm on the right track as I was wanting to check my ideas for this.

  • Your persistence middleware doesn't know ANYTHING about Realm, that's great! To be on the safe side just remove import RealmSwift and that way you know you won't be tempted to use Realm in there :)

LOL! Will do.

  • I don't like too much calling the struct as DTO and namespacing it to be under the realm's TaskObject. In fact, TaskObject should be private and very hidden from everything, ideally in a separate framework where nobody can use it by mistake, while your DTO should be public and in your AppState. This is the value type you want to distribute everywhere, give it a good name and remove from Realm-specific object namespace.

Good point - my adherence to good practice isn't always perfect but renaming is simple enough. I've not yet started working with frameworks, etc so something to look into. I'd like to use SPM but have found it irritating and a time-suck if I'm honest as the integration with Xcode isn't fully-featured.

  • Public interface of PersistanceService protocol should expose no Realm stuff. Currently we have some Realm objects in there, but ideally it should "speak" DTO for input and output. And AnyPublisher types, of course. Not sure about the NSPredicate thing, although CoreData also use it, maybe there will be other data storage providers that won't use NSPredicate. It's a native Cocoa thingy, I know, but still it's very tight to certain providers. You could have a "Filter" struct bridge that you control as a type-safe value-type object and only inside your protocol implementation you make your Filter to become a NSPredicate thingy, so the public interface will be composed only by types you control (and Combine, but Combine is generic enough)

Again, being quick and dirty with NSPredicate. I'd just use a closure ordinarily for filtering, sorting, etc plus or minus some custom operators but Realm doesn't accept these so bridging may be required to make the app entirely agnostic regarding the persistence framework.

So, these are some of the points I could complain about, but nothing is wrong, just some tips that could help you in 1) tests or 2) replacing Realm for other providers in the future. Once I heard: "people say your should do DI so you can replace your database, but who ever does that?" as an argument against Dependency Injection. Well, I'm old enough in this business to have replaced many databases (remember Parse?)...

Oh yes... :-( Personally I rather like DI (I used it a lot when I was writing Java) so will probably go down this path by default.

Thanks for the comments @npvisual and @luizmb - really instructive. I think I'm finally getting there! :-)

luizmb commented 3 years ago

You're on the right track! I'm really looking forward to see the final app!

rustproofFish commented 3 years ago

@luizmb, @npvisual - I think I've gone as far as is appropriate for an experimental project and have implemented almost all of the recommendations made. Would you mind having a quick skim and let me know if there's anything outstanding or glaringly awful? ;-)

One a related subject, I presume it's considered to be acceptable to keep derived state or view-specific state contained within the related View using @State? It would seem to be disproportionate to move all of these transient object into the App State even considering the "single source of truth" principle.

luizmb commented 3 years ago

Hi @rustproofFish

I will check it asap. Regarding the @State thing, I see rare cases where a State only belongs to the View and you don't want to pass this through the Store. Remember, anything that doesn't go through the store can't be used for Logging or Analytics, so all these "local" truth will be living in a silo that, in case of failure, will be at your blind spot. Some @Gesture for drag gesture could be local to avoid the "cost" of going through the store (actually the cost is zero, a bunch of reducer functions that are synchronous and pure, basically zero overhead there, but once the CurrentValueSubject changes we depend on Combine to propagate that, and although Combine does its best to perform this in the current run loop, this not always happens depending on iOS version and other things, so it would be a case in favour of local state). If the @State can be calculated from the Store's State, it should be part of view model. If not, maybe your Store's State is missing something. Still, you are free to use redux in the level you choose, therefore some violations could be helpful to make you go faster, but please be careful and notice that keeping @State in sync with the rest is more error-prone than well-known reducers.

luizmb commented 3 years ago

And also, sooner or later your PO will ask you for Analytics on that "view-only" property and you may be tempted to add Firebase to the View, which will definitely break all the "no side-effects outside middleware" rule.

npvisual commented 3 years ago

@rustproofFish : I am closing this as I think it's been resolved, but I have referenced it in the README because it has some valuable information.