search

samiskin / redux-electron-store

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

feature request: client-only redux fields #29

Closed bgrayburn closed 7 years ago

bgrayburn commented 8 years ago

it's nice to limit syncing with the filter in the store on the renderer process, but is there anyway to limit actions called from the renderer from propagating to the main thread?

samiskin commented 8 years ago

@bgrayburn Could you explain what your situation is exactly?

The reason I'm asking for more information is that simply adding a second method that'll dispatch but won't propagate will result in some weird behavior when considering the situation where:

Suppose the store was { count: 1 }

  1. The renderer dispatches an action to increase the count by 1 on the renderer only, so its count is now 2
  2. The main process later dispatches an action to increase the count by 3, so its count is now 4
  3. The main process then updates the renderer, and the renderer's data gets overridden and becomes 4.

Its now as if the renderer's local change never happened.

bgrayburn commented 8 years ago

These fields would have to exist only on the renderer. For example I have the position of a cursor in an editor. My main process doesn't care about that so I'd like to have that state only in the renderer redux store. There shouldn't be any sync issues because the renderer process only cares about what's in the renderer redux which can only be updated from the renderer in this hypothetical. I've been using filters to isolate state in the main process, but sometimes you only want a piece of state in the renderer process. Sorry if I'm repeating myself, just want to be clear. Your library is awesome, thank you!

On Fri, Sep 9, 2016, 17:46 Shiranka Miskin notifications@github.com wrote:

@bgrayburn https://github.com/bgrayburn Could you explain what your situation is exactly?

The reason I'm asking for more information is that simply adding a second method that'll dispatch but won't propagate will result in some weird behavior when considering the situation where:

Suppose the store was { count: 1 }

  1. The renderer dispatches an action to increase the count by 1 on the renderer only, so its count is now 2
  2. The main process later dispatches an action to increase the count by 3, so its count is now 4
  3. The main process then updates the renderer, and the renderer's data gets overridden and becomes 4.

Its now as if the renderer's local change never happened.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/samiskin/redux-electron-store/issues/29#issuecomment-246049604, or mute the thread https://github.com/notifications/unsubscribe-auth/ADBhdG4kpRw3p_OtU2cWlFsO2ix8z4MDks5qodOmgaJpZM4J5KHY .

samiskin commented 8 years ago

Ah makes sense. I think what I'll do then is make the renderer run the reduction, see what changes fit the filter, and then if there were changes, send that over.

Just to make sure, it does work right now right? The issue is just that the performance suffers since there are unnecessary operations to IPC the action over and run the reduction on the main process.

bgrayburn commented 8 years ago

exactly, that sounds perfect. Let me know if you'd rather me implement this. Wouldn't mind. And yes it does work currently, with the issue mentioned.

samiskin commented 8 years ago

Cool, yeah if you could implement it that'd be a huge help, as I'm rather busy for the next week :+1:

Since I've never gotten to adding proper automated testing for this, just add a Test plan in the PR to show what you did to test it. You can bump the version to 0.4.0 as well.

bgrayburn commented 8 years ago

ok I've implemented, and it works in my current project, although...I'm not sure if doing the comparison and everything on the renderer actually gives you a speed up over firing off the ipc. I want to make a small test app to test functionality and perf. Quickly checking with devtron in my current project indicates that my changes are working. As soon as I'm done with a test app I'll file the pull request.

samiskin commented 8 years ago

Cool. One thing that also needs to be done is that the renderer should send just an {updated, deleted} payload to the main process, and then the main process should just merge that into itself and forward that to the renderers. This is the same as how the main process currently sends updates to the renderers.

This may be the reason performance still isn't the greatest, as the reductions are synchronously ran twice, rather than asynchronously ran twice as they were before. If the {updated, deleted} payload was sent to the main process, the reduction would just be ran once total.

samiskin commented 7 years ago

I'm no longer certain about this solution of using the updated, deleted to see if an action should or should not be forwarded. The issue is that its a reasonable use case that an action wouldn't make any changes to the store, but the user still wants it to be forwarded anyway as they might have something like redux saga watching for that action to make a side effect.

I couldn't think of a better way to solve this, but I also couldn't think of a way to solve this outside of something internal to this library, so I added an actionFilter parameter. It takes an action and returns true if that action should be forwarded. I'll close this issue once https://github.com/samiskin/redux-electron-store/pull/35 is merged