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

Upgrade React Native to 0.66 #867

Merged
merged 83 commits into from
Jan 20, 2022
Merged

Upgrade React Native to 0.66 #867

merged 83 commits into from
Jan 20, 2022

Conversation

achou11
Copy link
Member

@achou11 achou11 commented Nov 10, 2021

Closes #840

Open questions so far:

  • I think react-native-fs may cause us problems after the bump to the sdk version we target (30). Several issues on the repo questioning whether it's actively maintained and there seem to be many issues with it on Android 11. More importantly, based on Android 11 new policy "MANAGE_EXTERNAL_STORAGE" itinance/react-native-fs#998 seems like submission won't even be allowed to the Play Store unless we patch something? All in all, I think we may need to look into a better maintained alternative
    • perhaps somewhat reassuring, but Manyverse also uses the dependency and I'm guessing they don't have any issues with submitting to the Play Store, so maybe we'll be okay for now. Could also possibly depend on the apis being used in the app code, but I guess we'll address the problem if we run into it 🤷‍♂️
  • Do we want to enable Hermes? I briefly tried it and nothing seemed noticeably broken as far as I can tell. Some benefits of doing so:
    • apparent performance and size advantages over existing solutions
    • take advantage of the Intl API work that was introduced in 0.65
    • allows us to use some other deps/more recent versions e.g. react-native-reanimated v2 (which is not really used heavily in our app)
    • remove configuration specific to incorporating react-native-v8 into the project

Known issues:

  • splash screen doesn't always show correctly. there are various issues with react-native-splash-screens and seems like it's not maintained well enough for it to be resolved. general consensus has been to use react-native-bootsplash instead, which I have mostly working on a local branch. The only thing that needs to be addressed is getting the ICCA assets generated (I basically just need the Mapeo logo with the ICCA copy to generate the necessary assets) cc @gmaclennan

Remaining work:

  • potentially need to update Detox
  • fix splashscreen

Dependency changes:

  • Added
    • Primary:
      • v8-android
    • Dev (not including @types):
      • metro-config
      • react-native-codegen
  • Upgraded
    • Primary:
      • @react-native-mapbox/maps (8.0.0 -> 8.5.0)
      • nodejs-mobile-react-native (0.6.1 -> 0.6.3)
      • react (16.13.1 -> 17.0.2)
      • react-native (0.63.4 -> 0.66.4)
      • react-native-device-info (5.6.3 -> 8.4.8)
      • react-native-v8 (0.63.2-patch.0 -> 0.66.3-patch.1)
    • Dev (not including @types):
      • @babel/core (7.11.1 -> 7.12.17)
      • @babel/runtime (7.11.2 -> 7.12.18)
      • @react-native-community/cli (4.12.0 -> 6.2.0)
      • babel-jest (25.3.0 -> 26.6.3)
      • eslint (7.6.0 -> 7.14.0)
      • flow-bin (0.122.0 -> 0.149.0)
      • metro-react-native-babel-preset (0.61.0 -> 0.66.2)
      • react-dom (16.13.1 -> 17.0.2)
      • react-test-renderer (16.13.1 -> 17.0.2)
  • Removed
    • Primary:
    • Dev (not including @types):

Copy link
Member

@gmaclennan gmaclennan left a comment

Choose a reason for hiding this comment

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

So two outstanding issues to fix:

  1. Ensuring that sync will work if we're using Node v12.19.0 (from [email protected])
  2. Fixing version numbers in bugsnag

@achou11
Copy link
Member Author

achou11 commented Jan 3, 2022

After internal discussion, going to do the following to make this PR less daunting and safer:

  1. Extract non-upgrade changes to a separate PR
  2. Merge other PR after ensuring changes don't break stuff
  3. Rebase this PR with those changes

See #887

@@ -4,6 +4,7 @@
<uses-permission android:name="android.permission.INTERNET" />
<uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION" />
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />
<uses-permission android:name="android.permission.FOREGROUND_SERVICE" />
Copy link
Member Author

Choose a reason for hiding this comment

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

for context, think this is automatically added via expo-location but explicitly added here for clarity

https://github.com/expo/expo/tree/master/packages/expo-location#configure-for-android

@achou11 achou11 requested a review from gmaclennan January 5, 2022 20:37
Comment on lines +60 to +62
uses: actions/setup-node@v2
with:
node-version: "12.16.3"
Copy link
Member Author

Choose a reason for hiding this comment

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

the upgrade of @react-native-community/cli (which is used to run android build steps) requires Node >=12

Copy link
Member

Choose a reason for hiding this comment

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

Interesting I had thought we were already using Node 12 for builds. It's set in .nvmrc, but I don't think Github actions pay attention to that file. The other two places this matters are in Bitrise and Cirrus CI. Bitrise respects .nvmrc because it uses nvm to manage Node (https://github.com/digidem/mapeo-mobile/blob/develop/bitrise.yml#L326). Cirrus CI uses a Docker container that we build ourselves, and that is currently set to 12.x e.g. Node v12 latest (https://github.com/digidem/docker-android/blob/master/Dockerfile#L16). So I think we should be ok for this change, just wanted to make sure you are aware of all these pieces.

Copy link
Member Author

@achou11 achou11 Jan 6, 2022

Choose a reason for hiding this comment

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

Certain parts of the build process were using Node 12, but they had node-version specified. Without it version 10 was being installed (or used via the cache).

Regardless, the additional context is appreciated 👍

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.

Migrate to React Native 0.66
2 participants