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

fix(#285): add composeWithStateSync to resolve issues with enhancer order #296

Merged
merged 5 commits into from
Feb 16, 2021
Merged

Conversation

sneljo1
Copy link

@sneljo1 sneljo1 commented Feb 9, 2021

As discussed in #285. Added a new composeWithStateSync function which mimics the composer function and should replace that function when using more than one enhancer. When using one enhancer, the other API is still the same.

To accommodate this, I have also added 2 more things:

  • preventActionReplay was added to the options because in extensionCompose() we re-use the stateSyncEnhancer, but we do not want the actions to start forwarding there because then it will not pick up the ones dispatched by the middleware. We rather want them to start forwarding in the last part of the chain, hence forwardActionEnhancer. This is also possible with denyList, but I thought this was cleaner. WDYT?
  • To simplify the compose function, I have opted to check whether in renderer or main process, inside the lib. With that, I have also added a helper function stateSyncEnhancer which can wrap around the process specific enhancer to do the same thing.

Will see if I can write a test for it tomorrow. Do you want me to go ahead and update the README already as well @matmalkowski ?

@CLAassistant
Copy link

CLAassistant commented Feb 9, 2021

CLA assistant check
All committers have signed the CLA.

@matmalkowski matmalkowski linked an issue Feb 9, 2021 that may be closed by this pull request
@matmalkowski
Copy link
Collaborator

Thanks for the contribution!

We will try to take a look at the change in next few days, you could also add some info into the README file, like you mentioned. I've also started a docs page for the project (/docs folder) - you might as well add a paragraph there or just add a placeholder so we can cover it later.

@slapbox if you have some time, you might also take a look at this branch & try it with your issue #294 and redux-saga

@matmalkowski matmalkowski added the v2 All issues related to v2 label Feb 10, 2021
@Nantris
Copy link
Contributor

Nantris commented Feb 10, 2021

I'd really like to give this a try!

Unfortunately my TypeScript noobishness may be getting in the way.

I cloned the repo, checked out the alpha branch, and ran yarn && yarn build, but I get an error like rollup has no idea what TypeScript is. Am I overlooking something obvious?

image

@Nantris
Copy link
Contributor

Nantris commented Feb 10, 2021

Friggin' Windows... Builds fine on Linux, will update soon.

@Nantris
Copy link
Contributor

Nantris commented Feb 10, 2021

@Superjo149 there's no code changes needed on our end to take advantage of these changes, right?

Unfortunately it doesn't seem to remedy the issue with Redux Saga.

I took a look at how to setup RxJS and it looks very similar to setting up sagas. Can you share the part of your store configuration where you're applying your enhancers and middlewares so I can compare it with our setup?

I also tried importing composeWithStateSync and replacing our compose with that, but that leads to Error: Attempted to register a second handler for 'electron-redux.INIT_STATE_ASYNC'

@sneljo1
Copy link
Author

sneljo1 commented Feb 10, 2021

@Superjo149 there's no code changes needed on our end to take advantage of these changes, right?

Unfortunately it doesn't seem to remedy the issue with Redux Saga.

I took a look at how to setup RxJS and it looks very similar to setting up sagas. Can you share the part of your store configuration where you're applying your enhancers and middlewares so I can compare it with our setup?

I also tried importing composeWithStateSync and replacing our compose with that, but that leads to Error: Attempted to register a second handler for 'electron-redux.INIT_STATE_ASYNC'

@slapbox There is some kind of code change required, yes. I will properly document this tomorrow along with the tests. But this is the jist. The error sounds like your are using both the old API and the compose function.

Current approach:

Just selected the main process here as an example.

const middleware = applyMiddleware(countMiddleware)
const enhancer = compose(middleware, mainStateSyncEnhancer())
const store = createStore(reducer, defaultState, enhancer)

Approach with multiple enhancers (Saga middleware,... etc) using composeWithStateSync:

Note that when using composeWithStateSync, neither mainStateSyncEnhancer or rendererStateSyncEnhancer are required as this will be handled by the compose function. This in line with how redux-devtools-extension works. Also note that this needs to be done in both processes.

const middleware = applyMiddleware(countMiddleware)
const enhancer = composeWithStateSync(middleware)
const store = createStore(reducer, defaultState, enhancer)

@Nantris
Copy link
Contributor

Nantris commented Feb 10, 2021

@Superjo149 thanks so much for your help, that makes perfect sense now and I can confirm that everything seems to work properly!

Not only a fix, but a further streamlining of an already great API; wow, great work!

PS: Is there a way to get Redux Devtools working with this too?

@Nantris
Copy link
Contributor

Nantris commented Feb 10, 2021

Oh, you can just wrap compose functions around compose functions. Never knew that, so that's the solution for Redux Devtools in case anyone is lurking this thread.

Copy link
Collaborator

@matmalkowski matmalkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks exciting! I believe this is the right direction, and will get us closer to releasing stable v2 version, thanks a lot for all the hard work you put into figuring out how to implement this 👍🏻

yarn.lock Outdated Show resolved Hide resolved
src/utils/actions.ts Show resolved Hide resolved
src/composeWithStateSync.ts Outdated Show resolved Hide resolved
@sneljo1 sneljo1 marked this pull request as ready for review February 15, 2021 07:30
@matmalkowski matmalkowski merged commit cce8018 into klarna:alpha Feb 16, 2021
@matmalkowski
Copy link
Collaborator

🎉 This PR is included in version 2.0.0-alpha.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sneljo1
Copy link
Author

sneljo1 commented Feb 20, 2021

Glad I could contribute to this 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @alpha v2 All issues related to v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v2 does not work with redux-observable
4 participants