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 to RN v0.68 #5344

Closed
gnprice opened this issue Apr 19, 2022 · 10 comments · Fixed by #5610
Closed

Upgrade to RN v0.68 #5344

gnprice opened this issue Apr 19, 2022 · 10 comments · Fixed by #5610
Assignees
Labels
P1 high-priority upstream: RN Issues related to an issue in React Native

Comments

@gnprice
Copy link
Member

gnprice commented Apr 19, 2022

The next round after #5232. Blog post here:
https://reactnative.dev/blog/2022/03/30/version-068
and more-detailed changelog:
https://github.com/facebook/react-native/blob/main/CHANGELOG.md#v0680

@gnprice gnprice added upstream: RN Issues related to an issue in React Native P1 high-priority labels Apr 19, 2022
@gnprice
Copy link
Member Author

gnprice commented Jun 22, 2022

This looks like more changes than v0.67 (I say having read the full changelog, but not looked at any of the code yet), but still no obvious obstacles. Other than one: it does sound like it'll probably require the Gradle and AGP upgrades, #5377.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 26, 2022
Following the "Bare workflow" instructions for upgrading:
  https://blog.expo.dev/expo-sdk-45-f4e332954a68#5954

As with the last one, 44 in 3edc37a, the `expo upgrade` command
went through a phase it described as "Updating packages to
compatible versions (where known)," and that changed several of our
dependency ranges, even of non-Expo things. (For speculation on what
that phase is about, see 3edc37a.)

We rejected version-range changes to these libraries that would have
resulted in downgrades to the libraries' resolved versions:

- react-native-safe-area-context from 4.3.1 to 4.2.4
- react-native-screens from 3.13.1 to 3.11.1
- react-native-webview from 11.22.2 to 11.18.1

We rejected upgrades to @react-native-community/netinfo and
react-native-reanimated for the reasons we gave in the Expo 44
upgrade; see those at
  zulip#5441 (comment) .

And we rejected bumping react-native from 0.67.4 to 0.68.2, with
optimism that we can get this Expo upgrade done before that RN
upgrade, which is zulip#5344.

We took the rest.

In the step

> Update your `App.Delegate.mm` (you should have moved from .m to
> .mm in the previous step) according to this diff

, the indicated changes just seemed to be the same changes we
applied in the Expo 44 upgrade (3edc37a), just rebased atop a RN
v0.68 upgrade. (The ".m" to ".mm" change is an example of that.)
When we do the RN v0.68, we should study some special code that Expo
wants us to add as part of that upgrade, beyond what the RN template
app says. But that's a job for later, and I didn't see anything to
do right now in this item.

I tested basic functionality on my iPhone 13 Pro running iOS 15.6,
and on the office Android device, a Samsung Galaxy S9 running
Android 9. I didn't see any problems with building or running.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 26, 2022
Following the "Bare workflow" instructions for upgrading:
  https://blog.expo.dev/expo-sdk-45-f4e332954a68#5954

As with the last one, 44 in 3edc37a, the `expo upgrade` command
went through a phase it described as "Updating packages to
compatible versions (where known)," and that changed several of our
dependency ranges, even of non-Expo things. (For speculation on what
that phase is about, see 3edc37a.)

We rejected version-range changes to these libraries that would have
resulted in downgrades to the libraries' resolved versions:

- react-native-safe-area-context from 4.3.1 to 4.2.4
- react-native-screens from 3.13.1 to 3.11.1
- react-native-webview from 11.22.2 to 11.18.1

We rejected upgrades to @react-native-community/netinfo and
react-native-reanimated for the reasons we gave in the Expo 44
upgrade; see those at
  zulip#5441 (comment) .

And we rejected bumping react-native from 0.67.4 to 0.68.2, with
optimism that we can get this Expo upgrade done before that RN
upgrade, which is zulip#5344.

We took the rest.

In the step

> Update your `App.Delegate.mm` (you should have moved from .m to
> .mm in the previous step) according to this diff

, the indicated changes just seemed to be the same changes we
applied in the Expo 44 upgrade (3edc37a), just rebased atop a RN
v0.68 upgrade. (The ".m" to ".mm" change is an example of that.)
When we do the RN v0.68, we should study some special code that Expo
wants us to add as part of that upgrade, beyond what the RN template
app says. But that's a job for later, and I didn't see anything to
do right now in this item.

I tested basic functionality on my iPhone 13 Pro running iOS 15.6,
and on the office Android device, a Samsung Galaxy S9 running
Android 9. I didn't see any problems with building or running.
@chrisbobbe
Copy link
Contributor

expo/expo@75e2cf7e3 claims that RN 68 will break animated GIF support unless we upgrade Fresco on Android to 2.5.0.

@chrisbobbe
Copy link
Contributor

When doing this upgrade, we should also examine changes to templates/expo-template-bare-minimum in the following commits, to see if there's anything special Expo wants us to do to handle the RN 68 upgrade:

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 22, 2022
Following the "Bare workflow" instructions for upgrading:
  https://blog.expo.dev/expo-sdk-45-f4e332954a68#5954

As with the last one, 44 in 3edc37a, the `expo upgrade` command
went through a phase it described as "Updating packages to
compatible versions (where known)," and that changed several of our
dependency ranges, even of non-Expo things. (For speculation on what
that phase is about, see 3edc37a.)

We rejected version-range changes to these libraries that would have
resulted in downgrades to the libraries' resolved versions:

- react-native-safe-area-context from 4.3.1 to 4.2.4
- react-native-screens from 3.13.1 to 3.11.1
- react-native-webview from 11.22.2 to 11.18.1

We rejected upgrades to @react-native-community/netinfo and
react-native-reanimated for the reasons we gave in the Expo 44
upgrade; see those at
  zulip#5441 (comment) .

And we rejected bumping react-native from 0.67.4 to 0.68.2, with
optimism that we can get this Expo upgrade done before that RN
upgrade, which is zulip#5344.

We took the rest.

For the step

> Update your `App.Delegate.mm` (you should have moved from .m to
> .mm in the previous step) according to this diff

, we found that we'd already taken some of those changes when Expo
backported them to the Expo 44 template app. Others, like the .m to
.mm rename, we'd like to postpone until we do the RN 68 upgrade. A
full audit of Expo's template-app commits from 44 to 45 found no
changes we'd be interested in applying, except some related to RN 68
that we'd like to do with that upgrade; I've mentioned those on our
RN 68 upgrade issue. See:
  zulip#5507 (comment)

I tested basic functionality on my iPhone 13 Pro running iOS 15.6,
and on the office Android device, a Samsung Galaxy S9 running
Android 9. I didn't see any problems with building or running.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Nov 23, 2022
Following the "Bare workflow" instructions for upgrading:
  https://blog.expo.dev/expo-sdk-45-f4e332954a68#5954

As with the last one, 44 in 3edc37a, the `expo upgrade` command
went through a phase it described as "Updating packages to
compatible versions (where known)," and that changed several of our
dependency ranges, even of non-Expo things. (For speculation on what
that phase is about, see 3edc37a.)

We rejected version-range changes to these libraries that would have
resulted in downgrades to the libraries' resolved versions:

- react-native-safe-area-context from 4.3.1 to 4.2.4
- react-native-screens from 3.13.1 to 3.11.1
- react-native-webview from 11.22.2 to 11.18.1

We rejected upgrades to @react-native-community/netinfo and
react-native-reanimated for the reasons we gave in the Expo 44
upgrade; see those at
  zulip#5441 (comment) .

And we rejected bumping react-native from 0.67.4 to 0.68.2, with
optimism that we can get this Expo upgrade done before that RN
upgrade, which is zulip#5344.

We took the rest.

For the step

> Update your `App.Delegate.mm` (you should have moved from .m to
> .mm in the previous step) according to this diff

, we found that we'd already taken some of those changes when Expo
backported them to the Expo 44 template app. Others, like the .m to
.mm rename, we'd like to postpone until we do the RN 68 upgrade. A
full audit of Expo's template-app commits from 44 to 45 found no
changes we'd be interested in applying, except some related to RN 68
that we'd like to do with that upgrade; I've mentioned those on our
RN 68 upgrade issue. See:
  zulip#5507 (comment)

I tested basic functionality on my iPhone 13 Pro running iOS 15.6,
and on the office Android device, a Samsung Galaxy S9 running
Android 9. I didn't see any problems with building or running.
@gnprice gnprice self-assigned this Dec 12, 2022
@gnprice
Copy link
Member Author

gnprice commented Dec 13, 2022

OK, I spent some time this afternoon starting on this upgrade.

  • I've scanned through the template-app changes upstream; sorted them all into the clusters below; and read many of them in detail.
  • I also have a draft branch that does the upgrade; it works in quick manual testing on Android, and passes tools/test --all. It still needs some of the changes below in order to keep up with the template app, and it needs more manual testing including some on iOS.
  • The word TODO below marks all the areas from the template-app changes that still need attention: either applying the changes, or investigating further.

I plan to edit the list below with updates as I proceed.

List of template-app changes

Preparing the list: in a react-native checkout,

$ git log --oneline --no-decorate --reverse v0.67.4..v0.68.5 -- template

then reformatted slightly: repo added to commit IDs, # replaced with # . Then kept a copy for reference (at end below), and started rearranging into groups and taking notes.

To review the changes in detail:

$ git log --stat -p --reverse v0.67.4..v0.68.5 -- template

Our own fix 🎉 returning to us from upstream

In the main upgrade commit

iOS changes

Android changes

Interesting for future, but nothing to do now

Not corresponding to changes for us to make

In original order, for reference

$ git log --oneline --no-decorate --reverse v0.67.4..v0.68.5 -- template
(slightly reformatted: repo added to commit IDs, # replaced with # )

facebook/react-native@ba7424d4d RN: Normalize Prettier Configuration
facebook/react-native@ce74aa4ed Add ReactInstanceEventListener for Venice and expose in FbReactInstanceHolder
facebook/react-native@1e6add1a4 iOS Ruby Updates (# 32456)
facebook/react-native@03a090786 Add back Xcode_12_5_M1_post_install_workaround
facebook/react-native@d15052965 Deploy 0.163.0 to xplat
facebook/react-native@b0711f1d3 Update ReactAndroid to use the AGP NDK Apis (# 32443)
facebook/react-native@4e587a1b7 JS: Upgrade to Prettier v2.4.1 [1/n]
facebook/react-native@43c38cdc8 Allow specifying an architecture in RNTester and release builds
facebook/react-native@fc962c9b6 Don't reconstruct app component on split-screen (# 32536)
facebook/react-native@0f39a1076 Make the reactNativeArchitectures property more discoverable
facebook/react-native@cf763cdf8 RN: Upgrade ESLint Packages (# 32560)
facebook/react-native@181464903 Deploy 0.164.0 to xplat
facebook/react-native@c180627ac bump gradle to 7.3 (# 32588)
facebook/react-native@8ace78c08 Deploy 0.165.0 to xplat
facebook/react-native@272cfe5d1 draft: bump AGP to 7 (# 32589)
facebook/react-native@00ac03435 Bump OSS Android build to SDK 31 (# 32606)
facebook/react-native@4fb49cd55 Rename AppDelegate.m to AppDelegate.mm in the app template
facebook/react-native@73a04d145 Refactor the iOS app template to move setups to a helper class
facebook/react-native@2098c8950 Add babel-plugin-codegen to babel.config.js for fabric (# 32756)
facebook/react-native@82b8f407c Revert the change to add babel-plugin-codegen
facebook/react-native@8ec0e6919 Add turbo module support in the default app template (# 32752)
facebook/react-native@2e9a376c8 Add fabric option to the default app template.
facebook/react-native@c0c543995 Rename the new architecture flag to RCT_NEW_ARCH_ENABLED
facebook/react-native@1cd8f05ee Refactor app template setup util functions
facebook/react-native@7dfb08a78 Deploy 0.167.1 to xplat
facebook/react-native@6a046fb9f Fix build issue for the new app template (# 32772)
facebook/react-native@e330eee9c Add Other C++ flags when RCT_NEW_ARCH_ENABLE=1 (# 32777)
facebook/react-native@aef843bfe update jsBundleURLForBundleRoot in template project
facebook/react-native@d1e359a15 Deploy 0.168.0 to xplat
facebook/react-native@b352aa369 Do not opt-out fbsource/xplat/js/react-native-github/template from CLANGFORMAT
facebook/react-native@8d652fba4 Setup a newArchEnabled property to Opt-in the New Architecture in the template (# 32790)
facebook/react-native@8bd3edec8 Update copyright headers from Facebook to Meta
facebook/react-native@cd4c6659d Bump Gradle, AGP and Download plugins
facebook/react-native@0fccbd53a Leverage Gradle implicit dependency substitution for Gradle Plugin
facebook/react-native@72d949e25 Bump gradle-plugin to 0.0.4
facebook/react-native@2f67f5d68 - Add vendor/bundle into .gitignore template (# 32930)
facebook/react-native@50057158c Bump Flipper to 0.125.0 (# 32923)
facebook/react-native@f595a4e68 Duplicate Xcodeproj UUIDs in iOS app template Podfile fixed
facebook/react-native@baea87811 Deploy v0.170.0 to xplat
facebook/react-native@ea74c5797 Bump direct Metro dependencies to 0.67.0
facebook/react-native@0fd6ade86 [0.68.0-rc.0] Bump version numbers
facebook/react-native@a2a703247 Increase max heap size for template gradle build
facebook/react-native@71548ffd6 Do not bundle libhermes.so or libjsc.so inside the React Native Android AAR (# 33038)
facebook/react-native@319e4df45 [0.68.0-rc.1] Bump version numbers
facebook/react-native@43d67cd1e Remove react-native-gradle-plugin as a dependency from template's package.json
facebook/react-native@bca4cf023 Set a resolution strategy for com.facebook.react:react-native when on New Architecture. (# 33134)
facebook/react-native@2cd317399 Remove optional codegen config inside template (# 33108)
facebook/react-native@66b82aec5 chore(deps): bump CLI version to 7.0.3 to address web debugging issue (# 33156)
facebook/react-native@65e4d98a7 [0.68.0-rc.2] Bump version numbers
facebook/react-native@762db49de Make sure configureNdkBuild* tasks are depending on preBuild
facebook/react-native@ccd170809 Re-apply: Consider relative to pwd installation root when looking for files in rn module via cocoapods (# 33427)
facebook/react-native@b3f19d7f1 [0.68.0-rc.3] Bump version numbers
facebook/react-native@a4a6e23de [0.68.0-rc.4] Bump version numbers
facebook/react-native@51f5ea15a [0.68.0] Bump version numbers
facebook/react-native@5a8033df9 Fix for building new architecture sources on Windows
facebook/react-native@8400590ab Template: Specify abiFilters if enableSeparateBuildPerCPUArchitecture is not set.
facebook/react-native@6268836c7 Improve support for Android users on M1 machine (# 33588)
facebook/react-native@387ee70e7 Use NDK 23 only for Windows users. (# 33611)
facebook/react-native@b5f1b26a8 [0.68.1] Bump version numbers
facebook/react-native@62ef6f5fa [Main][Windows] Working around Long paths limitation on Windows (# 33707)
facebook/react-native@72e1eda07 [0.68.2] Bump version numbers
facebook/react-native@36153e255 [0.68.3] Bump version numbers
facebook/react-native@b55518c4b [0.68.4] Bump version numbers
facebook/react-native@b12a7f9e6 [0.68.5] Bump version numbers

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Dec 13, 2022

Great, thanks for taking this on! 🙂 I think it's helpful for you to do one now and then, so we can compare notes on the process.

Here are a few small things from my notes (not having gone far in this upgrade yet), to add to what you've found above. Organized by which of your headings they apply under:

iOS changes

Android changes

Not corresponding to changes for us to make

@gnprice
Copy link
Member Author

gnprice commented Dec 13, 2022

Thanks for those notes!

I think it's helpful for you to do one now and then, so we can compare notes on the process.

Yeah, agreed.

Hmm, indeed. Specifically it sets android:exported on various activity elements in the manifest. We largely did that in fa6fbbf in #5174 (the PR that resolved #5101), but there's one other in debug/AndroidManifest.xml which we didn't. I'll take a look at applying that.

The other change I see is to buildToolsVersion. We don't set that directly, because AGP sets it automatically to an appropriate version. Instead we have this:

// Force all our third-party dependencies to a sensible buildToolsVersion.
// What version should that be?  It's whatever the Android plugin applied
// to our app project.
//
subprojects {
    //
        afterEvaluate {
            if (project.hasProperty("android")) {
                android.buildToolsVersion rootProject.ext.buildToolsVersion

(And even that I'm pretty sure is only necessary because various RN-ecosystem dependencies copied a buildToolsVersion line at some point and, naturally, haven't updated it — if they'd just let it be, I think AGP would take care of it completely.)

This one looks like it deletes some lines in template/ (which a previous commit had added), and adds some similar lines in ReactAndroid/ — i.e. inside the RN implementation itself. So I think there isn't anything for us to do — they found a way to take care of the issue within RN rather than push it out into apps, which is great.

Android changes

Yep, agreed.

Will note these down, thanks.

@gnprice
Copy link
Member Author

gnprice commented Dec 13, 2022

When doing this upgrade, we should also examine changes to templates/expo-template-bare-minimum in the following commits, to see if there's anything special Expo wants us to do to handle the RN 68 upgrade:

I scanned through these three, and they look like they're applying the changes from RN's own template app between 67 and 68.

There is a lot there, and very little explanation. So I didn't attempt to read every line closely. But having just read through those RN template changes, these changes generally looked familiar from that.

And on the other hand if there were some meaningful Expo-specific change buried inside one of those commits, that'd be pretty tough to find. It'd also be tough for a reviewer, or even the author, to find and think about… so one would have to wonder if it was even a fully intended change. I think we're best off just interpreting these as "do the RN upgrade according to the RN template changes."

@gnprice
Copy link
Member Author

gnprice commented Dec 13, 2022

expo/expo@75e2cf7e3 claims that RN 68 will break animated GIF support unless we upgrade Fresco on Android to 2.5.0.

Hmm, it looks like in fact this is already broken! On the current release v27.196, on both iOS and Android.

Repro: find some message with an animated GIF in it (first one I found was in #test here > codeblock.) In the message list, it's animated. (As expected because that's the browser's responsibility, not our code's.) When you open the lightbox, it's not.

Searching our tracker, this has happened a couple of times before — an RN upgrade breaks animated GIFs, until we upgrade Fresco:

So I guess this should become something we make a practice of updating on RN upgrades.

The recipe from those previous occasions is basically, look at this spot in the current docs:
https://reactnative.dev/docs/0.67/image#gif-and-webp-support-on-android
and follow the versions there. But it now has the following note:

Note: the version listed above may not be updated in time. Please check ReactAndroid/gradle.properties in the main repo to see which fresco version is being used in a specific tagged version.

So I guess that's the better recipe.

Better still would be if RN gives us a reasonable way to say "just use whatever version it is that RN wants to use here". I'll do some brief scouting around for that, but my hopes aren't super high.

@gnprice
Copy link
Member Author

gnprice commented Dec 13, 2022

Hmm, it looks like in fact this is already broken! On the current release v27.196, on both iOS and Android.

Oh and I guess iOS is a separate, older issue: #4097

Anyway, will fix for Android.

edit: -> #5608

gnprice added a commit to gnprice/zulip-mobile that referenced this issue Dec 13, 2022
This is the main upgrade commit for zulip#5344.  Some follow-up steps
remain to be done in the next few commits.

In the template app, this corresponds to:

 * Numerous commits just bumping the `react-native` dependency

 * One commit bumping Metro:
     facebook/react-native@ea74c5797 Bump direct Metro dependencies to 0.67.0

 * One commit updating AppDelegate.m:
     facebook/react-native@aef843bfe update jsBundleURLForBundleRoot in template project
   to correspond to an API change in RN itself:
     facebook/react-native@0912ee179 remove fallbackResource from RCTBundleURLProvider api

Other changes:

 * Update Podfile.lock after `yarn install`.

 * We get to remove a Flow fixme, because it was (as hoped for)
   fixed in RN upstream:
     facebook/react-native@851e87a1a make Easing an object, not a class
@gnprice
Copy link
Member Author

gnprice commented Dec 13, 2022

OK! Sent #5610 for the upgrade. Updated the big comment above about template changes to reflect taking care of each of them.

Also filed #5611 for one small followup from a pair of TODO(react-native-68) comments. That followup would be nice to do, but I think doesn't need to block considering the upgrade itself complete.

gnprice added a commit that referenced this issue Dec 14, 2022
This is the main upgrade commit for #5344.  Some follow-up steps
remain to be done in the next few commits.

In the template app, this corresponds to:

 * Numerous commits just bumping the `react-native` dependency

 * One commit bumping Metro:
     facebook/react-native@ea74c5797 Bump direct Metro dependencies to 0.67.0

 * One commit updating AppDelegate.m:
     facebook/react-native@aef843bfe update jsBundleURLForBundleRoot in template project
   to correspond to an API change in RN itself:
     facebook/react-native@0912ee179 remove fallbackResource from RCTBundleURLProvider api

Other changes:

 * Update Podfile.lock after `yarn install`.

 * We get to remove a Flow fixme, because it was (as hoped for)
   fixed in RN upstream:
     facebook/react-native@851e87a1a make Easing an object, not a class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 high-priority upstream: RN Issues related to an issue in React Native
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants