New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented redux-thunk
-like action creator
#240
Conversation
ReSwift/CoreTypes/Store.swift
Outdated
if let action = action { | ||
self.dispatch(action) | ||
callback?(self.state) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
ReSwift/CoreTypes/Store.swift
Outdated
|
||
open func dispatch(_ asyncActionCreatorProvider: @escaping AsyncActionCreator) { | ||
dispatch(asyncActionCreatorProvider, callback: nil) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
func dispatch<ReturnValue>(_ actionCreator: ActionCreator<ReturnValue>) -> ReturnValue | ||
|
||
@discardableResult | ||
func dispatch<State: StateType, ReturnValue>(_ actionCreator: StatedActionCreator<State, ReturnValue>) -> ReturnValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line Length Violation: Line should be 120 characters or less: currently 121 characters (line_length)
@@ -1,5 +1,8 @@ | |||
import Foundation | |||
|
|||
public typealias ActionCreator<T> = (_ store: DispatchingStoreType) -> T | |||
public typealias StatedActionCreator<S: StateType, T> = (_ store: DispatchingStoreType, _ getState: @escaping () -> S) -> T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line Length Violation: Line should be 120 characters or less: currently 123 characters (line_length)
|
||
@discardableResult | ||
func dispatch<ReturnValue>(_ actionCreator: ActionCreator<ReturnValue>) -> ReturnValue | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
@@ -21,4 +24,10 @@ public protocol DispatchingStoreType { | |||
return type, e.g. to return promises | |||
*/ | |||
func dispatch(_ action: Action) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
Hi! I love this, it feels closer to what the original Redux provides (even though the implementation is different because the differences between JavaScript and Swift, in Redux this is done in a separate middleware and not built-in in the core library). I have a question: what's the purpose of having two different action creator type(alias) ( Other than that, 💯. Maybe the library maintainers have more to say about the code itself. |
@danielmartinprieto, originally, the only purpose of |
Yup, that |
There is only one problem (in terms of type safety): this implementation allows us to dispatch any @danielmartinprieto Regarding two typealiases: as Dan Abramov wrote somewhere, accessing state in action creator is something like anti-pattern, thus, for those who want Also i'll reference #214 here, because this PR provides another way of handling this. |
Also I didn't touch tests and didn't wrote comments yet, cause if this modification of such core functionality as ActionCreators doesn't fit library maintainers 'view' it will be just a waste of time 😄 But if you like it guys, i'll be happy to finish this PR till merge. |
Yeah, true, I've read that as well, although everyone does it... apparently it's a better approach to access the state inside a middleware, instead. Anyway, I think having this action creator is nice, I didn't see the problem where the two state types (the action creator's one and the store's one) don't match, maybe that can be improved. Thanks for taking the time of implementing this, and answer my questions! |
Hey @mpsnp, thanks a lot for taking the time to implement this! I'm definitely interested in refining the async action creator API and this looks like a good starting point for a discussion! I have a few raw thoughts about this PR. Sorry if they aren't expressed super well, but I wanted to get the first version out quickly instead of waiting a week to refine my comment 😉 Current state in Redux: Redux states the following interface for action creators:
So async and sync action creators are dispatched the same way. For action creators returning sync actions the store dispatches the created action immediately, for ones returning async actions a transform happens in the middleware that eventually leads to dispatch of an action. Current state in ReSwift: ReSwift currently provides two separate dispatch interfaces: one for action creators and another for async action creators. Both have a strict interface that allows the action creators to create exactly one action, either synchronously or asynchronously. Suggested Change: Your suggested change replaces the distinct dispatch functions with a single more flexible one. Pros/Cons of change in my View: Pros:
Cons:
TL;DR Next Steps |
Hi, @Ben-G! About the API discoverability problem that you mention, in my opinion the API resulting of these changes is clearer now, and partially solves what's mentioning on #150. I still think that we should be able to dispatch only plain I totally see your point about the callbacks, though, and I also think that needs further discussion. |
- Changed middleware signature - Extracted helper dispatch functions to extension - Added thunk middleware and helper dispatch funcs
ReSwift/CoreTypes/Store.swift
Outdated
callback?(self.state) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
ReSwift/CoreTypes/Store.swift
Outdated
@@ -117,49 +116,26 @@ open class Store<State: StateType>: StoreType { | |||
" (e.g. from multiple threads)." | |||
) | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
ReSwift/CoreTypes/Middleware.swift
Outdated
return actionCreator(store, callback) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
ReSwift/CoreTypes/Middleware.swift
Outdated
if let actionCreator = actions.first as? ActionCreator<Any> { | ||
return actionCreator(store) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
return result | ||
} | ||
|
||
public func dispatch<CallbackParam>(_ asyncActionCreator: @escaping AsyncActionCreator<CallbackParam>, callback: @escaping (CallbackParam) -> Void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line Length Violation: Line should be 120 characters or less: currently 153 characters (line_length)
} | ||
callback(param) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
next(param) | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
} | ||
return result | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
ReSwift/CoreTypes/Store.swift
Outdated
callback?(self.state) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
ReSwift/CoreTypes/Store.swift
Outdated
@@ -117,49 +116,26 @@ open class Store<State: StateType>: StoreType { | |||
" (e.g. from multiple threads)." | |||
) | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
ReSwift/CoreTypes/Middleware.swift
Outdated
return actionCreator(store, callback) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
ReSwift/CoreTypes/Middleware.swift
Outdated
if let actionCreator = actions.first as? ActionCreator<Any> { | ||
return actionCreator(store) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
return result | ||
} | ||
|
||
public func dispatch<CallbackParam>(_ asyncActionCreator: @escaping AsyncActionCreator<CallbackParam>, callback: @escaping (CallbackParam) -> Void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line Length Violation: Line should be 120 characters or less: currently 153 characters (line_length)
} | ||
callback(param) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
next(param) | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
} | ||
return result | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing Whitespace Violation: Lines should not have trailing whitespace. (trailing_whitespace)
Hey @Ben-G, @danielmartinprieto ! I've rethought this PR with respect to @Ben-G cons, and decided to extract functionality related to func asyncActionCreator(store: DispatchingStoreType, completionHandler: @escaping (Int) -> Void) {
DispatchQueue.main.asyncAfter(deadline: .now() + 1.0) {
store.dispatch(SomeAction())
completionHandler(10)
}
} As well as normal action creators, which allow us to return any type, for example promises: func actionCreator(store: DispatchingStoreType) -> Promise<Void> {
return firstly {
doAnyAsyncStuff()
}.then {
store.dispatch(SomeAction())
}
} And here is example usage of action creators declared above: let store = Store<AppState>(reducer: rootReducer, state: nil, middleware: [thunkMiddleware])
store.dispatch(asyncActionCreator) { result in
print("Async with result = \(result)")
}
store.dispatch(actionCreator).then {
print("Done!")
}.catch { (error) in
print(error)
} Also this implementation even allows us to extract DownsidesAs you see, this required to change type of Middleware param, and add func dispatch(_ params: Any...) -> Any Maybe it is a pro too, because such change brings more flexibility in future and as i've shown in |
I think it's a great idea to move this to a middleware, and even though we're not trying to replicate 1 to 1 the original Redux implementation because the differences between the two languages, it's nice if they follow the same principles if possible. This made me think that maybe we don't need to change at all the public API of the current ReSwift version, or even better, we can simplify it removing both The idea would be to provide (maybe, as you said before, as another micro framework, as it is the original redux-thunk) both a new protocol and a middleware: protocol Thunk: Action {
func body(dispatch: @escaping DispatchFunction, getState: @escaping () -> State?)
}
let thunkMiddleware: Middleware<State> = { dispatch, getState in
return { next in
return { action in
switch action {
case let thunk as Thunk:
thunk.body(dispatch: dispatch, getState: getState)
default:
next(action)
}
}
}
} This means that the dispatch function can keep its current, simple signature This also means that the thunks you create in your code are super flexible, as they can contain the information you want (data, callbacks...), as the current What do you think? Example of usage, as in your example before: struct SomeAction: Action {}
struct MyThunk: Thunk {
let completionHandler: (Int) -> Void
func body(dispatch: @escaping DispatchFunction, getState: @escaping () -> State?) {
DispatchQueue.main.asyncAfter(deadline: .now() + 1.0) {
dispatch(SomeAction())
self.completionHandler(10)
}
}
}
let myThunk = MyThunk { someInt in
print(someInt)
}
store.dispatch(myThunk) |
@danielmartinprieto great idea of wrapping it into a Thunk protocol. And i absolutely agree that it'd be better to extract this functionality out of ReSwift core. But this construction is not so comfortable in my opinion: let myThunk = MyThunk { someInt in
print(someInt)
}
store.dispatch(myThunk) Also, i don't see in your proposal possibility to return any value out of the dispatch as in original redux (question to community: do we need it?). What I recommend is: to change middleware func from public typealias Middleware<State> = (@escaping DispatchFunction, @escaping () -> State?) -> (@escaping DispatchFunction) -> DispatchFunction to public typealias Middleware<State> = (DispatchingStoreType, @escaping () -> State?) -> (@escaping DispatchFunction) -> DispatchFunction Because:
But this still doesn't give us possibility to return any value from dispatch method. That's why proposal of second change: // from
public typealias DispatchFunction = (Action) -> Void
// to
public typealias DispatchFunction = (Action) -> Any And in order to keep Public API the same and don't break existing functionality: protocol DispatchingStoreType {
@discardableResult
func dispatch(_ action: Action) -> Any
} But this will give us full control over 'Dispatch - Middleware - return value' chain. And possibility to write helper dispatch methods of any signature (probably with generic return value) In summary it's kind of crossed product of @danielmartinprieto last comment and lighter version of mine. It is much more architectural change and i suppose we guys need more discussion to find the best way. |
Hey! About the Thunk constructor, a function is definitely cooler, but, TIL, Swift won't allow us to extend a non-nominal type (aka we can't make a function conforms to It's true I didn't think about Anyway, the original Apart from these, the takeaways of this whole thread for me are:
|
Hey @danielmartinprieto!
I guess yes, just in order to return the same type of action which it received 😄 by simply: func dispatch<A>(_ action: A) -> A where A: Action But if we go this way, we need to save result of thunk in action itself somehow in order to return it in convenience dispatch function. Also regarding thunk constructor, i guess you misunderstood me, i mean some kind of this: public typealias Thunk<T, State> = (_ store: DispatchingStoreType, _ getState: () -> State?) -> T
public typealias AsyncThunk<T, State> = (_ store: DispatchingStoreType, _ getState: () -> State?, _ next: (T) -> Void) -> Void
protocol ThunkAction: Action {
associatedtype Result
associatedtype State
var body: ActionCreator<Result, State> { get }
var result: Result? { get set }
}
struct ConcreteThunkAction<T, S> {
let body: ActionCreator<T, S>
var result: T?
}
extension DispatchingStoreType {
func dispatch<T, State>(_ thunk: Thunk<T, State>) -> T {
let resultAction = dispatch(ConcreteThunkAction(body: thunk))
guard let result = resultAction.result else { ... }
return result
}
} And usage will be the same as in this comment, but with simplified SummarisingSo summarising whole that thread, correct me if i'm wrong. We need to:
Haha, long comment! 😆 So, @Ben-G waiting your approval, if everything ok, we'll move forward 🚀 |
Hi, @mpsnp. I thought you wanted to return something random (and therefore the About the thunk library, I'd like to have something less complicated, more like my So summarising, your summary lgtm, just note the comment I made above about the dispatch function. |
Are there plans to merge this soon? |
@danielmartinprieto with the ReSwift-Thunk lib on the horizon, can you have a second look at this and see if the new lib could scavenge some things from this PR? |
I’ve reviewed the PR and I can’t find anything specific that you can’t do right now with the basic ReSwift types and middleware. ReSwift-Thunk would be just some help to those that prefer having their side effects closer to where they’re dispatched, but we decided to put it in a micro framework because it’s just a utility and not something foundational. The main difference here would be the returning type of the dispatch function. As I mention in previous comments, the original Redux library returns (for convenience) the same action dispatched, but then custom middleware could change this and a) I don’t think it provides any specific benefit and b) it’s hard to type in a “swifty” way (without having to return Any, which I don’t think it’s cool). Also, I know this was the case here in ReSwift and it got changed, so I would vote for leaving it like that. I hope this helps. Sent with GitHawk |
Sounds good to me. I'll close the issue for now and we move with the µ-framework as it is. Sorry to turn down your first PR, @mpsnp! Maybe you can find a place to continue work in the ReSwift-Thunk library itself :) |
This implementation allows us to write action creators with return value, just as redux-thunk middleware do. For example it allows composition of action creators this way:
What do you think about this?