fix: stop making actions of initial renderer async

[sapkra]

Fixes #254

The tests are still broken but it would be great to get some feedback if this approach is fine, before I fix them. I will also add the new id option/param to the docs later.

A full explanation what my PR is doing you can in the linked issue.

[sapkra]

Using my fork with this fix in production for about a month and seems to work completely fine. Would be great to get feedback if this solution is ok like that or if we should search for another one.

[matmalkowski]

@sapkra Hey, sorry for getting back to you so late,

As you might probably noticed, we started working on the v2 rewrite of the library, that includes also a bit of changes about how the logic itself is implemented & exposed. Your proposed solution in the issue about making the renderers identifiable and moving the filtering logic makes sense - so please take a look at the v2 code thats right now on the alpha branch and let us know if you could work on implementing this feature on the new code base :)

[sapkra]

I will have a look at it. Thank you for reaching out.

[sapkra]

@matmalkowski It's already implemented I guess. But with a more dynamic solution so that the user don't have to manually specify the ID anymore.

https://github.com/klarna/electron-redux/blob/f1e1b2b370bb92c60f1319e66e0b60abd243e97a/src/syncMain.ts#L21-L22

Edit: I've tested it and it seems that it's not working correctly. Options for a custom blacklist is also currently missing. I also added it in my fork.

[sapkra]

@matmalkowski Ok, I found the problem why it's not working. First of all, the stuff I added in this PR is almost completely implemented in 2.0.0-alpha.1.

The problem is that the state change I'm doing in my preload script is faster than the replaceState action because it relies on an async callback from the main process. So my state is getting set and immediately reset. It's a problem in the core concept of the rewrite, so currently I have no idea how to fix it in the new codebase.

When logging the actions you can see that they are executed in the wrong order:

Object
  payload: {activeUser: "ckerc42xh00100828ivf1l95s"}
  type: "auth/SET"

Object
  meta: {scope: "local"}
  payload: {activeUser: null}
  type: "electron-redux.REPLACE_STATE"

How I use it:

store/renderer.ts

import { createStore, AnyAction } from 'redux';
import { syncRenderer } from 'electron-redux';

import { AppState, createRootReducer } from './reducers';

window.store = createStore<AppState, AnyAction, unknown, unknown>(
  createRootReducer,
  syncRenderer,
);

preload.ts

import { setActiveUser } from 'reducers/auth';
import 'store/renderer';

window.store.dispatch(setActiveUser('ckerc42xh00100828ivf1l95s'));

[matmalkowski]

@sapkra I was thinking about this, and I'm not sure what is the right way to approach this problem, that seems hard to avoid, since state sync, due to making it even driven, is by nature asynchronous. We cannot wait and delay store creation, as the enhancer have to synchronous.

One way of going about this problem, that could be helpful for some (and maybe you) would be to expose a callbacks that can get executed once the store get synced for example. This way you would be able to dispatch your action to the store after it updated. Thoughts?

[sapkra]

@matmalkowski The approach in v1 using remote.getGlobal('getReduxState'); was working fine. The new async approach broke the code.

[matmalkowski]

@sapkra Yes, but I believe the remote module is disabled by default in electron v10 for security reasons - but there is another way of synchronous communication, that could fetch the state from renderer on demand: ipcRenderer.sendSync

Maybe we could use that instead of remove solution from v1, and make it default (so we don't break the lib behaviour that much compared to v1) and have the async synchronisation as an opt-in option?

[sapkra]

@matmalkowski Yes true, the remote module is disabled by default but I think it's still safe to use it in the preload script. But I'm not 100% sure. But anyway, it seems like sendSync is the better solution. Is the async solution even necessary if it's not working completely right. What are the advantages of this option?

Another small related topic is that I'm having self containing javascript objects in my store which breaks when using JSON.stringify in this library but I don't know if it's even possible to sync such complex stores.

[matmalkowski]

@sapkra

Is the async solution even necessary if it's not working completely right. What are the advantages of this option?

I think it's still a nice thing to have, considering, that sync operation is a blocking one, and I can see how it might slow down the store initialisation when dealing with big / deep store.

Another small related topic is that I'm having self containing javascript objects in my store which breaks when using JSON.stringify in this library but I don't know if it's even possible to sync such complex stores.

This used to support by default custom Map & Set serialization, but I have removed it and made it opt in since its agains the redux design to store non-serializable data in the store. Please take a look at #272 where I removed it, would using this help?

[sapkra]

@matmalkowski

I think it's still a nice thing to have

Ok, I don't think it will help that much because most apps rely on an already initialized store anyway I think. It's you project and if it might helpful for someone it could be implemented.

This used to support by default custom Map & Set serialization

Thank you, I will have a look it it.

I'm closing this PR because there is still the open issue and this PR has no change anymore which will contribute to this topic. Are you planning implement the sync logic?

[matmalkowski]

@sapkra Yes, I will create issue now for it, and maybe tackle it on the weekend 👍🏻