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

Update to RN 61 and AndroidX #1343

Merged
merged 50 commits into from
Oct 29, 2019
Merged

Update to RN 61 and AndroidX #1343

merged 50 commits into from
Oct 29, 2019

Conversation

cmcewen
Copy link
Contributor

@cmcewen cmcewen commented Oct 15, 2019

Description

This updates RN and a ton of our dependencies for 0.61. This gives us the Hermes JS engine, which is better on low end phones, autolinking of deps, AndroidX, and lots of other goodies. I had to update a ton of dependencies and do a lot of wrangling so apologies for the large size.

Tested

In progress right now, but using the app and running tests

Other changes

Describe any minor or "drive-by" changes here.

Related issues

Backwards compatibility

Lots of potential breakage here so please keep an eye out

@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #1343 into master will increase coverage by 8.34%.
The diff coverage is 58.06%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1343      +/-   ##
==========================================
+ Coverage   65.39%   73.73%   +8.34%     
==========================================
  Files         271      277       +6     
  Lines        8187     7418     -769     
  Branches      570      660      +90     
==========================================
+ Hits         5354     5470     +116     
+ Misses       2711     1836     -875     
+ Partials      122      112      -10
Flag Coverage Δ
#mobile 73.73% <58.06%> (+8.34%) ⬆️
Impacted Files Coverage Δ
packages/mobile/src/app/actions.ts 96% <ø> (+0.16%) ⬆️
packages/mobile/src/language/Language.tsx 74.07% <ø> (ø) ⬆️
packages/mobile/test/values.ts 100% <ø> (ø)
packages/mobile/test/utils.ts 97.14% <ø> (ø)
packages/mobile/src/firebase/notifications.ts 48.38% <0%> (+2.67%) ⬆️
packages/mobile/src/geth/geth.ts 21.98% <0%> (+1.25%) ⬆️
packages/mobile/src/account/saga.ts 83.78% <100%> (+1.96%) ⬆️
packages/mobile/src/redux/store.ts 100% <100%> (ø) ⬆️
packages/mobile/src/app/ErrorBoundary.tsx 100% <100%> (ø) ⬆️
packages/mobile/src/app/App.tsx 84.21% <100%> (ø) ⬆️
... and 270 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 494f140...6ff0a10. Read the comment docs.

@@ -34,26 +34,28 @@
"packages/*"
],
"nohoist": [
"@celo/verifier/react-native",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that RN uses the normal module resolution system instead of haste, we don't have to nohoist anymore

"@celo/web/@timkendrick/monaco-editor",
"@celo/web/@types/react-i18next",
"@celo/web/next-i18next",
"**/openzeppelin-solidity"
]
},
"devDependencies": {
"@types/jest": "^24.0.17",
"@babel/core": "^7.6.2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just moving up shared devDeps

@@ -1 +1,6 @@
export function navigate() {}
export const navigate = jest.fn()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jest started complaining about mocking in scope so I moved to this. It should work the same in tests

@@ -223,56 +253,32 @@ android {
}

dependencies {
implementation project(':react-native-install-referrer')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Autolinking means we don't have to add these intentionally any more

Copy link
Contributor

Choose a reason for hiding this comment

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

Woot!

@@ -1,5 +1,4 @@
import NetInfo from '@react-native-community/netinfo'
import { ConnectionInfo } from 'react-native'
import NetInfo, { NetInfoState } from '@react-native-community/netinfo'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NetInfo was complaining about deprecated methods

@@ -1,7 +1,7 @@
import AsyncStorage from '@react-native-community/async-storage'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed from RN core

environment: DeviceInfo.getBundleId(),
react: true,
})
Sentry.init({ dsn: SENTRY_URL, environment: await DeviceInfo.getBundleId() })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New sentry version changed the API a bit

Copy link
Contributor

Choose a reason for hiding this comment

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

import * as _ from 'lodash'
import DeviceInfo from 'react-native-device-info'

const TAG = 'CeloAnalytics'

function getDeviceInfo() {
async function getDeviceInfo() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to change this due to the async functions

@@ -1,19 +1,29 @@
const defaultConfig = require('../../jest.config.js')
const reactNativeJestPreset = require('react-native/jest-preset')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Jest was being v annoying to had to start over with this

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.

Really great work, thanks for taking this on!!
Mostly lgtm, just some questions below

packages/mobile/android/app/build.gradle Show resolved Hide resolved
packages/mobile/android/app/build.gradle Show resolved Hide resolved
packages/mobile/android/app/build.gradle Show resolved Hide resolved
@@ -223,56 +253,32 @@ android {
}

dependencies {
implementation project(':react-native-install-referrer')
Copy link
Contributor

Choose a reason for hiding this comment

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

Woot!

@@ -1,6 +1,6 @@
jest.useFakeTimers()

import Button from '@celo/react-components/components/Button'
import mockButton from '@celo/react-components/components/Button'
Copy link
Contributor

Choose a reason for hiding this comment

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

What I read is that symbols beginning with 'mock' will be hoisted to the top of the file before jest messes around with the real imports.

@@ -18,16 +19,14 @@ import Navigator from 'src/navigator/NavigatorWrapper'
import { persistor, store } from 'src/redux/store'
import Logger from 'src/utils/Logger'

useScreens()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we use it? I know it's a dep of some of other other deps but to my knowledge we don't use it anywhere. Just gave their readme a look, don't fully understand what it's for.

packages/mobile/src/app/App.tsx Show resolved Hide resolved
packages/mobile/src/firebase/notifications.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

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

That's an EPIC effort!! Looks awesome! 🥇 🎉 💣

Just some small issues/questions.

@@ -1,81 +1,66 @@
# File contents of "ios/Podfile"
platform :ios, '9.0'
platform :ios, "9.0"
require_relative "../../../node_modules/@react-native-community/cli-platform-ios/native_modules"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! 👍

packages/mobile/jest.config.js Outdated Show resolved Hide resolved
packages/mobile/jest.config.js Show resolved Hide resolved
}

async componentDidMount() {
const phoneNumber = this.props.e164PhoneNumber
const verified = await isPhoneNumberVerified(phoneNumber)
this.setState({ verified })
this.setState({ version: await DeviceInfo.getVersion() })
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's kind of painful for some basic stuff :/

}

async componentDidMount() {
const phoneNumber = this.props.e164PhoneNumber
const verified = await isPhoneNumberVerified(phoneNumber)
this.setState({ verified })
this.setState({ version: await DeviceInfo.getVersion() })
Copy link
Contributor

Choose a reason for hiding this comment

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

Hum, they kept some of the basic info as normal functions. getVersion() still returns a string synchronously:
https://github.com/react-native-community/react-native-device-info/blob/v4.0.1/src/index.ts#L309-L318

You should be able to keep the existing code as is in this specific instance.

packages/react-components/analytics/CeloAnalytics.tsx Outdated Show resolved Hide resolved
Comment on lines 9 to 10
// Disables type-check when running tests as it takes valuable time
// and is redundant with the tsc build step
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you really don't like this comment? 😸

yarn.lock Show resolved Hide resolved
@cmcewen cmcewen requested review from asaj and m-chrzan as code owners October 28, 2019 20:12
@cmcewen cmcewen added the automerge Have PR merge automatically when checks pass label Oct 29, 2019
@celo-ci-bot-user celo-ci-bot-user merged commit 74c81bd into master Oct 29, 2019
@celo-ci-bot-user celo-ci-bot-user deleted the cmcewen/androidx branch October 29, 2019 05:45
aaronmgdr added a commit that referenced this pull request Oct 29, 2019
…repo into aaronmgdr/invite-page

* 'aaronmgdr/invite-page' of github.com:celo-org/celo-monorepo: (63 commits)
  Fix compile error during local clean build (#1506)
  Update to RN 61 and AndroidX (#1343)
  Set usage of shuffled round robin in the genesis block (#1464)
  Add spanish backup key for backup key flow (#1500)
  Fix sync tests by pulling genesis block to determine epoch length (#1504)
  [Wallet] fix missing full name error alert (#1496)
  [Wallet + Verification Pool] Add details about generating an app-signature (#1482)
  Deploy celo's image of ethstats (#1421)
  Storing previous randomness values (#1197)
  [Wallet] Fix wei invite bug (#1489)
  Point all packages to latest ganache-cli master (#1488)
  Point end-to-end tests back to master (#1469)
  [Wallet] Migrate app view functions to contractkit (#1381)
  [Wallet] Add script to translate locale strings (#1485)
  [Wallet] Update wallet celo client version and add missing translations for backup flow (#1483)
  [Wallet] Hotfix local currency (#1481)
  [Wallet] Remove QR debouncing to improve responsiveness (#1480)
  [Wallet] Upgrade app version to v1.5.1 (#1463)
  Update governance end-to-end tests to work with changed precompile (#1476)
  Fixes key_placer.sh when encrypting files (#1465)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wallet should use new Sentry version Users SBAT see better JS performance with Hermes
4 participants