Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

consolidate state in app/ledger.js with appState #3433

Closed
diracdeltas opened this issue Aug 25, 2016 · 3 comments
Closed

consolidate state in app/ledger.js with appState #3433

diracdeltas opened this issue Aug 25, 2016 · 3 comments

Comments

@diracdeltas
Copy link
Member

there is a lot of state that is duplicated between appState and ledgerInfo/publisherInfo in app/index.js, which becomes confusing to keep in sync. this should be deduplicated somehow.

@bsclifton
Copy link
Member

bsclifton commented Nov 26, 2016

I'd like to propose taking this one step further:

  • creating a ledgerState or working directly with appState (not using actions to manipulate state)
  • refactoring code to be reducer friendly
  • breaking up app/ledger.js into a utils and a state helper

I have an example that I spent maybe 20 minutes hacking on which shows some progress towards that goal. Definitely curious what others think!
cc: @bbondy @bridiver @diracdeltas @mrose17

https://github.com/bsclifton/browser-laptop/commit/bd9e1356a0e3c5819cad6dc88823d72a821420bc

Notice that with a reasonable amount of effort, we can pull most of the code out of app/ledger.js and into both a state helper (for methods manipulating state; eg: the reducers) and a utils class (business logic). With the code broken out here, we can then begin the process of writing unit tests 😄

@bridiver
Copy link
Collaborator

bridiver commented Nov 26, 2016

a separate ledger state would be going against the plan to consolidate into a single appState. We also want to move away from the json files and move things into the chromium user prefs storage which is already setup for cross-device sync. We can also bring in the history component to keep it out of the general preferences. I think we should focus on merging the ledger info into the appState so it will get migrated to user prefs store along with everything else

@bsclifton
Copy link
Member

@bridiver that makes sense- instead of ledgerState, these reducers could be manipulating appState 😄 Let me update the original post

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants