klarna / electron-redux

Use redux in the main and browser processes in electron
MIT License
746 stars 95 forks source link

Thoughts for 2.0 #171

Closed aslilac closed 3 years ago

aslilac commented 4 years ago

Hi, it's me again! Hopefully you remember me from #145 😊

So I just wanna preface this by saying I don't expect this PR to get merged, but I did want to have a discussion!

I ended up really making myself at home on my fork while I worked on some stuff. A lot of changes in the diff are just boilerplate type things, but there's a lot of meat in there too.

Support for using Map and Set types in stores

Pretty simple, but adds a little machinery to serialize and deserialize Map and Set objects for initializing renderer state, like I had in my original PR

A greatly simplified API

import { syncMain } from 'electron-redux';
import { createStore } from 'redux';

const store = createStore(reducer, syncMain);
import { syncRenderer } from 'electron-redux';
import { createStore } from 'redux';

const store = createStore(reducer, syncRenderer);

That's it. That's all you need now. Renderer state initialization is now handled through an async action that we dispatch internally. Forwarding and replaying are handled from within the enhancer and don't need a separate function to subscribe to updates. It also doesn't need applyMiddleware. This was mostly out of necessity, since it doesn't provide the full store API, and I couldn't get the result I wanted without direct access to the store. It does have the nice side effect though of being just a little less code for the end user to type/understand. 🙂

Cleaned up some internal workings

Particularly the way that the initial state is fetched for renderer processes really got cleaned up. Rather than using global variables and a getter function, it uses idc communication which allows it to also happen asynchronously. The renderer will start with the usual default state determined by Redux, and will be swapped out once the correct state has been fetched from main.


Particularly the API changes are pretty substantial, but I'm curious what you think about them!

hardchor commented 4 years ago

Amazing work, and fantastic base for 2.0. Give me a bit of time to digest and go through the changes you've made, but I think in general the direction is great.

sameoldmadness commented 4 years ago

Great work! The API is so much cleaner now.

There're a couple of issues though that have to be resolved before merging this PR.

  1. Namespace prefixed should be removed from readme and action names, i.e. mckayla.electron-redux.FETCH_STATE.
  2. Either old tests should be restored, or new tests should be implemented.

@partheseas Is it ok if I make these changes on top of your work, or would you rather handle it yourself?

aslilac commented 4 years ago

@sameoldmadness Yeah, the intent was never for this PR to actually get merged, but more of "hey this is what I'm doing to simplify/improve things for me, feel free to copy if you like my ideas" so feel free to rebase and make whatever changes you want.