Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thoughts for 2.0 #171

Closed
wants to merge 21 commits into from
Closed

Thoughts for 2.0 #171

wants to merge 21 commits into from

Conversation

aslilac
Copy link

@aslilac aslilac commented May 6, 2020

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
Copy link
Contributor

hardchor commented May 7, 2020

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
Copy link
Collaborator

sameoldmadness commented Aug 14, 2020

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
Copy link
Author

aslilac commented Aug 17, 2020

@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.

@matmalkowski matmalkowski linked an issue Sep 17, 2020 that may be closed by this pull request
5 tasks
@aslilac aslilac closed this Dec 20, 2020
@aslilac aslilac deleted the branch klarna:master December 20, 2020 01:28
@aslilac aslilac deleted the master branch December 20, 2020 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🔥 The road to electron-redux 2.0
3 participants