Skip to content
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

Store.ActionCreator , Store.AsyncActionCreator and Middlewares #75

Closed
HeMet opened this issue Feb 7, 2016 · 4 comments
Closed

Store.ActionCreator , Store.AsyncActionCreator and Middlewares #75

HeMet opened this issue Feb 7, 2016 · 4 comments

Comments

@HeMet
Copy link

HeMet commented Feb 7, 2016

As far as I understand, all overloads of Store.dispatch that using both Store.ActionCreator and Store.AsyncActionCreator types wrap _defaultDispatch actually. It seems like a job for middlewares, isn't it?

BTW, why does Middleware take references to dispatch and getState separately? Maybe it is worth to extract both from StoreType to some StoreCoreAPIType and pass it by weak reference.

P.S. ReSwift is very interesting framework 👍

@Ben-G
Copy link
Member

Ben-G commented Feb 8, 2016

As far as I understand, all overloads of Store.dispatch that using both Store.ActionCreator and Store.AsyncActionCreator types wrap _defaultDispatch actually. It seems like a job for middlewares, isn't it?

Generally this would be a job for middleware. However, there are two specific reasons we have chosen to implement them as separate methods on the store:

  1. Some of the overloads change the API of dispatch by taking a callback as an additional argument - this cannot be accomplished by middleware
  2. We want to provide some features, such as dispatching ActionCreators or having a way to be notified about the completion of an AsyncActionCreator by default. We don't want that developers need to add middleware for these essential features. That means this library will be more opinionated than Redux, but that was a conscious decision on our end.

An additional note on 2., we are looking for a better default way for async in ReSwift and we'll have the discussion under #64. AsyncActionCreators work for now but we are hoping to find a better solution.

Lastly:

BTW, why does Middleware take references to dispatch and getState separately? Maybe it is worth to extract both from StoreType to some StoreCoreAPIType and pass it by weak reference.

I think in this case it's not worth introducing a new type. We are borrowing the middleware API from Redux (which makes it easier for some folks to understand the mechanism) and I would only want to deviate from it if we can add significant value. I don't think that's the case for this suggestion.

Thanks a lot for contributing your questions & ideas 👍

@HeMet
Copy link
Author

HeMet commented Feb 8, 2016

@Ben-G Thanks for your detailed explanation.

@HeMet
Copy link
Author

HeMet commented Feb 8, 2016

Some of the overloads change the API of dispatch by taking a callback as an additional argument - this cannot be accomplished by middleware

Actually I have a question about this too :) I perfectly understand the value of that callback for asynchronous version of dispatch and don't see any profit to use it on synchronous ones. It leads to pyramid of doom. Is it all about consistency of dispatch overloads?

@Ben-G
Copy link
Member

Ben-G commented Feb 9, 2016

I agree on the latest comment. The callbacks on the synchronous dispatch methods don't make a lot of sense. They were added for consistency but don't add a lot of value. We should likely remove them soon! You are right that they encourage to use the library in an undesirable way.

Ben-G added a commit that referenced this issue Mar 20, 2016
Using ReSwift there should never be a reason for needing a callback upon state update for synchronous dispatches. This was correctly pointed out in #75.
Ben-G added a commit that referenced this issue Mar 20, 2016
Using ReSwift there should never be a reason for needing a callback upon state update for synchronous dispatches. This was correctly pointed out in #75.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants