Follow some template-app changes on the way to RN v0.64. #4887

chrisbobbe · 2021-07-08T15:49:48Z

(Another one was #4846.)

Toward #4426, but this doesn't do the actual upgrade in package.json yet.

I've tested this in debug and release builds on my iPhone, and in a debug build on the office Android device (see #4886 for release). From some poking around in the message list and elsewhere, all seems fine.

The most important-looking change, to me, looks like

metro config: Turn on inline requires.

This is a feature that's been available since RN v0.59, but that they've now started turning on in the template app. It gets its own section in the RN v0.64 release announcement: https://reactnative.dev/blog/2021/03/12/version-0.64#inline-requires-enabled-by-default. The RN v0.64 changelog entry warns of a breaking change: "slightly different JS execution order". I haven't yet noticed any problems this seems to cause.

I plan to post screenshots of the Xcode UI for a few commits where I think that'll be helpful. (edit: done; see the two comments that follow.)

chrisbobbe · 2021-07-08T15:54:46Z

To follow some of facebook/react-native@6685aba46:

ios build: Run "Update to recommended settings" for ZulipMobile project.

chrisbobbe · 2021-07-08T15:55:14Z

To follow facebook/react-native@538446e75:

ios build: Bump Xcode compatibility to 12.0.

Done on Xcode "Version 12.5 (12E262)" in the GUI; screenshot at zulip#4887 (comment). Done to follow some of the template-app changes in facebook/react-native@6685aba46, on the path to the RN v0.64 upgrade. In particular, this bullet point: > - Update to recommended settings for Xcode 12 I didn't use a similar option that presented itself for the Pods project. (We don't make any changes corresponding to the "Add -e to the bundle images script" bullet point; see https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20v0.2E64.20upgrade.3A.20Xcode.20config.20changes.3F/near/1209893.)

Done to follow the template-app change in facebook/react-native@538446e75, on the path to the RN v0.64 upgrade. Done on Xcode "Version 12.5 (12E262)", in the GUI [1]. The commit message also mentions bumping CocoaPods to 1.10.0. Ours is already at 1.10.1 (see ios/Podfile.lock), so, nothing to do there. After making the change in the GUI, there were auto-generated changes to the project.pbxproj that looked a lot like the ones in facebook/react-native@538446e75. Then, after I did `pod install`, CocoaPods updated some of its own `PBXShellScriptBuildPhase`s (marked with `[CP]` in their names) with the `{input,output}Paths -> {input,output}FileListPaths` changes that also appear here. [1] I found a way to do this described on Stack Overflow: https://stackoverflow.com/questions/49622341/failed-to-load-project-incompatible-project-version-pop-up-appears-when-i-op. I selected "ZulipMobile" in the "Project navigator" in the left sidebar. Then, in the "File inspector" in the right sidebar, under "Project Document", I chose "Xcode 12.0-compatible" in the "Project Format" dropdown (from its previous value of "Xcode 3.2-compatible"). See a screenshot at zulip#4887 (comment).

gnprice · 2021-07-08T20:52:31Z

metro config: Turn on inline requires.

It looks like this is the RN docs on this feature:
https://reactnative.dev/docs/ram-bundles-inline-requires

It's not real clear about the role of that Metro config option. The option doesn't appear to be documented in the Metro docs:
https://facebook.github.io/metro/docs/configuration

My guess as to what the option means is that when we have a require function call (as opposed to an import statement), when this option is false, the required module actually ends up getting imported statically. (I.e., its top-level code gets executed before the requiring module's top-level code is executed.) And when the option is true, then the required module only gets imported lazily, if and when the require call actually happens.

If that's right, then the new behavior certainly makes more sense. Also, if that's right then the effect is limited to places where we have require calls, which we have very few of … in fact, a quick grep tells me we have just one outside of tests, namely where we import immutable-devtools conditioned on a debug build.

Anyway, we probably don't need to pin down exactly what this undocumented feature actually does. As long as things still seem to work, I'm happy to assume it didn't break anything.

gnprice

Thanks for all this upgrade work! I appreciate the clear descriptions of where these changes came from -- particularly the Xcode project file, where the changes can easily get gnarly.

Comments below, all small (and some are just remarks on the nice changes from upstream, nothing to change.) Please merge at will modulo those.

gnprice · 2021-08-04T05:20:37Z

ios/Podfile

-  # "Autolinking": automatically link pods that depend on React
-  # Native, unless omitted in our own `react-native.config.js`.
-  use_native_modules!
+  use_react_native!(:path => config[:reactNativePath])


This isn't super well explained, but it looks like their claim for this being needed starts by mentioning @react-native-community/cli@e949e234b, which was released in v4.10.1 of @react-native-community/cli. And `yarn why` tells me that we're on v4.14.0 of that package, so it

This probably means the intermediate state with config["reactNativePath"] is broken, right? So probably we should just squash these two changes together.

gnprice · 2021-08-04T05:21:44Z

.prettierrc.js

@@ -13,4 +13,5 @@ module.exports = {
  parser: 'babel-flow',
  singleQuote: true,
  trailingComma: 'all',
+  arrowParens: 'avoid',


commit-message comment:

That commit reports that the default behavior before Prettier 2.0 is like setting `arrowParens: 'avoid'`. It's looking unlikely that we have an easy path to using Prettier 2.0 (see prettier/prettier-eslint-cli#304), but we might as well follow the RN template app.

This looks like a previous draft of the same material that's in the rest of the commit message. Probably you meant to delete this version?

gnprice · 2021-08-04T05:29:25Z

ios/ZulipMobile.xcodeproj/project.pbxproj

-			inputPaths = (
-				"${PODS_ROOT}/Target Support Files/Pods-ZulipMobile/Pods-ZulipMobile-resources.sh",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/AntDesign.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/Entypo.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/EvilIcons.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/Feather.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/FontAwesome.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/FontAwesome5_Brands.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/FontAwesome5_Regular.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/FontAwesome5_Solid.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/Fontisto.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/Foundation.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/Ionicons.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/MaterialCommunityIcons.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/MaterialIcons.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/Octicons.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/SimpleLineIcons.ttf",
-				"${PODS_ROOT}/../../node_modules/react-native-vector-icons/Fonts/Zocial.ttf",
-				"${PODS_CONFIGURATION_BUILD_DIR}/React-Core/AccessibilityResources.bundle",
+			inputFileListPaths = (
+				"${PODS_ROOT}/Target Support Files/Pods-ZulipMobile/Pods-ZulipMobile-resources-${CONFIGURATION}-input-files.xcfilelist",


Neat -- this looks like a definite improvement in decoupling our Xcode project file from the details of the libraries we use.

From the way you produced this commit, it sounds like this is probably something CocoaPods has supported doing for at least a little while, but depends on a newish Xcode feature (this inputFileListPaths option) that wasn't available in Xcode 3.2 (!), so CocoaPods couldn't make use of it as long as the compatibilityVersion setting was so ancient.

gnprice · 2021-08-04T05:31:47Z

package.json

  "devDependencies": {
-    "@babel/core": "^7.11.4",
+    "@babel/core": "^7.12.9",
    "@babel/preset-env": "^7.11.0",
-    "@babel/runtime": "^7.8.4",
+    "@babel/runtime": "^7.12.5",


deps: Follow RN in updating some dev dependencies.

Worth mentioning somewhere in the commit message that this doesn't change any of the versions we actually end up using -- our yarn.lock already contained versions as new as all of these. It just bumps the explicit lower bounds in the package.json.

gnprice · 2021-08-04T05:35:22Z

ios/ZulipMobile.xcodeproj/project.pbxproj

-				IPHONEOS_DEPLOYMENT_TARGET = 11.0;
-				LD_RUNPATH_SEARCH_PATHS = "$(inherited) @executable_path/Frameworks";
+				IPHONEOS_DEPLOYMENT_TARGET = 12.0;
+				LD_RUNPATH_SEARCH_PATHS = (
+					"$(inherited)",
+					"@executable_path/Frameworks",
+				);


This is also a nice change. The old version looks like it probably would have broken if you have a directory name with a space ( ) in it somewhere above your worktree! That is a classic kind of bug for old shell scripts to have… but even there it rather went out of style sometime in the aughts.

Like we did in 9683540, but this time it's for the RN v0.64 upgrade.

Done to follow the template-app change in facebook/react-native@cd13c99d0 [1], on the path to the RN v0.64 upgrade. Greg points out [2] that a later commit, facebook/react-native@e599d6c5d, suggests that the `config["reactNativePath"]` form in the earlier commit is actually probably broken. So, we squash that in. That later commit isn't very well explained, but it looks like their claim for the change being needed starts by mentioning @react-native-community/clie949e234b, which was released in v4.10.1 of @react-native-community/cli. `yarn why` tells me that we're on v4.14.0 of that library, so it seems fine and good to take this. [1] It actually looks like we maybe could/should have taken this change when we did the RN v0.63 upgrade; it was backported to v0.63 in facebook/react-native@696fb5588. Looks like we didn't; oops. [2] zulip#4887 (comment)

Done to follow the template-app change in facebook/react-native@d8e6c4578, on the path to the RN v0.64 upgrade.

Done now, to follow the template-app change in facebook/react-native@fcffb961f, on the path to the RN v0.64 upgrade. That commit reports that this lint rule is no longer needed because it's turned on by default. This is corroborated by the Flow changelog for v0.98.0 [1], and I verified it experimentally -- using `$Supertype` (one of the deprecated utility types this rule is meant to catch [2]) is flagged as an error with or without this line in our config. [1] https://github.com/facebook/flow/blob/master/Changelog.md#0980 [2] https://flow.org/en/docs/linting/rule-reference/#toc-deprecated-utility

Done to follow the template-app change in facebook/react-native@959365a90, on the path to the RN v0.64 upgrade. This looks like the change that gets its own section in the RN v0.64 release announcement [1]. The RN commit links to the RN v0.59 release announcement [2]; that's the version where this became available, as an experimental feature. Both release announcements advertise a performance benefit. The changelog entry for v0.64 [3] warns of a breaking change: "slightly different JS execution order". I tested on the office Android device (debug build) and my own iPhone (debug and release builds), and I didn't see any issues after navigating around the app a bit. [1] https://reactnative.dev/blog/2021/03/12/version-0.64#inline-requires-enabled-by-default [2] https://reactnative.dev/blog/2019/03/12/releasing-react-native-059#-faster-app-launches-with-inline-requires [3] https://github.com/react-native-community/releases/blob/master/CHANGELOG.md#v0640

`yarn why` tells me we don't have an NPM package called "warning", so I can't imagine what this line might have been doing for us. Anyway, done to follow the template-app change in facebook/react-native@54e19a6b7, on the path to the RN v0.64 upgrade.

Done to follow the template-app change in facebook/react-native@635ac1ba5, on the path to the RN v0.64 upgrade. The default behavior on our Prettier version, according to the doc [1], is already `arrowParens: 'avoid'`, but that default will change to 'always' in Prettier v2.0.0. The RN commit sets it to 'avoid', explicitly, to avoid a silent change of behavior when upgrading. Might as well do the same, although the path to us actually using Prettier v2 might be complicated; see prettier/prettier-eslint-cli#304. [1] https://prettier.io/docs/en/options.html#arrow-function-parentheses

Done to follow the template-app change in facebook/react-native@e1bf515ae, on the path to the RN v0.64 upgrade. The motivation for this change isn't one we share with the template app: our build process doesn't directly output a different APK per ABI (CPU architecture) [1], so it doesn't matter if the ABI is human-readable in the version code. But we might as well follow along for the sake of staying close to the template app; we don't see an obvious reason not to. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Multiple.20APKs.3F/near/1131637

Done to follow the template-app change in facebook/react-native@216037757, on the path to the RN v0.64 upgrade.

Done on Xcode "Version 12.5 (12E262)" in the GUI; screenshot at zulip#4887 (comment). Done to follow some of the template-app changes in facebook/react-native@6685aba46, on the path to the RN v0.64 upgrade. In particular, this bullet point: > - Update to recommended settings for Xcode 12 I didn't use a similar option that presented itself for the Pods project. (We don't make any changes corresponding to the "Add -e to the bundle images script" bullet point; see https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20v0.2E64.20upgrade.3A.20Xcode.20config.20changes.3F/near/1209893.)

Done on Xcode "Version 12.5 (12E262)" in the GUI. Done to follow some of the template-app changes in facebook/react-native@6685aba46, on the path to the RN v0.64 upgrade. In particular, this bullet point: > - Remove the main.jsbundle file that doesn't exists, this file is > included automatically in the app bundle and doesn't need to be > in xcode (it won't even be at that path). (We don't make any changes corresponding to the "Add -e to the bundle images script" bullet point; see https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20v0.2E64.20upgrade.3A.20Xcode.20config.20changes.3F/near/1209893.)

Done to follow the template-app change in facebook/react-native@538446e75, on the path to the RN v0.64 upgrade. Done on Xcode "Version 12.5 (12E262)", in the GUI [1]. The commit message also mentions bumping CocoaPods to 1.10.0. Ours is already at 1.10.1 (see ios/Podfile.lock), so, nothing to do there. After making the change in the GUI, there were auto-generated changes to the project.pbxproj that looked a lot like the ones in facebook/react-native@538446e75. Then, after I did `pod install`, CocoaPods updated some of its own `PBXShellScriptBuildPhase`s (marked with `[CP]` in their names) with the `{input,output}Paths -> {input,output}FileListPaths` changes that also appear here. [1] I found a way to do this described on Stack Overflow: https://stackoverflow.com/questions/49622341/failed-to-load-project-incompatible-project-version-pop-up-appears-when-i-op. I selected "ZulipMobile" in the "Project navigator" in the left sidebar. Then, in the "File inspector" in the right sidebar, under "Project Document", I chose "Xcode 12.0-compatible" in the "Project Format" dropdown (from its previous value of "Xcode 3.2-compatible"). See a screenshot at zulip#4887 (comment).

….64.0. Done to follow the template-app change in facebook/react-native@06fedcd32, on the path to the RN v0.64 upgrade. Apparently this is fine; we don't get a peer-dep warning.

Done to follow the template-app change in facebook/react-native@fffa4d1ae, on the path to the RN v0.64 upgrade. I ignored the deps we don't use, and I ignored the ones for which we already specify a higher lower bound. Squinting at the `yarn.lock`, this doesn't actually change any of the versions we actually end up using. It just bumps the explicit lower bounds in the `package.json`.

Done to follow the template-app change in facebook/react-native@7159bcbf7, on the path to the RN v0.64 upgrade.

chrisbobbe · 2021-08-04T15:35:00Z

Please merge at will modulo those.

Done, thanks for the review!

chrisbobbe requested review from gnprice and WesleyAC July 8, 2021 15:49

chrisbobbe added the upstream: RN Issues related to an issue in React Native label Jul 8, 2021

chrisbobbe added the P1 high-priority label Jul 8, 2021

chrisbobbe force-pushed the rn-64-prep branch from f2f1609 to 610b7ae Compare July 8, 2021 16:13

chrisbobbe force-pushed the rn-64-prep branch from 610b7ae to 782b86a Compare July 8, 2021 16:16

gnprice reviewed Aug 4, 2021

View reviewed changes

chrisbobbe added 14 commits August 4, 2021 09:23

android [nfc]: Format some files.

2fad173

Like we did in 9683540, but this time it's for the RN v0.64 upgrade.

android: Move DevSettingsActivity from main to debug manifest.

fc17b21

Done to follow the template-app change in facebook/react-native@d8e6c4578, on the path to the RN v0.64 upgrade.

metro config [nfc]: Auto-format.

1939328

ios build [nfc]: Remove more remnants of tvOS.

64e5f8c

Done to follow the template-app change in facebook/react-native@216037757, on the path to the RN v0.64 upgrade.

deps: Bump metro-react-native-babel-preset's range from ^0.63.0 to ^0…

cabab22

….64.0. Done to follow the template-app change in facebook/react-native@06fedcd32, on the path to the RN v0.64 upgrade. Apparently this is fine; we don't get a peer-dep warning.

chrisbobbe force-pushed the rn-64-prep branch from 782b86a to ba85b2a Compare August 4, 2021 14:08

chrisbobbe added 2 commits August 4, 2021 11:31

flipper android: Upgrade from 0.54.0 to 0.75.1.

aa70f10

Done to follow the template-app change in facebook/react-native@7159bcbf7, on the path to the RN v0.64 upgrade.

chrisbobbe force-pushed the rn-64-prep branch from ba85b2a to aa70f10 Compare August 4, 2021 15:31

chrisbobbe merged commit aa70f10 into zulip:master Aug 4, 2021

chrisbobbe deleted the rn-64-prep branch August 4, 2021 16:10

chrisbobbe mentioned this pull request Sep 8, 2021

Upgrade to RN v0.64! #4991

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Follow some template-app changes on the way to RN v0.64. #4887

Follow some template-app changes on the way to RN v0.64. #4887

chrisbobbe commented Jul 8, 2021 •

edited

Loading

chrisbobbe commented Jul 8, 2021

chrisbobbe commented Jul 8, 2021

gnprice commented Jul 8, 2021

gnprice left a comment

gnprice Aug 4, 2021

gnprice Aug 4, 2021

chrisbobbe Aug 4, 2021

gnprice Aug 4, 2021

gnprice Aug 4, 2021

gnprice Aug 4, 2021

chrisbobbe commented Aug 4, 2021

Follow some template-app changes on the way to RN v0.64. #4887

Follow some template-app changes on the way to RN v0.64. #4887

Conversation

chrisbobbe commented Jul 8, 2021 • edited Loading

chrisbobbe commented Jul 8, 2021

chrisbobbe commented Jul 8, 2021

gnprice commented Jul 8, 2021

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 4, 2021

Choose a reason for hiding this comment

gnprice Aug 4, 2021

Choose a reason for hiding this comment

chrisbobbe Aug 4, 2021

Choose a reason for hiding this comment

gnprice Aug 4, 2021

Choose a reason for hiding this comment

gnprice Aug 4, 2021

Choose a reason for hiding this comment

gnprice Aug 4, 2021

Choose a reason for hiding this comment

chrisbobbe commented Aug 4, 2021

chrisbobbe commented Jul 8, 2021 •

edited

Loading