klarna / electron-redux

Use redux in the main and browser processes in electron
MIT License
743 stars 94 forks source link

v2 does not work with redux-observable #285

Closed sneljo1 closed 3 years ago

sneljo1 commented 3 years ago

Describe the bug This is a separate ticket to highlight the second issue I was having with V2 and redux-observable briefly mentioned in #260 . The issue I was having, was that for some reason I had to re-arrange my enhancer and middleware, otherwise my dispatched actions from the main process weren't picked up by the renderer process. I am not sure if this should be documented or resolved.

This change is related to the API change to remove replayActionRenderer. Listening for and dispatching actions from the middleware instead of after the createStore has been created, like the V1 api, is causing the dispatched actions to not get picked up by the middleware. The renderer process is receiving an action, the reducer are being called because of it, but not the middleware. I think it is because when the enhancer is after the middleware in the compose function, the dispatch in the enhancer isn't setup yet with the middleware.

I see 3 solutions here. Re-introduce replayActionRenderer, ~document the necessity to have the enhancer as the first item~, or fix it somehow inside the enhancer itself so that the simple API remains and the compose order is not so error-prone. But I am not sure if this is possible.

To Reproduce I may be able to provide a sample project if needed, but it will take some effort to set up.

My setup: Main process -> some action -> epic middleware -> action -> dispatched to renderer -> middleware not triggered (neither redux-logger or redux-observable)

sneljo1 commented 3 years ago

I think my observation from yesterday may not be 100% correct. I think the solution that works for me, is to put both main & renderer listeners for the ACTION type after the createStore outside the enhancer. Just re-arranging the enhancers and middleware is not sufficient.

Nantris commented 3 years ago

@Superjo149 did you get this working? I think this might be closely related to our issue #294

sneljo1 commented 3 years ago

@Slapbox Yes, I seem to have been through the same rabbit-hole. I am actually currently working on a proper fix as we speak. I have found some hacks to get it working, but I am now working on a proper solution. However, it will change the API for the library significantly.

matmalkowski commented 3 years ago

@Superjo149 It would be great if you could share an overview of the direction of the fix. We could have the discussion on the API too - since redux-saga & redux-observable are rather big part of the ecosystem, we wouldn't want to release a final version without support for those

sneljo1 commented 3 years ago

@matmalkowski I was doing more research around this yesterday, and I landed on this section in the redux-devtools-extension. I think this library has similar issues with the order of the enhancers/middleware. This issue slipped into electron-redux due to the "simplification" of the API, changing the position in the enhancer execution stack.

They state a few times, that if you are not using any different enhancers or middleware, you can just use the devToolsEnhancer. But when you are using other enhancers or middleware, you should be using the composeWithDevTools to wrap your other enhancers.

In this regard, I am taking a similar approach. I am adding a new method composeWithStateSync, which resembles the composeWithDevTools. Both methods solve the issue with the order by basically wrapping the compose and making sure the enhancers are in the correct position in the chain. You'll still be able to just use the stand-alone enhancer. But the same thing applies here. You'll need the custom compose function if you have other enhancers.

I actually just got it working as well.

Nantris commented 3 years ago

Edit: I wrote this before the two most previous messages, might be outdated.

@Superjo149 thanks for your speedy reply! I think I'm going to be following your work because as much as I love the new API, I fear it may never be made compatible with Sagas and Observables.

Is this the repo you're planning to do your work on? https://github.com/Superjo149/electron-redux/commits/alpha

Is your thinking that replayActionRenderer will need to be reintroduced?

Would you be able to share these "hacks" that you got Observables working with? I've come pretty far along this path and really want to avoid having to reverse course now. It seems like the main problem we're seeing is that new Redux actions triggered from our sagas aren't firing.

(although I may have just gotten that working, if it is working, I couldn't say why)

sneljo1 commented 3 years ago

@Slapbox I will just create a PR to this repo as soon as I am happy with it. No need to start yet another fork. That repo is just to create a PR from.

With the above approach,replayActionRenderer will not need to be introduced. Something extra will just be introduced to replace the current enhancer.

And no, I just removed those hacks while now preparing these changes, so I will not be able to share these anymore.

matmalkowski commented 3 years ago

@Superjo149 So seems like the way to go might be the redux-devtools-extension way, and we can keep the current API for the no enhancers usecase, while introducing also the composeWithStateSync that would come as a bonus for more complex scenarios. It doesn't sound like a significant lib API change TBH

Nantris commented 3 years ago

Whoops, it looks like it "works" for me because I'd forgotten I tried this lunacy of running applyMiddleware both before AND after the electron-redux enhancers are included.

Shockingly it seems like this works well enough for us to continue our work for now, though I am sure that something will explode soon and I'd never use this in production:

  middleware.push(sagaMiddleware);
  enhancers.push(applyMiddleware(...middleware)); // Apply middleware

  if (scope === 'renderer') {
    enhancers.push(require('electron-redux').rendererStateSyncEnhancer());
  }
  if (scope === 'main') {
    enhancers.push(require('electron-redux').mainStateSyncEnhancer());
  }

   enhancers.push(applyMiddleware(...middleware)); // Apply middleware AGAIN
sneljo1 commented 3 years ago

@matmalkowski No, indeed, that may have been over exaggerated, wasn't sure yet 1 hour ago if more changes were needed. Will send a PR your way as soon as I get things cleaned up. 🙂

Nantris commented 3 years ago

@Superjo149 that's awesome what you've found and what you're working on, thanks so much for sharing. Looks like I have some reading to catch up on.

And @matmalkowski thank you so much for all your work on this project and being committed to having both Sagas and Observables working.

The rapid fire developments here are super encouraging!

matmalkowski commented 3 years ago

:tada: This issue has been resolved in version 2.0.0-alpha.8 :tada:

The release is available on:

Your semantic-release bot :package::rocket: