Pre-RN v0.61 changes by chrisbobbe · Pull Request #4150 · zulip/zulip-mobile

chrisbobbe · 2020-06-11T01:59:25Z

These will help smooth the RN v0.61 upgrade. I have a few more planned (hence the 1 in the branch name!) that I'm still working on, but @gnprice, you might like to see these.

gnprice

These look good, thanks! Small comments on a few of them.

`953fcf2` ios build tests: Follow RN template in RN v0.61.

s/build //; this is all in test code for the app, not for the build process.

(Also it's a stub test file we don't use -- so possibly we should just delete it, and if we ever do create a test like this in the future we can refer to the template to help.)

... Oh, in fact, as I actually read that test file and with the help of that upstream commit to illustrate what it does: this test will certainly fail, because what it tests is that we render the welcome screen of the hello-world app. It makes perfect sense to have in the template app, as an example of how to write these tests, but when left around in our app, the only thing it can do is cause confusion. So yeah, let's delete it. I think that means we should delete every mention of ZulipMobileTests. Some parts of our Xcode config will become noticeably simpler to navigate!

gnprice · 2020-06-12T00:03:08Z

-
-    @Override
-    public void onConfigurationChanged(Configuration newConfig) {
-        super.onConfigurationChanged(newConfig);
-        Intent intent = new Intent("onConfigurationChanged");
-        intent.putExtra("newConfig", newConfig);
-        this.sendBroadcast(intent);
-    }


Interesting, good catch!

Let's remove this from ReceiveShareActivity.kt too. I puzzled a bit over that when reviewing #4124 which added it... and then saw it was copied from MainActivity and I guess I figured it was generic RN boilerplate, and didn't study it closer. (But in retrospect this story makes more sense -- if it were generic RN boilerplate they could just put it in the base class.)

gnprice · 2020-06-12T00:12:34Z


        google()
        jcenter()
+        maven { url 'https://jitpack.io' }


I think we can skip this. The upstream PR that added it doesn't give any further information, and searching the web I don't see a lot of evidence of people using this service.

It'll be easy to add a line like this in the future as part of adopting any library that is published there.

gnprice · 2020-06-12T00:15:36Z

+jest.mock('@react-native-community/cameraroll', () => ({
+  // Incomplete, but error-free
+  saveToCameraRoll: jest.fn(),
+  save: jest.fn(),
+  getPhotos: jest.fn(),
+}));


Hmm, it's interesting that we need this now and didn't before! Do you understand what changed?

EDIT: Moving this discussion to CZO, here.

gnprice · 2020-06-12T00:21:34Z

  animatedValue = new Animated.Value(this.props.visible ? 1 : 0);

-  componentWillReceiveProps(nextProps: Props) {
+  UNSAFE_componentWillReceiveProps(nextProps: Props) {


The effect of this is basically to acknowledge that we've seen the warning, to tell it to stop warning us.

That's good to do, but before we do we should file an issue about the need to move away from these methods. It can link to that upstream announcement https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html to handle most of the specifics.

Makes sense! Opened as #4154.

This is a stub that's been hanging around for a long time without being used. As long as we don't use it, it just causes confusion. We made this decision [1] while considering some changes suggested by the upgrade helper [2] during the RN v0.60 -> v0.61 upgrade. In fact, Greg points out that what it's doing is in fact testing that we render *the welcome screen of the hello-world app*. So it makes sense to happen in the template app, as an example, but it's bound to fail in our app. The changes to the Xcode config were done entirely through the Xcode UI: removing the target and various references to it (e.g., in "Edit Scheme"), searching for the text "ZulipMobileTests" to see if we were done yet. [1]: zulip#4150 (review)

chrisbobbe · 2020-06-12T21:38:58Z

Thanks for the review! Ready for another one.

The old name is easy to read as modules *not to be compiled*.

In ca23523, we made a change to the Podfile without re-running `pod install` and committing the new checksum. So, do.

In 0d5a19a, for zulip#4147, we forgot to remove some Android configuration specific to `react-native-orientation` in MainActivity.java [1], added in 3149c9d. So, remove it. It appears that the iOS configuration at the `react-native-orientation` doc was never done, so there's nothing to do for that. Greg pointed out that this override was replicated in ReceiveShareActivity.kt when that file was created (in 0b84717) and that he was puzzled by it while reviewing zulip#4124 and supposed it might be generic RN boilerplate. Now that we've identified it, remove it. [1]: https://github.com/yamill/react-native-orientation#configuration

This is a stub that's been hanging around for a long time without being used. As long as we don't use it, it just causes confusion. We made this decision [1] while considering some changes suggested by the upgrade helper [2] during the RN v0.60 -> v0.61 upgrade. In fact, Greg points out that what it's doing is in fact testing that we render *the welcome screen of the hello-world app*. So it makes sense to happen in the template app, as an example, but it's bound to fail in our app. The changes to the Xcode config were done entirely through the Xcode UI: removing the target and various references to it (e.g., in "Edit Scheme"), searching for the text "ZulipMobileTests" to see if we were done yet. [1]: zulip#4150 (review)

Part of the RN v0.60 -> v0.61 changes to the template app [1], corresponding to facebook/react-native@a7b9be4a1. This can freely be done before the upgrade commit because the presence or absence of this file isn't associated with the upgrade, but rather (it seems) a detail of how the app was created in the very beginning. Judging from the template app, it seems that this line is meant to address the potential existence of a directory at `ios/ZulipMobile.xcodeproj/project.xcworkspace`. We don't have a directory there, whether or not it's git-ignored. Greg mentions in discussion [2] a quote from a blog post [3] with some plausible details: """ It is possible to just have an .xcodeproj visible to the desktop, and have a .xcworkspace directory inside the .xcodeproj directory. In this case, the workspace is always called `project.xcworkspace` and is at the root directory of the project. This is what you get if you use the Xcode wizard to create a new project first, instead of creating a workspace and adding projects to it. """ Greg concludes that if we did have a directory there, it would belong in source control. So it doesn't belong in the .gitignore, and it's good that they fixed that in the template. [1]: https://react-native-community.github.io/upgrade-helper/?from=0.60.6&to=0.61.5 [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/.23M3781.20RN.20v0.2E61.20upgrade/near/900957 [3]: http://neurocline.github.io/dev/2016/04/16/xcode-xcworkspace-and-xcodeproj.html

Done by selecting `Code > Reformat Code` in Android Studio, to apply our preferred formatting, as configured in the version-controlled files in `android/.idea`. Some of these changes coincide with changes to the template app in facebook/react-native@9b0adb5ad, which shows up in the RN v0.60 -> v0.61 upgrade guide [1]. [1]: https://react-native-community.github.io/upgrade-helper/?from=0.60.6&to=0.61.5

`CameraRoll` in React Native core is deprecated [1]. In the upcoming RN v0.60 -> v0.61 upgrade, we see errors that it has in fact been removed from `react-native`. We are to use `@react-native-community/cameraroll` instead. So, do. Very recently, the `saveToCameraRoll` method was removed in favor of `save` [2]. The first argument is exactly the same; further config is possible in later arguments if we want to. Thankfully, this library has built-in Flow support, so we don't need to make our own libdef. But the types haven't yet been updated to reflect the removal of `saveToCameraRoll`; that should be coming soon [3]. Also tell our Jest config to use Babel on `node_modules/@react-native-community/cameraroll`, as it's not compiled. [1]: https://reactnative.dev/docs/0.60/cameraroll [2]: react-native-cameraroll/react-native-cameraroll#183 [3]: react-native-cameraroll/react-native-cameraroll#184

With Flow v0.105.0, which we'll start using along with the RN v0.60 -> v0.61 upgrade [1], we get some new errors that this commit fixes. We never precisely identified the cause of these errors [2], but it also wasn't completely clear why we redefined `StoreEnhancer` instead of importing it from Redux, in f3c6ff7. From reading the commit message, it seems likely that we wanted to cut down on complexity by not supporting some features that we don't use, which makes sense as a strategy. Kicking the tires, there don't appear to be useful checks being suppressed. [1]: https://react-native-community.github.io/upgrade-helper/?from=0.60.6&to=0.61.5 [2]: See discussion at https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20v0.2E105.2E0.20upgrade/near/899790.

In the upcoming RN v0.60 -> v0.61 upgrade [1], we'll also upgrade to React v16.9.0, which continues the path toward deprecating some lifecycle methods by logging warnings to the console unless you prefix the methods with `UNSAFE_` [2]. Some background on how these methods came to be considered harmful: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html. React v16.9.0 suggests (without requiring) the following [2]: """ However, maybe you don’t have the time to migrate or test these components. In that case, we recommend running a “codemod” script that renames them automatically: ``` cd your_project npx react-codemod rename-unsafe-lifecycles ``` """ So, run that script. That means that we won't get noisy warnings in the console, but the method names will stand out as requiring a fix at some point; in particular, in an as-yet-unreleased React v17. Actually removing our uses of these methods is zulip#4154. [1]: https://react-native-community.github.io/upgrade-helper/?from=0.60.6&to=0.61.5 [2]: https://reactjs.org/blog/2019/08/08/react-v16.9.0.html

gnprice · 2020-06-12T22:24:56Z

Thanks for the revisions! Looks great -- merged.

chrisbobbe requested a review from gnprice June 11, 2020 01:59

chrisbobbe force-pushed the rn-61-prequel-1 branch 3 times, most recently from 4be55c9 to 7936287 Compare June 11, 2020 19:15

chrisbobbe mentioned this pull request Jun 11, 2020

RN v61 upgrade #4151

Merged

gnprice reviewed Jun 12, 2020

View reviewed changes

This was referenced Jun 12, 2020

Upgrade to RN v0.61 #3781

Closed

Replace unsafe React component lifecycle methods. #4154

Open

chrisbobbe force-pushed the rn-61-prequel-1 branch from 7936287 to a0633c6 Compare June 12, 2020 20:15

chrisbobbe force-pushed the rn-61-prequel-1 branch from a0633c6 to 038f528 Compare June 12, 2020 21:37

chrisbobbe added 9 commits June 12, 2020 15:24

jest [nfc]: Rename uncompiledModules to transformModulesWhitelist.

658d6ab

The old name is easy to read as modules *not to be compiled*.

ios deps [nfc]: Fix Podfile checksum after NFC comment edit.

bfd637e

In ca23523, we made a change to the Podfile without re-running `pod install` and committing the new checksum. So, do.

gnprice force-pushed the rn-61-prequel-1 branch from 038f528 to 8f849e5 Compare June 12, 2020 22:24

gnprice merged commit 8f849e5 into zulip:master Jun 12, 2020

chrisbobbe mentioned this pull request Aug 29, 2020

Upgrade to RN v0.62 #3782

Closed

chrisbobbe deleted the rn-61-prequel-1 branch November 6, 2020 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Pre-RN v0.61 changes#4150

Pre-RN v0.61 changes#4150
gnprice merged 9 commits intozulip:masterfrom
chrisbobbe:rn-61-prequel-1

chrisbobbe commented Jun 11, 2020

Uh oh!

gnprice left a comment

Uh oh!

gnprice Jun 12, 2020

Uh oh!

gnprice Jun 12, 2020 •

edited

Loading

Uh oh!

gnprice Jun 12, 2020

Uh oh!

chrisbobbe Jun 12, 2020 •

edited

Loading

Uh oh!

chrisbobbe Jun 12, 2020 •

edited

Loading

Uh oh!

gnprice Jun 12, 2020

Uh oh!

chrisbobbe Jun 12, 2020

Uh oh!

chrisbobbe commented Jun 12, 2020

Uh oh!

gnprice commented Jun 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

chrisbobbe commented Jun 11, 2020

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

953fcf2 ios build tests: Follow RN template in RN v0.61.

Uh oh!

gnprice Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 12, 2020

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 12, 2020

Uh oh!

gnprice commented Jun 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`953fcf2` ios build tests: Follow RN template in RN v0.61.

gnprice Jun 12, 2020 •

edited

Loading

chrisbobbe Jun 12, 2020 •

edited

Loading

chrisbobbe Jun 12, 2020 •

edited

Loading