angular-redux / ng-redux

Angular bindings for Redux
MIT License
1.16k stars 177 forks source link

Adjust store creation to resolve issue with redux-first-router #166

Closed rshackleton closed 7 years ago

rshackleton commented 7 years ago

There is an existing issue when using ng-redux with redux-first-router (RFR).

When first loading the page RFR dispatches an action for the matched route. Due to how ng-redux creates the store this action is not passed to the registered middleware. I tested creating the store separately and passing this into ng-redux using the method outlined at the end of #19 but this then caused the digest loop to not be updated with the new state (due to the digestMiddleware not being added to the same middleware chain).

These changes to store creation mean the store is created as per the redux documentation and the middleware is initialised at the same time as the other store enhancers, thus preventing the action being swallowed.

This is very much an edge case and this was a quick fix for us so it may not meet your guidelines for PRs.

AntJanus commented 7 years ago

@rshackleton Thanks for the work on this! I haven't run into the same issue you're talking about but can you explain to me how this fix works?

I'm having a hard time understanding how passing in a store that already has middleware built in, and then adding to that middleware chain.

I'll merge this in right after, I just want to make sure I understand it. Again, thanks for working on this 👍

rshackleton commented 7 years ago

It seemed to be related to when the middleware and enhancers were registered and in what order they were composed.

Calling applyMiddleware(...)(finalCreateStore)(_reducer) meant the middleware weren't added before the redux-first-router's enhancer had dispatched the first action, whereas calling createStore(...) with the middleware and enhancers composed works.

It is worth me clarifying that I'm not providing an existing store anymore, I'm using ng-redux as per the documentation and this PR fixes the original issue I had. Providing an existing store had a separate issue related to the digest middleware.

AntJanus commented 7 years ago

:+1: sounds good! Merging in.

maxlapides commented 6 years ago

@rshackleton Wow thanks so much for this fix. Just spent a few hours digging deep into RFR and ng-redux trying to figure out what was going wrong. I ended up looking at the ng-redux code on Github, scratching my head cuz it looked totally correct, only to discover that that's because you just fixed the issue and the code I'm testing is the latest on npm! Anyway, thanks again, this is exactly the fix I needed at exactly the right time. And if it's helpful for anyone else, here's a workaround until this fix is pushed to npm:

$ngRedux.createStoreWith(
  combinedReducers,
  [],
  [
    rfrEnhancer,
    applyMiddleware(
      rfrMiddleware,
      myMiddleware1,
      myMiddleware2
    )
  ]
)