Skip to content
This repository has been archived by the owner on Nov 10, 2023. It is now read-only.

#99 Refactor redux store #103

Merged
merged 52 commits into from
Apr 22, 2019
Merged

#99 Refactor redux store #103

merged 52 commits into from
Apr 22, 2019

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Apr 18, 2019

In this PR:

  • Update dependencies
  • Update README.MD to use yarn since we use yarn on our ci and IMO it's better to keep it the same everywhere
  • Rewrite logic for saving active tokens: now active tokens are calculated for all safes inside src/routes/safe/store/middleware/safeStorage.js:24. This was kind of required by the next change I'll describe
  • Remove duplication of tokens inside redux store:
    Previously tokens state looked like this:
  tokens: {
    safeAddress: list of active tokens for the safe + list of tokens from relay
}

now it's just

  tokens: {
    tokenAddress: token
  }

and activeTokens for each safe are stored on Safe's state in activeTokens array

  • Add array of token balance on safe's object. Again, in previous state since we stored tokens for each safe the token had balance property on it. Since it's not the case anymore, Safe now has an array with token balances
  • Tokens modal logic: calculate current active tokens on modal mount, and before it unmounts dispatch an action to update safe's active tokens / fetch balances. Because a user can toggle token activity and previously it'd fetch token's info every update, it caused selectors to recalculate and also produced a lot of network requests. To optimize that, I implemented the logic I describe above (thanks Andre for the idea :))
  • Fix updateSafe action: previously for updating a safe you can to create safe again, fetch all the info, update the property and then pass a new safe to the reducer. Otherwise It'd create a safe with initial values and overwrite the safe in store with them. Now it can accept a single prop update and the logic is handled in the reducer
  • Remove sessionStorage and cookieStorage from immortalDB used storage's list because of these potential issues:
    I wouldn't recommend using SessionStorage gruns/ImmortalDB#22
    Disable cookie store by default. gruns/ImmortalDB#6

OPTIMIZATION IDEA
Currently it saves safes/tokens to local storage on each UPDATE_SAFE and ADD_SAFE actions. I think maybe it do it right before the app window is closed? So the storage is not bombed with updates every 10-15 seconds. I need to do some research about this approach. Would love to hear your opinion you have done something like that :)

@mmv08 mmv08 requested a review from germartinez April 18, 2019 13:16
@ghost
Copy link

ghost commented Apr 18, 2019

Travis automatic deployment:
https://pr103--safereact.review.gnosisdev.com

Storybook book automatic deployment:
https://pr103--safereactstorybook.review.gnosisdev.com

@ghost
Copy link

ghost commented Apr 18, 2019

Travis automatic deployment:
https://pr103--safereact.review.gnosisdev.com

Storybook book automatic deployment:
https://pr103--safereactstorybook.review.gnosisdev.com

@germartinez
Copy link
Member

Answering to your question on the OPTIMIZATION IDEA, it looks good to me. Just remember that the user could also refresh the app, so that should be handled too.

@germartinez
Copy link
Member

Users visiting the app for the first time, when storage is empty, will get this error in the console:

Captura

src/routes/safe/store/actions/fetchSafe.js Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Apr 22, 2019

Travis automatic deployment:
https://pr103--safereact.review.gnosisdev.com

Storybook book automatic deployment:
https://pr103--safereactstorybook.review.gnosisdev.com

@ghost
Copy link

ghost commented Apr 22, 2019

Travis automatic deployment:
https://pr103--safereact.review.gnosisdev.com

Storybook book automatic deployment:
https://pr103--safereactstorybook.review.gnosisdev.com

@mmv08 mmv08 merged commit a689c3b into development Apr 22, 2019
@mmv08 mmv08 deleted the 99-app-state branch April 22, 2019 13:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants