Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure v2 works with applyMiddlaware #260

Open
matmalkowski opened this issue Sep 29, 2020 · 8 comments
Open

Make sure v2 works with applyMiddlaware #260

matmalkowski opened this issue Sep 29, 2020 · 8 comments
Labels
Projects
Milestone

Comments

@matmalkowski
Copy link
Collaborator

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 matmalkowski added bug 🐛 v2 All issues related to v2 typescript labels Sep 29, 2020
@matmalkowski matmalkowski added this to the 2.0 milestone Sep 29, 2020
@matmalkowski matmalkowski added this to To do in v2 via automation Sep 29, 2020
@matmalkowski
Copy link
Collaborator Author

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
Copy link

sneljo1 commented Dec 5, 2020

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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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
Copy link

sneljo1 commented Dec 7, 2020

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
Copy link

sneljo1 commented Dec 7, 2020

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
Copy link
Collaborator Author

matmalkowski commented Dec 8, 2020

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
Copy link

sneljo1 commented Dec 8, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
v2
  
To do
Development

No branches or pull requests

2 participants