RN v0.68 upgrade #5610
RN v0.68 upgrade #5610
Conversation
gnprice
added
upstream: RN
There was a problem hiding this comment.
Great! Thanks for taking on this RN upgrade! 😅
Looks good to me, please merge at will—and thanks for your detailed notes on the issue thread! I tested a bit on an iPhone simulator, my iPhone, and the office Android device, and things seem to work as normal, which is a good sign.
Tomorrow is an office day, so if you'd like we can chat there and compare workflows, etc.
package.json
Outdated
| @@ -117,6 +117,7 @@ | |||
| "jest-expo": "^45.0.0", | |||
| "jest-extended": "^3.2.0", | |||
| "jetifier": "^2.0.0", | |||
| "metro-babel-register": "^0.66.2", | |||
There was a problem hiding this comment.
deps [nfc]: Add metro-babel-register to direct dev-deps
At present, we get this indirectly through metro@0.66.2, which is
pulled in via RN.
In RN v0.68, we'll get metro@0.67.0, and that drops this dependency.
But we still need it -- without it, RN's `jest/preprocessor.js` fails,
breaking our Jest tests. So add it in directly.
Huh. It sounds like RN 68 broke Jest for everyone who was using it, or at least this jest/preprocessor.js thing? 🤔
There was a problem hiding this comment.
It does! Or anyway left them to figure out they needed to add this.
| @@ -7,7 +7,7 @@ | |||
| objects = { | |||
|
|
|||
| /* Begin PBXBuildFile section */ | |||
| 13B07FBC1A68108700A75B9A /* AppDelegate.m in Sources */ = {isa = PBXBuildFile; fileRef = 13B07FB01A68108700A75B9A /* AppDelegate.m */; }; | |||
| 13B07FBC1A68108700A75B9A /* AppDelegate.mm in Sources */ = {isa = PBXBuildFile; fileRef = 13B07FB01A68108700A75B9A /* AppDelegate.mm */; }; | |||
There was a problem hiding this comment.
[…] That commit has a bunch of additional churn and new mess
in the Xcode project file, which apparently Xcode did automatically
when that change's author tried opening the project; but happily that
didn't happen for me.
Weird. I think all of the "new mess" is actually stuff that we want and have, or once had and reverted.
In particular, I think it's all covered by these commits of ours. But it's a lot of stuff so I might've missed something:
33f4b41 ios build: Start using CocoaPods again.
4b334f2 ios: Remove ZulipMobileTests target and references.
912cb9e flipper ios: Pin to some specific versions, to fix build with Xcode 12.5.
0d2cc3a deps: Upgrade to RN v0.67.4!
There was a problem hiding this comment.
Yeah. I didn't look closely at all of it, because it was clear from the commit message it wasn't intended except the parts that were actually about renaming that one source file. But I did notice that a lot of it looked like the stuff CocoaPods inserted for us.
| OTHER_CPLUSPLUSFLAGS = ( | ||
| "$(OTHER_CFLAGS)", | ||
| "-DFOLLY_NO_CONFIG", | ||
| "-DFOLLY_MOBILE=1", | ||
| "-DFOLLY_USE_LIBCPP=1", | ||
| ); |
There was a problem hiding this comment.
From a spot-check against Expo (not exhaustive): odd that Expo doesn't add this chunk, or the similar one below. 🤷 No one explained why, as far as I could find.
There was a problem hiding this comment.
Yeah. Given that Expo updates their templates in a single commit for each upgrade between RN releases, it seems like they're probably taking the entire diff between two released versions of the upstream template and attempting to merge that into their templates. That is a very big bite to chew at once, and having set themselves that task it's unsurprising that they would miss some things when trying to carry it out.
|
Thanks for the review! Merging. And yeah, let's discuss today in the office. I've tried to make visible in that comment on the issue thread (#5344 (comment)) a lot of how I approached it, but we'll certainly have more to talk about. |
This follows a change in the RN template app leading to RN v0.68: facebook/react-native@00ac03435 Bump OSS Android build to SDK 31 Because this activity doesn't have any intent filters, this attribute already has "false" as a default value, and there's no need to set it explicitly: https://developer.android.com/guide/topics/manifest/activity-element#exported SDK 31, aka Android 12, started requiring an explicit setting for this attribute when there *is* an intent filter (and the app targets SDK 31+): https://developer.android.com/about/versions/12/behavior-changes-12#exported (I think the default in that case had been "true", which is unsafe when not intended.) So in fa6fbbf we added it to our two other activities, which do have intent filters. It isn't really needed here; not only is the default what we want, but if this activity somehow ever grew an intent filter then there'd be a lint clearly pointing out that we needed this attribute. But the explicitness doesn't hurt anything, and keeps us aligned with the template app; add it.
At present, we get this indirectly through metro@0.66.2, which is pulled in via RN. In RN v0.68, we'll get metro@0.67.0, and that drops this dependency. But we still need it -- without it, RN's `jest/preprocessor.js` fails, breaking our Jest tests. So add it in directly.
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
This squashes three small Flipper-related commits as they affect the template RN app between RN v0.67.4 and v0.68.5: facebook/react-native@ce74aa4ed Add ReactInstanceEventListener for Venice and expose in FbReactInstanceHolder facebook/react-native@8bd3edec8 Update copyright headers from Facebook to Meta facebook/react-native@50057158c Bump Flipper to 0.125.0 The change touching ReactNativeFlipper.java requires the RN v0.68 upgrade in order to compile, so we do it after the upgrade.
I opened the file in Xcode, used "Navigate > Reveal in Project Navigator", and then long-clicked it there to get a UI to rename the file. That moved the file itself, and made these small changes in the Xcode project file. This follows a change in the template app on the way to RN v0.68: facebook/react-native@4fb49cd55 Rename AppDelegate.m to AppDelegate.mm in the app template which was done as preparation for the New Arch-related changes we'll follow next. That commit has a bunch of additional churn and new mess in the Xcode project file, which apparently Xcode did automatically when that change's author tried opening the project; but happily that didn't happen for me.
The bulk of this code only does anything if we enable New Arch. I'm not sure if some of the other bits might have some behavioral effect, though. This squashes together the following template changes upstream on the way to RN v0.68: facebook/react-native@73a04d145 Refactor the iOS app template to move setups to a helper class facebook/react-native@8ec0e6919 Add turbo module support in the default app template 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@6a046fb9f Fix build issue for the new app template facebook/react-native@e330eee9c Add Other C++ flags when RCT_NEW_ARCH_ENABLE=1 facebook/react-native@ccd170809 … Consider relative to pwd installation root … They use new APIs introduced to RN at the same time, some of which get refactored further by the later commits; the last one fixes a bug a previous one introduced in the Podfile. It also includes: * Fixing the type of `rootView` back to the more specific `RCTRootView *`, rather than `UIView *`. We need the more specific type when setting its `.loadingView` property below. * Re-following Expo instructions for adding Expo to an existing RN app: https://docs.expo.dev/bare/installing-expo-modules/#configuration-for-ios This just requires the one hunk of that diff that changes the initializer for `rootView`.
Corresponds to the following upstream commit on the way to RN v0.68: facebook/react-native@b352aa369 Do not opt-out […]/template from CLANGFORMAT
This follows a change in the template app on the way to RN v0.68: facebook/react-native@f595a4e68 Duplicate Xcodeproj UUIDs in iOS app template Podfile fixed The change is one that seems to be needed only with Fabric / New Arch: https://reactnative.dev/docs/0.68/new-architecture-app-renderer-ios#1-enable-fabric-in-podfile But it sounds like it's working around a CocoaPods mess (one in perhaps the same spirit as the bad caching we perennially see from CocoaPods.) May as well take it now.
As explained by the comment.
For detailed notes on changes in the template app and how they're accounted for, see: #5344 (comment)
The first commit is the same as #5608 .
Fixes: #5344