klarna / electron-redux

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

Make sure v2 works with applyMiddlaware #260

Open matmalkowski opened 4 years ago

matmalkowski commented 4 years ago

Right now the only way to use the syncMain is to pass it as storeEnchancer to store creator:

const store = createStore(reducer, syncMain)

Its not possible to use it with applyMiddleware (probably need to applyMiddleware + compose - but it might be more

matmalkowski commented 4 years ago

This should be supported with compose, just needs to be documented. We also need a migration guide since it might be slightly confusing if someone was using middleware before.

Will leave it here as a reference for missing doc

sneljo1 commented 3 years ago

Just did a testdrive with the alpha. I'm guessing this is the reason why actions in the renderer process are not picked up by redux-logger and redux-observable middleware, correct?

EDIT: I've applied a quick fix in the electron-redux library itself, and this solved my issues. But I'm eager to learn what the proper solution is. Or does the library need to be adjusted for this either way?

matmalkowski commented 3 years ago

@Superjo149 I haven't had a chance to test drive it with redux-logger so not sure what is the issue - can you share the quick fix you did? 🤔

matmalkowski commented 3 years ago

I just tried adding the `redux-logger as middleware to the renderer store on our tests, it works as expected:

import { createStore, compose, applyMiddleware } from 'redux'
import { reducer } from '../../counter'
import { rendererStateSyncEnhancer } from 'electron-redux'
import logger from 'redux-logger'

const middlewareEnhancer = applyMiddleware(logger)
const composedEnhancers = compose(middlewareEnhancer, rendererStateSyncEnhancer())

const store = createStore(reducer, composedEnhancers)
sneljo1 commented 3 years ago

I had to do 2 things, but I do not fully understand why. I believe that indeed the actions are dispatched in the other process, but they weren't picked up by the middleware when coming from the other process. I think this issue is specifically for actions which need to be dispatched and run inside the middleware of the renderer process, but I have to do more testing to be sure. With the fixes, they do dispatch and get handled correctly. Firstly, for the compose, I had to place my middleware first, then the other enhancers. Then, I patched the package like this image

sneljo1 commented 3 years ago

I did some more investigating, and I do think this is the way to go instead of adding a 3rd argument to the createStore function. This is how it is also done in redux-devtools and redux itself. I would be happy to submit a PR for this if you want. But it still wants me to add my enhancers before my middleware otherwise it does not work.

matmalkowski commented 3 years ago

I did some more investigating, and I do think this is the way to go instead of adding a 3rd argument to the createStore function. This is how it is also done in redux-devtools and redux itself. I would be happy to submit a PR for this if you want.

@Superjo149 This change makes sense, and it would be a good thing to follow same principles as other libs - if you would like to, feel free to open a pull request for that change 👍🏻 Please use the alpha branch as the base, so it would target the v2

sneljo1 commented 3 years ago

@matmalkowski Allright, I will. I also did some more investigating into my other issue. It is possibly related specifically to the usage with redux-observable. I hope I can provide some input and possible fix for this soon.