-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
React native upgrade 0.62.1 #1491
Conversation
connection.project.dir= | ||
eclipse.preferences.version=1 | ||
gradle.user.home= | ||
java.home=/Library/Java/JavaVirtualMachines/jdk1.8.0_171.jdk/Contents/Home |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to add this? My thinking here is that this is system dependant and will be different for everyone. What if a user has Java installed in a different directory? I'm on linux and so, I don't even have a /Library
folder on my system (as an example). Of course, i also don't use Eclipse either so 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think you are right, I am not sure why this file was created, I think android studio may have created it. I will take a look, thanks for pointing it out!
] | ||
} | ||
} | ||
"name": "metamask", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why we're re tabbing this? It's difficult to get a sense of what actually changed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this is strange, will fix it!
@@ -0,0 +1,138 @@ | |||
module.exports = { | |||
root: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe pulling the eslintConfig
into a .eslintrc.js
file (a change I'm in favour of) could be done in a separate PR to keep changes minimal in this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that there is a huge amount of rules now, package.json would be very confusing. I think we have no choice than to do this either way. Also, the rules are done in a different way now, the lib we were using does not work with this version of react native anymore.
Description
This PR addresses the upgrade from React Native 0.59.10 to 0.62.1. Although there were lots of changes necessary, an effort was made in order to keep as much as possible the version of the libraries that were already being used (no unnecessary library upgrades were made), and also to keep as much of the code as intact as possible (no unnecessary code changes were made). This makes it easier to track any possible problems to the React Native upgrade itself. The upgrading of libraries to a newer current version is a step 2 outside of this PR context.
Note: To run this PR, don't forget to check the changes on the README. More specifically installing the pods:
brew install cocoapods
And on the root of the project
cd ios && pod install && cd ..
To open the project on Xcode, the file MetaMask.xcworkspace should be used instead of MetaMask.xcodeproj.
yarn clean
also installs the pods now.General file changes
The first part of the upgrade was making all the file changes needed in order to integrate the new React Native version. Those changes can be followed on this upgrade-helper.
Library changes
One of the biggest changes in React 0.60+ is the integration by default of the pods system. This means every library needed to be unlinked. This is because with the new system, every library is automatically linked. More specifically, the libraries that were unlinked were:
Libraries that did not support auto-linking
Some libraries couldn't be unlinked because they did not support auto-linking. Furthermore, in order for some of them to work, changes needed to be made on each library.
More specifically on iOS:
and on Android:
Patches removed
react-native+0.59.10.patch
. Since a new version of React Native was installed, this patch did not make sense anymore. No patch for the new version was necessary, at least for successfully compiling and running the app.Removed assets
react-native-vector-icons
were removed. The auto-linking should add those fonts automatically now.Errors fixed
After the app compiled successfully, there were some errors encountered when running the app. More specifically:
WebView
. After some research, the fix was modifying our custom WebView.import 'react-native-gesture-handler';
to theindex.js file
. See here.Warnings fixed
Animated: 'useNativeDriver' was not specified. This is a required 'option and must be explicitly set to true or false
. This warning was fixed by upgradingreact-native-modal
from 9.0.0 to 11.5.6.Animated.event now requires a second argument for options
. This warning was fixed by upgradingreact-native-modal
from 9.0.0 to 11.5.6.Calling getNode() on the ref of an Animated component is no longer necessary
. Since a lot of libraries were causing this warning likereact-native-scrollable-tab-view
,react-native-keyboard-aware-scroll-view
andreact-native-tab-view
, this warning was added to the ignore list. We should address this by upgrading the libraries mentioned.componentWillUpdate deprecated
. Since this appears on a lot of our components, this warning was added to the ignore list. Check this for possible fix.componentWillReceiveProps deprecated
. Since this appears on a lot of our components, this warning was added to the ignore list. Check this for possible fix.Lint
The lint plugin we used called
eslint-config-react-native
no longer worked correctly with the new React Native version. The reccommended plugin now is@react-native-community/eslint-config
. An effort was made so that our current rules remained the same. Because of this, there were lots of rules added so a new file called.eslintrc.js
was created to accommodate all the rules. It's possible that this new config is more strict, so we may still change the config file to our liking.Issue
Resolves #1201