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

[wallet] upgrade react-redux #812

Merged
merged 4 commits into from
Sep 3, 2019
Merged

Conversation

jeanregisser
Copy link
Contributor

@jeanregisser jeanregisser commented Sep 3, 2019

Description

Upgrade react-redux so we can use the new hooks API which is nicer to use with functional component than wrapping the component with connect.
I’m working on the new wallet home (#759) and since I’m writing new functional components I felt the need for this.

Tested

Upgraded and ran tests. Also did a quick test run of the app.

Other changes

  • Upgraded redux (patch version)
  • Removed duplicated redux and react-redux versions (yarn.lock)
  • Upgraded hoist-non-react-statics to fix a new error about propTypes
  • Moved and @types/hoist-non-react-statics to react-component. They are only used there.
  • Migrated some new failing tests to react-native-testing-library (our enzyme version is incompatible with the new react context API used by the upgraded react-redux)
  • Simplified CalculateFee a little by using the new useDispatch hook

Related issues

Backwards compatibility

Yes.

This is so we can use the new hooks API
Also removed duplicated redux / react-redux versions
- Use bindActionsCreator instead of the shorthand syntax for some
mapDispatchToProps that were using redux-thunk actions to workaround a
new typescript error with the upgraded react-redux definitions
- Migrated some tests to react-native-testing-library as they
were raising 'TypeError: ShallowWrapper::dive() can only be called on components'.
This is because the enzyme version we're using is not compatible with the new react
context API used by the new react-redux
@jeanregisser jeanregisser changed the title [wallet] upgrade react redux [wallet] upgrade react-redux Sep 3, 2019
Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


useEffect(
() => {
// Generic error banner
if (asyncResult.error) {
Logger.error('CalculateFee', 'Error calculating fee', asyncResult.error)
showErrorFunction(ErrorMessages.CALCULATE_FEE_FAILED)
dispatch(showError(ErrorMessages.CALCULATE_FEE_FAILED))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@cmcewen cmcewen merged commit 4ae2c70 into master Sep 3, 2019
@cmcewen cmcewen deleted the jeanregisser/upgrade-react-redux branch September 3, 2019 18:59
m-chrzan pushed a commit that referenced this pull request Sep 4, 2019
* Upgrade react-redux to latest stable version

This is so we can use the new hooks API
Also removed duplicated redux / react-redux versions

* Upgrade failing tests with the react-redux upgrade

- Use bindActionsCreator instead of the shorthand syntax for some
mapDispatchToProps that were using redux-thunk actions to workaround a
new typescript error with the upgraded react-redux definitions
- Migrated some tests to react-native-testing-library as they
were raising 'TypeError: ShallowWrapper::dive() can only be called on components'.
This is because the enzyme version we're using is not compatible with the new react
context API used by the new react-redux

* Ignore a new typescript error

* Simplify CalculateFee by using useDispatch instead of passing showError down to the component
aaronmgdr added a commit that referenced this pull request Sep 5, 2019
* master:
  Export geth metrics from StatefulSet tx-nodes and validators (#827)
  Adjust governance end-to-end gold total supply block number  (#853)
  [Blockchain API] Add currency conversion endpoint to blockchain api (#855)
  [Wallet] Fix Payment Requests and DRY up confirmation card code (#831)
  Use HTTPS instead of SSH for ethereumjs-vm (#850)
  Kill end-to-end celo-blockchain instances with SIGINT (#849)
  [CLI]Minor code improvement in CLI, remove ts-ignore-next-line (#796)
  [wallet] Always allow users to view backup key (#825)
  [contractkit] Complete Exchange Wrapper (#801)
  [Wallet] Fix web3 syncing sagas (#809)
  Aaronmgdr/social links connect (#792)
  Deploy integration (#806)
  [wallet] upgrade react-redux (#812)
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.

3 participants