manaflair / redux-batch

Enhance your Redux store to support batched actions
174 stars 12 forks source link

Doesn't seem to be calling connect after actions are batched? #1

Closed colbycheeze closed 7 years ago

colbycheeze commented 7 years ago

Not sure exactly what is happening, but none of the components update their props after a set of batched actions are dispatched. The store has all of the updated data, but not the components.

This happens with arrays passed OR even normal dispatches after hooking up the reduxBatch middleware via

  const store = createStore(rootReducer, composeEnhancers(
    reduxBatch, applyMiddleware(...middleware), reduxBatch
  ), initialState)
arcanis commented 7 years ago

Hm that's weird, I just checked and listeners seem to be correctly called, @connect should have no issue being notified of store updates. Can you try using the 0.0.2 version I just pushed? It should fix a small issue where too many notifications were sent, maybe it will also solve your problem.

Noitidart commented 7 years ago

Excuse the off topic @colbycheeze - Thanks @arcanis for your fast-awesome maintenance/support - I have been thinking super hard for like 2 months now to pick up a batching lib but saw so many issues. After seeing your support and reading the whole topic you based this on - https://github.com/reactjs/redux/pull/1813#issuecomment-227623481 - I am going to give this a go :D <3 Thanks for it!

arcanis commented 7 years ago

Thanks, let me know if it works for you :) I've been using it on a project of mine with success (cf https://github.com/manaflair/todo-demo for a small demo project that shows how to use it), but sometimes subtle differences in environments (or dependencies updates) can break things. I hope it will be fine!

jimbol commented 7 years ago

Has this issue been resolved? I think I am still seeing it.

Edit: To clarify, notifyListeners is getting called but subscribers aren't actually notified. Effectively, these actions don't get called until a not-batched action updates subscribers.

arcanis commented 7 years ago

Would you happen to have a minimal testcase I could use to reproduce the issue? I haven't yet experienced this bug myself, so it would definitely help. If not, I'll try to make one on my side later this week.

arcanis commented 7 years ago

I've tried the following code, and it seemed to work fine, can you check on your end?

let redux = require('redux');
let reduxBatch = require('@manaflair/redux-batch').reduxBatch;

let store = redux.createStore((state = {}, action) => {
    console.log(action);
    return state;
}, reduxBatch);

store.subscribe(state => {
    console.log('been notified!');
});

store.dispatch({ type: 1 });
store.dispatch([ { type: 1 }, { type: 2 } ]);
{ type: '@@redux/INIT' }
{ type: 1 }
been notified!
{ type: 1 }
{ type: 2 }
been notified!

If it does, then I'll need some guidance to reproduce the exact issue. If it doesn't, please tell me which version of redux and redux-batch you're currently using (yarn list / npm list should give you this information).

jimbol commented 7 years ago

Yea by the looks of it, it should work. Perhaps it's having issues in combination with redux-saga. I'll try to find some time to investigate. We switched to redux-batched-action for now.

arcanis commented 7 years ago

I have fixed an edge-case issue that could be the cause of the behaviour you've experienced, but without test case it's hard for me to be sure (cf https://github.com/manaflair/redux-batch/commit/8c17160430a837e2dc9542a4f5a9aacbf42105bb). If you happen to try again, let me know if it works better with the release 0.0.3 :)

I'll close this issue for the time being, but feel free to reopen it if it's not resolved.

jimbol commented 7 years ago

Thanks for looking into it!

arcanis commented 7 years ago

Bad news: I believe the bug's still there. Good news: I have a testcase \o/ cf #4

arcanis commented 7 years ago

I just released a v0.1.0 that should fix this bug.