Skip to content
This repository has been archived by the owner on Dec 15, 2018. It is now read-only.

Store enhancer might not be needed #215

Open
villesau opened this issue Jul 29, 2017 · 0 comments · May be fixed by #216
Open

Store enhancer might not be needed #215

villesau opened this issue Jul 29, 2017 · 0 comments · May be fixed by #216

Comments

@villesau
Copy link

villesau commented Jul 29, 2017

Hi, thanks for the redux-little-router, it's great!

I'm trying out if redux-little-router could be used in project i'm working on. The problem is that in order to be able to have side effects on redux-little-router actions in middlewares, the enhancer needs to be the first one in the enhancer chain. Some libraries have strong opinions about this. For example ngRedux don't allow having any enhancers before middlewares: https://github.com/angular-redux/ng-redux . Also redux documentation recommends against having other enhancers before middlewares which makes the decision of ngRedux understandable as it just follows the best practices from redux docs: http://redux.js.org/docs/api/applyMiddleware.html#tips

When i looked into code, I saw that the enhancer only adds one function, matchRoute into store, which actually is not used anywhere. When removing matchRoute, the enhancer doesn't anymore enhance the store; It just connects to it and uses it. If the enhancer does not enhance the store in any way, it shouldn't be in enhancer chain in the first place. Instead I suggest to change the enhancer to less magical connect -function where you pass the store in. I think this would actually also convey the meaning of the enhancer better, as it's purpose is to connect the router into redux store. It also makes the enhancer code slightly simpler. What do you think?

Since the router would connect to ready made store, it would eliminate the risk that middlewares wouldn't capture actions dispatched by the router.

I also wrote a PR to demonstrate what I mean: #216

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants