yonekawa / SwiftFlux

A type-safe Flux implementation for Swift
MIT License
237 stars 19 forks source link

ActionCreator and Action #15

Closed waveyus closed 8 years ago

waveyus commented 8 years ago

First, thanks for the work creating the framework. I got some questions regarding the design of ActionCreator and Action.

According to official recommendation from Flux, if we have asynchronous API call, we should put it in ActionCreator and the actionCreator would pass the result with the action to dispatcher.

https://github.com/facebook/flux (See diagram) http://facebook.github.io/react/blog/2014/07/30/flux-actions-and-the-dispatcher.html http://www.code-experience.com/async-requests-with-react-js-and-flux-revisited/

However, in SwiftFlux, the "ActionCreator" is more like a central place holding the default dispatcher. It delegates the actual invocation of dispatching to individual action itself. (inside action itself)

This way, the only place for these API calls is inside each action itself where it needs to dispatch the result with action once the data returned from the asynchronous call.

For instance, ActionCreator.invoke(TodoAction.Fetch()) the fetch occurs inside the fetch action itself

let todos = [ Todo(title: "ToDo 1"), Todo(title: "ToDo 2"), Todo(title: "ToDo 3") ]

My understanding is that action is small and light weight (only its type and its payload data) But this way, the action would become an really heavy item passing around (to stores)

The heaviest components should be ActionCreator (backend calls) and Stores (client logic).

Is this observation valid?

If so, it would be needed to move the responsibility out of Action to ActionCreator. (Maybe we still holds a central dispatcher but have each individual ActionCreator created for its purpose with the dispatcher passing in at initialization )

If not, please advise. Thanks again!

yonekawa commented 8 years ago

Yes, SwiftFlux’s Action serve both as Action and ActionCreator of original flux. I think this is name problem. ActionCreator may be ActionInvoker right. I would like to consider about naming of each modules.

However, I think you can create heavy action currently. For example.

struct HeavyAction: Action {
    typealias Payload = (HeavyResult, ComplexResult)
    let anyOtherBigParams: BigParamsType

    func invoke(dispatcher: Dispatcher) {
       SomeHeavyProcess(anyOtherBigParams) { (heavyResult) in
          SomeComplexLogic(result) { (complexResult) in
              dispatcher.dispatch(self, result: Result(value: (heavyResult, complexResult)))
          }
    }
}

let bigResult = someBigProcess()
ActionCreator.invoke(HeavyAction(anyOtherBigParams: bigResult))

Any problem?

mortyccp commented 8 years ago

I think having invoke(...) insides of Action makes the logic of providing the payload a specific type of Action limited. If invoke(...) is outside of Action, developers can have better flexibility. For example in doing unit testing, developers can just provide the fake payload with ease and need not to mock the Action type.

waveyus commented 8 years ago

I see 2 problems as ActionCreator and Action merged into one.

Here is Facebook official description of action and its creator:

"an action — an object literal containing the new fields of data and a specific action type. We often create a library of helper methods called action creators that not only create the action object, but also pass the action to the dispatcher."

Action is simple as a value carrier object so that anything interacts with it would only cares about its type and data. ( like a piece of message)

  1. Anything receives action doesn't care about its inner service calls or creator logic. But SwiftFlux action came as a larger object anyway because it got two souls (action and creator). Not sure what impact would be on performance and memory footprint.
  2. Let's continue with your example above. Unless we don't need any tests and just make everything concrete implementation inside the action, SomeHeavyProcess() shall come from external module or API. Let's say it comes from HeaveyProcessAPI.SomeHeavyProcess(). For easy test, we want to pass a protocol of it (HeaveyProcessAPIType) for dependency injection.

Now we have 2nd parameter:

let HeavyProcessAPI: HeaveyProcessAPIType

anyOtherBigParams is indeed some data came in for consumption. But HeavyProcessAPI is not a piece of data anymore.

From this point, anything triggers action in view would see this parameter as part of initialization. (Even though they only need to pass bigResult for example above)

ActionCreator.invoke(HeavyAction(anyOtherBigParams: bigResult))

Or pass nothing for action Fetch and only "Title" for action Create as in demoApp todolist).

ActionCreator.invoke(TodoAction.Fetch())
ActionCreator.invoke(TodoAction.Create(title: "New ToDo"))

2.1 This confuses user or ( they would attempt to supply HeavyProcessAPI instance in view where action was invoked)

2.2 As action creator, it needs to make sure that HeavyProcessAPI has an instance before it calls Invoke(); So the current "ActionCreator" (the ActionInvoker for new naming) has to provide it somewhere with itself.

There is only one ActionInvoker in the app so we can't put all these external api in one file for different modules.

If we leave it in Action as it is, can the ActionInvoker do this? (Invoker inevitably knows inner details of an action )

action(HeavyProcessAPI).invoke(self.dispatcher)

or is there a better way?

At least, I can see we have a central ActionInvoker (or none?), but individual action has its own ActionCreator (may be shared with same group of actions) with all those externals packed inside.

Any thought? Thanks for your reply.

mortyccp commented 8 years ago

In the official implementation of flux, ActionCreator is not within the library source code. I think the essential pieces that let developers following SwiftFlux with be Dispatcher, EventEmitter, Store, Action. So I think SwiftFlux should not include ActionCreator into the library. If we want to keep ActionCreator in the library, I think we should use Dependency Injection style which the dispatcher should be pass into the ActionCreator. In this developers can have better flexibility. Also, I suggest it should be Utilis instead.

waveyus commented 8 years ago

Most of us would appreciate the benefits as a framework having some protocol or base class (ActionInvoker or ActionCreator) out-of-box for user to get started or provided as guideline.

Flux is a pattern. The official implementation is for web. So it doesn't have to be 100% mapped with that as we move in Swift.

But it would be nice to have capability opt-out or overriding the default implementation.

waveyus commented 8 years ago

Soapsign I think we should use Dependency Injection style which the dispatcher should be pass into the ActionCreator.

Waveyus Maybe we still holds a central dispatcher but have each individual ActionCreator created for its purpose with the dispatcher passing in at initialization

We're on the same page on this one. The dispatcher could be singleton. Each ActionCreator accept it as initialization parameter.

I think that is all the value the ActionInvoker brought to table ( passing a shared dispatcher) but all done by itself. If an ActionCreator can decide its own dispatcher ( the singleton), ActionInvoker can retire since there is nothing to invoke. ActionCreator dispatches an action right there by itself and an Action doesn't need to know what is a dispatcher and why it needs to accommodate a func Invoke inside itself.

yonekawa commented 8 years ago

@waveyus Sorry, maybe I don't understand correctly of your problems.

  1. Anything receives action doesn't care about its inner service calls or creator logic. But SwiftFlux action came as a larger object anyway because it got two souls (action and creator). Not sure what impact would be on performance and memory footprint.

I think there is no problem. SwiftFlux's Action is original flux's action creator to be sure but Dispatcher dispatches Payload data to stores. if divided into action and creator, I think it will be nothing but object is split .

  1. Let's continue with your example above. Unless we don't need any tests and just make everything concrete implementation inside the action, SomeHeavyProcess() shall come from external module or API. Let's say it comes from HeaveyProcessAPI.SomeHeavyProcess(). For easy test, we want to pass a protocol of it (HeaveyProcessAPIType) for dependency injection.

I think that you can use any technique to create payload data to dispatch to stores. Because SwiftFlux’s Action is super simple protocol.

I would like to know this in detail.

action(HeavyProcessAPI).invoke(self.dispatcher)

Does action return Action object? How about implementation of action if HeavyProcessAPI has async call?

waveyus commented 8 years ago

The SwiftFlux action protocol expects any object implemented serving two purposes.

  1. Carry data: payload, error, and type as its name ( struct Fetch: Action)
  2. Dispatch itself via Invoke()

Use the demo app for example,

ActionCreator.invoke(TodoAction.Fetch()) was triggered at TodoListViewController

Let's say we have an external asynchronous call AsyncAPI.GetTodos(), according to the demo app, it has to be called inside func Invoke() of Fetch action.

struct Fetch: Action {
       typealias Payload = [Todo]
        func invoke(dispatcher: Dispatcher) {
        let todos = AsyncAPI.GetTodos()
        dispatcher.dispatch(self, result: Result(value: todos))
 }
}

If we want to do dependency injection with AsyncAPI as AsyncAPIType, we need to pass an instance somewhere in the initialization.

If we look at the moment we call this line at view ActionCreator.invoke(TodoAction.Fetch())

Where should we initialize AsyncAPI? Thanks for your reply.

waveyus commented 8 years ago

I would like to know this in detail.

action(HeavyProcessAPI).invoke(self.dispatcher)

Does action return Action object? How about implementation of action if HeavyProcessAPI has async call?

Please ignore the details. Just trying to say if we don't want to initialize HeavyProessAPI when action was invoked at View ActionCreator.invoke(TodoAction.Fetch()), what would be the alternative option? Thanks.

yonekawa commented 8 years ago

If we want to do dependency injection with AsyncAPI as AsyncAPIType, we need to pass an instance somewhere in the initialization.

If we look at the moment we call this line at view ActionCreator.invoke(TodoAction.Fetch())

Where should we initialize AsyncAPI? Thanks for your reply.

struct Fetch: Action {
    typealias Payload = [Todo]
    func invoke(dispatcher: Dispatcher) {
        let asyncApi = AsyncApi()
        asyncApi.GetTodos() { (todos) in
            dispatcher.dispatch(self, result: Result(value: todos))
        }
    }
}

ActionCreator.invoke(TodoAction.Fetch())

or

struct Fetch: Action {
    typealias Payload = [Todo]
    let apiInstance: AsyncAPIType
    func invoke(dispatcher: Dispatcher) {
        apiInstance.GetTodos() { (todos) in
            dispatcher.dispatch(self, result: Result(value: todos))
        }
    }
}

ActionCreator.invoke(TodoAction.Fetch(apiInstance: AsyncAPI()))

or inject to Fetch by any dependency injection technique.

waveyus commented 8 years ago

Thank you for your prompt reply.

The first is a concrete initialization. The latter we probably want to avoid since Action was invoked at ViewController. This introduced one API reference (or multiple references) in the view anytime you want to invoke an action. (Not good for test. Also now the action not only needs to worry about preparing payload data it carries but also the external dependencies it has to supply for the func Inoke ()).

That's not ideal. I felt there is no proper place for constructor injection. It has to be container.resolve an instance inside action then

struct Fetch: Action {
    typealias Payload = [Todo]
    func invoke(dispatcher: Dispatcher) {
        let asyncApi = container.resolve(asyncApiType.self)!
        asyncApi.GetTodos() { (todos) in
            dispatcher.dispatch(self, result: Result(value: todos))
        }
   }
   }

ActionCreator.invoke(TodoAction.Fetch())
yonekawa commented 8 years ago

@waveyus

Sorry, I don't understand your problem... You can use Typhoon if you need dependency injection. DI is not responsibility of SwiftFlux.

Please give me a Pull-Request if you have any problem about SwiftFlux implementation.

waveyus commented 8 years ago

Thanks. Will do. I'm using Swinject.