samiskin / redux-electron-store

⎋ A redux store enhancer that allows automatic synchronization between electron processes
MIT License
375 stars 32 forks source link

Seems to break immutable.js #18

Open nickyhajal opened 8 years ago

nickyhajal commented 8 years ago

Hi there,

I'm using Immutable.JS in my state and it seems that if I have electronEnhancer() in the renderer, I get the data fine as a basic object but it breaks Immutable's functions.

For example, getState().auth.toJS() works without the electronEnhancer but with it I get toJS is not a function.

samiskin commented 8 years ago

Unfortunately much of this library is dependent on the use of plain JavaScript objects, even though it really shouldn't. I'll likely have to do a few API-breaking changes so I'm going to put this off for a bit until I start working on a v0.4.0.

hhff commented 7 years ago

Just hit this too! Let me know if there's anything I can do to help @samiskin !

samiskin commented 7 years ago

@hhff Unfortunately I likely won't be able to do significant work on this project for a while 😞, possibly until the very end of this year, but I'll always be open to PRs, so feel free to try implementing this on your own if its a pressing issue. I haven't used immutable.js in a real project before, only played around with it, so I'm not too sure what the challenges would entail.

I assume a cheap solution code-wise (but perhaps costly performance-wise) would be tailoring it specifically for immutable and just scattering toJS() and fromJS() calls everywhere. The main issue is that I'm not sure what the details of passing immutable structures through ipc is.

arieldf commented 7 years ago

@hhff @nickyhajal Any of you managed to get this working with Immutable? Any tips?

I have started to modify the codebase to see whether I'll manage. I'll keep you updated! We have a big project and need to move over to Immutable.

hhff commented 7 years ago

@arieldf it's not possible I believe (without hacky rehydration stuff), as this project uses IPC to keep everything in sync. IPC can't transmit non-primitives.

arieldf commented 7 years ago

@hhff thanks for your quick response. Are there any alternatives that you are aware of?

nickyhajal commented 7 years ago

The project I was using this on is currently on my back burner but here's how I got around it (not using redux-electron-store).

In the rendered window:

const store = configureStore(initialState);
store.subscribe(() => {
  const state = store.getState();
  ipcRenderer.send('state', {
    settings: state.settings.toJS(),
    auth: state.auth.toJS(),
  });
});

In the electron main.js

let state = {};
ipcMain.on('state', (ev, updatedState) => {
  state = updatedState;
  // do other stuff
});

This is obviously pretty basic but it worked for my needs. Hope it helps!

arieldf commented 7 years ago

Thanks for the response @nickyhajal - valuable input

hhff commented 7 years ago

heck ya! that would work!

samiskin commented 7 years ago

Might be able to get it to work within this project using https://github.com/intelie/immutable-js-diff and https://github.com/intelie/immutable-js-patch

The core of this project is just that when an action happens, diff the state, send the diffs to the other processes, then apply the diff from the other processes.

Theres just a couple more optimization parts (the "filter" stuff) to not send updates to a window unless the window actually cares about that update.

arieldf commented 7 years ago

@samiskin thanks a lot for the suggestion. I'll revert when I manage to solve this

arieldf commented 7 years ago

@samiskin This could be a viable solution, but I did not successfully manage yet. When will the latest release be available on NPM? I would like to work with the latest code base. Thanks again

arieldf commented 7 years ago

@samiskin Ref. the above question, there is a second alternative which is to have the root state split between an immutable state-tree and a plain JS state-tree, where everything that should be in sync with main and renderer using this lib goes into the plain JS state. What would be the best practice to accomplish this with electron-redux-store? Basically I would like a different state/reducers in main and renderer, but with a plain JS state property in common that is equal.

samiskin commented 7 years ago

@arieldf It might be able to work if you just set the filter property on the renderer to ignore the immutable part of the state. Theres also an excludeUnfilteredState option you could set to true so that it wouldn't show any of the ipc-ified immutable objects in the renderer's state.

I'll try to publish to NPM sometime today

arieldf commented 7 years ago

Thanks for the input @samiskin!

UPDATE: using a different reducer and initial state in the main solved it

///// I hope there is a small change to the code I can do so that this will indeed work as the below is what I get following the above (both for 0.4 and 0.6):

Doing as you proposed with the filter I get the following errors: I. The previous state received by the reducer is of unexpected type. Expected argument to be an instance of Immutable.Collection or Immutable.Record with the following properties: ... II. Uncaught TypeError: inputState.withMutations is not a function

adding the excludeUnfilteredState = true I get: Uncaught Error: Values in the sink must be another object, function, or true at keys.forEach (http://localhost:3000/dist/bundle.js:91146:13) at Array.forEach (native) at fillShape (http://localhost:3000/dist/bundle.js:91138:14) at http://localhost:3000/dist/bundle.js:90950:51 at ../node_modules/redux/lib/applyMiddleware.js:38:19 at http://localhost:3000/dist/bundle.js:97030:211 at ../node_modules/redux/lib/applyMiddleware.js:38:19 at :1:5448 at Object.createStore (../node_modules/redux/lib/createStore.js:65:33) at Object.configureStore [as default] (http://localhost:3000/dist/bundle.js:121161:25)

samiskin commented 7 years ago

@arieldf I published master under the beta tag. Going to try to add more tests soon and then publish to latest.

initFabian commented 6 years ago

Was this issue ever resolved? I'm encountering the same same issue when using Record from immutableJS

samiskin commented 6 years ago

Nope never ended up being able to work on it. I haven't really used Immutable.js at all so I'm not too familiar on the best ways to handle it 😞 .