samiskin / redux-electron-store

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

Cannot set property `deleted` of undefined error in 0.3.5 #16

Closed davej closed 8 years ago

davej commented 8 years ago

I get this error in 0.3.5 but it works fine in 0.3.4. The corresponding line is here (line 52): https://github.com/samiskin/redux-electron-store/blob/d2e9ab2c6fdede530edea5d3d7ab576a26548d64/src/electronRendererEnhancer.js#L52

I logged out action.data and it is indeed undefined. I can give you more detail on the kind of action that triggers this error if needed.

samiskin commented 8 years ago

Ah I hoped this wouldn't happen. In 0.3.5 I basically took off some checks that didn't make sense at the time, since it felt like a hack. Hopefully we'll find out why this would happen now.

Could you give more detail on what happened / how to reproduce it?
The only case I can think of would be if the root reducer is somehow called a second time during a single dispatch. Does that happen?

davej commented 8 years ago

Ok, so this is a bit of a long one but this scenario is where I'm seeing the error.

I have a hidden background worker window where I do CPU intensive work in my Electron app. This is because if I do it in the main GUI window then it lowers the frame rate and causes janky UI.

In short the process is (1) create an action with type worker/* (2) action ends up being re-created in the worker window with the worker/ prefix removed.

In detail it works like this.

On the UI window:

Then on the worker window:

function performAction(action) {
  // clone existing action
  const clonedAction = { ...action };
  // delete the `source` property, otherwise redux-electron-store will complain
  delete clonedAction.source;
  store.dispatch(clonedAction);
}

At the point that the clonedAction is dispatched, it looks like this:

{"type":"INSTALL_PLUGIN","pluginString":"coffee-script@^1.10.0"} 

But it happens on all actions that follow the outlined pattern above.

davej commented 8 years ago

I should add that reducers are the same on both the UI and worker window.

samiskin commented 8 years ago

I made a very small change that might fix it, since it should fix the case where the reducer is called within a dispatch. Could you try with v0.3.6-beta?

Could you also check if it still complains if you don't delete clonedAction.source, since that was one of the things I tried to fix in v0.3.5

davej commented 8 years ago

Excellent, 0.3.6 works perfectly for me. Big THANKS for your help!

davej commented 8 years ago

... and I also no longer need to delete the source property. :)

samiskin commented 8 years ago

Awesome :+1:

samiskin commented 8 years ago

@davej : Right now redux-electron-store actually runs actions twice, in the main electron thread as well as in the render. This is because the way it was initially used at Slack was having it only doing the actual processing of actions in the main thread, and everything being treated by the renderers as being asynchronous. Having it be done on both was only added when publishing this library, so that it was synchronous by default. We never used worker windows, so I never considered the case where expensive tasks would be desirable on a renderer process only.

Would it help to make it so that the actions will only be processed by the renderer that fires it, such that then whatever updates occurred will be forwarded to the main process, which can then forward it to all the windows as usual? This'll remove extra computations and let worker windows work using normal actions.

The reason I didn't immediately want to do this was to be able to always log the raw actions from the main thread. Maybe I could add an option for that for a "dev mode" of sorts.

davej commented 8 years ago

All that makes perfect sense.

For my use case, certainly, I only need the action to be run in the source window. I can see the case for the main process having a top-down view of all actions, particularly for logging.

Both are reasonable so I guess it comes down to preference. I will say that I find it easier to reason about it if the action is only run on the source window though. Then I don't need to worry about potential race conditions or whether the actions will be async or sync, it seems a bit easier to grok what's happening.