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.

Sign up for GitHub

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

Jump to bottom

Consolidated application state store. #1721

Merged
merged 5 commits into from
Aug 16, 2019
Merged

Consolidated application state store. #1721

merged 5 commits into from
Aug 16, 2019

Conversation

tonyanziano
Copy link
Contributor

@tonyanziano tonyanziano commented Aug 13, 2019 •
edited
Loading

Resolves #1697

===

This is a huge PR, so here are a couple of notes:

  • The real magic happens in forwardToMain.ts, forwardToRenderer.ts, & store.ts on both main and client
  • The reducers & actions had to be combined and duplicated on both sides. I'm thinking that in a future PR, these could all be moved to the /app/shared/ package so that we only need one copy of each reducer and set of actions
  • Removed a lot of unnecessary code calling between main / renderer to sync data
  • Removed some unused / unnecessary reducer & action code
  • Added missing tests for all state actions (planning to add tests for all state reducers) DONE

@cwhitten
Copy link
Member

@tonyanziano from the description it looks like you still plan to implement some additional tests?

(planning to add tests for all state reducers)

@tonyanziano
Copy link
Contributor Author

@cwhitten Correct, but I was thinking of including them in a separate PR to keep this one as small as possible.

I can include them in this PR though if you'd prefer.

@coveralls
Copy link

coveralls commented Aug 15, 2019 •
edited
Loading

Coverage Status

Coverage increased (+2.4%) to 65.323% when pulling 452a118 on toanzian/state into 54e8cbe on master.

@tonyanziano
Copy link
Contributor Author

Reducer tests have been added.

@tonyanziano tonyanziano merged commit 889b7d2 into master Aug 16, 2019
@tonyanziano tonyanziano deleted the toanzian/state branch August 16, 2019 20:10
@tonyanziano tonyanziano mentioned this pull request Oct 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cwhitten cwhitten cwhitten approved these changes

@a-b-r-o-w-n a-b-r-o-w-n Awaiting requested review from a-b-r-o-w-n

@compulim compulim Awaiting requested review from compulim

@corinagum corinagum Awaiting requested review from corinagum

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consolidated state management
3 participants
@tonyanziano @cwhitten @coveralls