Closed nick-benoit14 closed 6 years ago
Hi @nick-benoit14, thanks for submitting. Before I review, can you update your additions to all use tabs? Right now it's a combination of tabs and spaces, and is a bit difficult to read.
Thanks again!
@tshelburne Sorry about that. I didn't even realize you were using spaces because my editor makes them 2 spaces wide. Let me know what you think.
Take Care!
@tshelburne Any thoughts?
@tshelburne I would like to use this package in a work project soon. Have you had a chance to look at this code at all?
Thanks for the time, Nick
@tshelburne Made those changes you requested
@nick-benoit14 Thanks for this, published as version 0.2.1!
Hi @tshelburne @nick-benoit14 I have run the following code and encounter infinitely recursive call error,
RangeError: Maximum call stack exceeded
, while dispatching a non-batched action
const doThing = () => ({ type:'DO_THING'})
const doOther = () => ({ type:'DO_OTHER'})
function reducer(state='init', action) {
switch (action.type) {
case 'DO_THING': return 'thing'
case 'DO_OTHER': return 'other'
default: return state
}
}
const store = createStore(
enableBatching(reducer),
applyMiddleware(batchDispatchMiddleware)
)
store.dispatch(doThing())
which's quite understandable. because we push back non-batched action to pass through all the middleware chain again via store.dispatch, which included batchDispatchMiddleware itself.
export function batchDispatchMiddleware(store) {
function dispatchChildActions(store, action) {
if(action.meta && action.meta.batch) {
action.payload.forEach(function(childAction) {
dispatchChildActions(store, childAction)
})
} else {
// here, when we found non-batched action we push them to pass
// through chain again.
store.dispatch(action)
}
}
return function(next) {
return function(action) {
// non-batched action came here again and again.
dispatchChildActions(store, action)
next(action)
}
}
}
All tests passed, because we stub dispatch function, which has obscured recursive manner inside it.
I'm not sure have i missed something. please point me out.
I really appreciated your work. a much respect here. I love Redux and batching in redux is necessarily thing. Really looking forward to your work in middleware or enhancer layer. I have something to do with batching in enhancer layer (batching that can work with other enhancer).
I think i would love to contribute on this topic, if you think i can help and has plan to make this topic to the next level.
Regards Krittin :)
Hey @sinonkt, Thanks for looking into this and giving a thorough bug report. I will do my best to dig into it more as the week goes on. I am not a maintainer, but i'm sure if you came up with a good fix it would be merged.
The one thing I noticed that is unexpected with your example, which may not be having any effect is that you have both enhanced your reducer and applied the batch middleware. I added the batch middleware with the intention of using it without enhancing the reducer. Like I said though, not sure if that is actually changing anything.
Thanks again for the help!
@nick-benoit14 I have tried enhance only your middleware but still no luck here, it seems they didn't even call enableBatching.
Another thing that i have tried is that I modified redux to inject vanilla dispatch function to your middleware via store. All your logic went well as you expected. cause it's skipped the current middleware which cause the loop.
but that is not what redux does, they inject full chain of middleware dispatch function.
@sinonkt Thanks for reaching out - I'm going to open a new issue referencing this as a bug. It's unlikely I'll be able to get to it for a while, so if you come up with a fix, please feel free to submit a PR.
Thanks again!
I'm wondering if batchDispatchMiddleware
is in fact the wrong way to go about solving the issue of interoperating w/ other middleware. As reported by @sinonkt and in issue #23 , actions dispatch successfully, but are no longer batched:
store.dispatch(batchActions([{ type: 'ONE' }, { type: 'TWO' }]);
store.subscribe(() => console.log('update'));
// > update
// > update
// > update
i.e. rather than batching two actions into one, it emits three actions (of type ONE
, TWO
, and BATCHING_REDUCER.BATCH
).
I can't think of any way to solve this that simultaneously batches store emits and allows seamless integration w/ other middleware. But, patterns to get this to work w/ other middleware should be fairly minimal. E.g., to use w/ redux-observable:
export const patchLinkEpic = (action$) => action$.pipe(
// flatten actions if batched
mergeMap((action) => action.meta && action.meta.batch ? of(...action.payload) : of(action))
ofType(SOME_ACTION),
...
);
I may be wrong, though, i.e. if it is in fact possible to emit only once for batched actions and provide no-config interop w/ other middlewares. If it is not possible, then fixing the underlying issue of interop might just be a matter of documenting the patterns--can submit a PR for common 3rd party middleware (redux-observable, sagas, thunk, etc.) if this is the case.
@jameslaneconkling I haven't looked at this in a while, I really only use Cyclejs for state anymore, but your description resonates; perhaps we should look at encouraging use of a set of middleware "helpers" rather than this rough approach.
I would be open to a PR as you suggested, either as documentation of patterns or even containing common 3rd party middleware - but in that case I think they should be separate modules in the package that can be imported and used as a bridge for those cases.
@tshelburne agreed--either good docs or simple adapter utils should solve the problem.
I can take a stab at the Epic adapter, if helpful--see my comment on this issue in redux-observable.
Use Case
Dispatch a batched action, that includes at least one action that triggers an external side effect in another middleware
Solution
Add middleware that will intercept a batched action, and dispatch all of the bulk actions individually. This allows a single user interaction to kick off a batch action, while still decoupling other middleware from the concept of batch actions.