Codecov Report

Merging #595 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #595      +/-   ##
==========================================
+ Coverage   88.18%   88.28%   +0.09%     
==========================================
  Files          49       49              
  Lines         364      367       +3     
==========================================
+ Hits          321      324       +3     
  Misses         43       43

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de05776...6e92f41. Read the comment docs.

Hi, Thank you.
It was created as some kind of simplified redux, no callback there https://redux.js.org/docs/api/Store.html#dispatch
We have no analog of middlewares to call side effects on action complete etc.
I personally dont like the idea to provide api extension the only use case it has is to call some side-effect and somehow broke similarity to redux dispatch. In all such cases you can call sideeffect in handler with just action (and compute state where it needed using the same logic as in store)

Thank you for the feedback!

While I definitely see that withReducer is a simple redux analogue, I still think that something that models this functionality of dispatch could really benefit recompose.

The store's reducing function will be called with the current getState() result and the given action synchronously. Its return value will be considered the next state. It will be returned from getState() from now on, and the change listeners will immediately be notified.

I havent used withReducer ;-) @wuct what are you think about, may be in absence of middlewares this is not so bad as I wrote?

It'd be great to have middlewares like in redux to manage side effects cleanly. 😄 But I imagine that adding support for middleware would be tough, given that the withReducer HOC is basically emulating the redux store with the main difference being that the HOC is bound to React's API internally.

There are already some differences in the dispatch implementations because of this. The dispatch from withReducer is technically asynchronous since it relies on and returns a setState call internally. So, it does not return the same thing as dispatch from redux as is.

So I ask - in lieu of middleware support and given that the dispatch implementations are seemingly going to be slightly different - whether or not an API extension like this is an acceptable way to handle this simple side effect type of logic with the withReducer HOC.

My suggestion would be to pass in a callback to the payload, e.g.

https://github.com/azz0r/fedsimulator/blob/ppvs/src/actions/data.js#L9
https://github.com/azz0r/fedsimulator/blob/ppvs/src/reducers/roster.js#L24

But I believe this is considered bad practise, you'd be expected to use componentWillReceiveProps to check the nextProps change then trigger your "callback".

@therealparmesh @istarkov I don't like the idea to extend API surface for side effects, too. However, as @therealparmesh said, we've already provided this kind of surface in withState, and if we consider withReducer as another setState wrapper, it makes sense that we create the same surface for it. Moreover, I don't think we will support for middlewares, so it's okay for me to have this callback.

@azz0r IMHO it's not a good idea because impure reducers are harder to test and trace code.

@wuct @istarkov Great! Thank you for taking the time to look at this. What should the next steps be?

Looks like it is good to be shipped. @istarkov any thought?

Any update on if/when this will be released? It would be helpful in a project where I'm currently using recompose.

There are workarounds for sure, but I'm not a fan of having an impure reducer either and componentWillReceiveProps is distant from the code that is dispatching the action. Having this would be a great convenience.

Oops sorry,
@wuct I see nothing bad, please merge.

Thank you @therealparmesh