yonekawa / SwiftFlux

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

[Proposal]Store should emit Changed event only #12

Closed yonekawa closed 8 years ago

yonekawa commented 8 years ago

This is proposal with breaking changes.

Currently, developer can specify Store's event type by typealias to Event. I think that it should not notify to view from store because event type is store's state.

before

struct TodoStore: Store {
    enum TodoEvent {
    case Created
    case Updated
    }
    typealias Event = TodoEvent
    let eventEmitter = EventEmitter<TodoStore>()

    init() {
        ActionCreator.dispatcher.dispatch(Create.self) { (result) in
            switch result {
            case .Success(let value):
                self.state = value
                self.eventEmitter.emit(.Created)
            }
        }
        ActionCreator.dispatcher.dispatch(Update.self) { (result) in
            switch result {
            case .Success(let value):
                self.state = value
                self.eventEmitter.emit(.Updated)
            }
        }
    }
}

after

struct TodoStore: Store {
    init() {
        ActionCreator.dispatcher.dispatch(Create.self) { (result) in
            switch result {
            case .Success(let value):
                self.state = value
                self.emitChange()
            default:
                break
            }
        }
        ActionCreator.dispatcher.dispatch(Update.self) { (result) in
            switch result {
            case .Success(let value):
                self.state = value
                self.emitChange()
            default:
                 break
            }
        }
    }
}
mortyccp commented 8 years ago

This will make the view restricted to be React.js style. The React.js implementation is ComponentKit which is Obj-c++ and Few.swift. ComponentKit will not be supporting Swift in the short term and Few.swift is not ready for production[Source]

I think this should be a choice up to the developers. Forcing the store to be only able to emit ChangeEvent, will make developer cant make specific optimization for view update in some cases.

yonekawa commented 8 years ago

@soapsign Yeah, got it. I would like to implement at next release(v0.5.0).

waveyus commented 8 years ago

For react.js style, at event of an change in the store, regardless action type, the view updates to get latest state. It doesn't care how/where the state was mutated by action. It always renders the view from current state.

If store emits change with action type, then developers can make 'optimization' based on action type ( Create , Update or Delete ) which break react style and mutate the view accordingly.

There are benefits for listeners to know what changed the store instead of "just changed". For large benefit, the listener should have that piece of payload data the action acted on. (For instance, if you want to partially update view at the event of Create. You can just append the new data at the end of view). If it's not available, then view has no choice but just reload the latest state, which make no sense to know the action type of the change. ( or just prompt user "Your todo created" ?)

But there are obvious benefits for store just emits emitChange(). Then the view doesn't need to subscribe every action type in the store even though it just reloads the data (get latest state) in any of them.

self.todoStore.eventEmitter.listen(TodoStore.Event.Fetched) { () in
        self.tableView.reloadData()
    }
    self.todoStore.eventEmitter.listen(TodoStore.Event.Created) { () in
        self.tableView.reloadData()
    }
    self.todoStore.eventEmitter.listen(TodoStore.Event.Deleted) { () in
       self.tableView.reloadData()
   }

So maybe we provide both self.emitChange() and self.eventEmitter.emit(.ActionType) if developers want one way or another?

I prefer emitChange(). If we use a local db as state persistence, we can always do partial udpate in action itself (before emitting change event) , then the view just listen to one event and always loads the latest state from local db. The view is going to be really light and small without dealing with each action type.

mortyccp commented 8 years ago

If we only have ChangeEvent, what is the best approach for use to animate the state change. For example, a button to be animated if its state change from disabled to enabled.

yonekawa commented 8 years ago

@soapsign

If we only have ChangeEvent, what is the best approach for use to animate the state change. For example, a button to be animated if its state change from disabled to enabled.

Yes, I know it is difficult problem. If we manage animation state, it will emit too many events to view.

Just an idea, how about separate store to manage animation state?