Feature/action linker #98
Conversation
|
I suggest you show a snippet of how to use the linker here. |
|
You want it here in the PR? |
There was a problem hiding this comment.
Beside the feedback, I'd like to have a snippet of code of usage as @lucaquerella asked because I'm not 100% sure about the additional struct that holds the array of tuples.
I'm not saying is wrong, it is just to me to picture it
Moreover Travis reports errors with the MacOS tests, can you take a look?
| All the conversion between Action.Type and String is handled internally, | ||
| to leverage the type checking system. | ||
| */ | ||
| let links: [String : [LinkeableAction.Type]] |
There was a problem hiding this comment.
Shouldn't it be private?
| The ActionLinker is responsible to check if there are actions that must be issued after a source actionis dispatched. | ||
| For each of this actions, it checks if the conditions are met: if yes, they are dispatched. | ||
| */ | ||
| struct ActionLinker { |
There was a problem hiding this comment.
I don't agree.
A user should not know that there is an ActionLinker.
The store init it, you just have to pass the links
| var tmpLinks: [String: [LinkeableAction.Type]] = [:] | ||
|
|
||
| /// Create the dictionary for a fast and efficient access to the actions. | ||
| for tuple in links { |
There was a problem hiding this comment.
I think you can use reduce here
| - parameter oldState: state of the application before the source action is applied | ||
| - parameter newState: state of the application after the source action is applied | ||
| */ | ||
| func dispatchLinkedActions(_ dispatch: (Action) -> Void, forAction source: AnyAction, oldState: State, newState: State) { |
There was a problem hiding this comment.
It should be public
moreover I think that
dispatchActions(for State:oldState:dispatch)is a better name
You always have to think about the usage
so
linkedActions.dispatchActions(for newState, oldState: oldState, dispatch: self.dispatch)|
|
||
| /// Create the dictionary for a fast and efficient access to the actions. | ||
| for tuple in links { | ||
| let actionType = String(reflecting: tuple.source) |
There was a problem hiding this comment.
Since you use this in two places, I'd move it into a private method
func stringName(for action: Action) -> StringThere was a problem hiding this comment.
I thought about it but it will change a line for a line so no code is saved... but i'll do that
There was a problem hiding this comment.
It is not about saving lines of code. It is about avoid to update it somewhere and forget to update it in another place
There was a problem hiding this comment.
ok, make sense.
| func dispatchLinkedActions(_ dispatch: (Action) -> Void, forAction source: AnyAction, oldState: State, newState: State) { | ||
| let linkedActions = self.links[ String(reflecting:(type(of: source)))] | ||
|
|
||
| if let actions = linkedActions { |
There was a problem hiding this comment.
Use guard to avoid indentations
| @@ -109,11 +112,12 @@ open class Store<StateType: State> { | |||
| - parameter dependencies: the dependencies to use in the actions side effects | |||
| - returns: An instance of store configured with the given properties | |||
There was a problem hiding this comment.
Update the documentation
| self.dispatchFunction(action) | ||
|
|
||
| self.actionLinker.dispatchLinkedActions(self.dispatch, forAction: action, oldState: oldState, newState: self.state) |
There was a problem hiding this comment.
You should move this into performDispatch
| func testCreationOneSourceActionOneLink() { | ||
| let actionLinker = ActionLinker(links: [ActionLinks(source: BaseAction.self, links: [LinkedAction1.self])]) | ||
| XCTAssertEqual(actionLinker.links.count, 1) | ||
| XCTAssertEqual(actionLinker.links[String(reflecting:BaseAction.self)]!.count, 1) |
There was a problem hiding this comment.
Instead of String(reflecting) you should use the method provided by ActionLinker (which could be also static, btw).
You don't want to test how things are stored, but just that they are stored
| } | ||
| } | ||
|
|
||
| struct LinkedAction4: LinkeableAction { |
There was a problem hiding this comment.
stub structs should be fileprivate
|
@lucaquerella We have a |
|
yes I meant here, |
|
It could be, yes. Maybe it is better since we don't add too many things to the store. let actionLinks: [ActionLinks] = ...
let middleware = LinkedActions.middleware(for: [actionLinks]) |
|
Yesterday, when we talked about it, we decided to avoid middlewares if possible to remove the possibilities to make black magic or mess up with the architecture... But maybe I'm missing something. |
Why
Add the ActionLinker to dispatch a set of
Actions that depends on another oneChanges
The
Storerequire alinks: [ActionLinks]new parameter to be initialized.I keep the compatibility with the convenience empty init.
Tasks