RN v0.61 upgrade followup. #4152

chrisbobbe · 2020-06-11T22:55:15Z

~~Awaiting #4151.~~ Done!

chrisbobbe · 2020-07-09T19:47:08Z

package.json

@@ -116,6 +116,7 @@
    "jest-cli": "^24.9.0",
    "jest-environment-jsdom": "^24.9.0",
    "jest-environment-jsdom-global": "^1.2.0",
+    "jest-expo": "^37.0.0",


b706f4d jest: Add jest-expo preset, to be used in the next commit.

This commit is in this PR (instead of a PR that doesn't have RN v0.61) because I didn't want to bother figuring out all the appropriate versions (of this and other things we have to add with it, for peer dependency constraints) based on RN v0.60, which we'll move away from soon.

One thing I haven't looked into further, but plan to, is that this particular version of jest-expo isn't compatible with Jest 26.1.0, which we're thinking of switching to soon (see here). ISTR some errors with jest-expo using require.requireActual, which was removed in jestjs/jest#9854.

chrisbobbe · 2020-07-13T19:31:31Z

Cool, and #4151 has been merged! Thanks, @gnprice! I'll fix these conflicts and push this branch.

chrisbobbe · 2020-07-13T19:39:50Z

Mentioning this here, as #4091 will be closed automatically once this PR lands:

There'll be another important step after upgrading Unimodules, which would be easy to forget about, I think—c8d0c8d should be reverted once we're on the new version, IIUC.

I could make that part of this PR if you want, @gnprice.

gnprice · 2020-07-13T20:33:54Z

c8d0c8d should be reverted once we're on the new version, IIUC.

SGTM!

chrisbobbe · 2020-07-13T22:07:18Z

(huh, looking into the tools/verify-webview-js failure now, that's odd, and reproduces locally)

chrisbobbe · 2020-07-13T23:30:15Z

(huh, looking into the tools/verify-webview-js failure now, that's odd, and reproduces locally)

OK, just pushed a fix! Yikes, the culprit was a yarn yarn-deduplicate && yarn upon installing jest-expo, where tons of Babel-related changes were made to yarn.lock. One more reason to stop considering jest-expo, I suppose—the idea to use it was mostly a whim, though I slightly like not having to make boring mocks for expo- modules.

We'd like to upgrade to the latest, but versions 10.1.0 and above don't have support for RN v0.61; minimum supported is v0.62. (Hence the tilde.) Breaking changes announced: - v9: iOS: Props updates to `injectedJavaScript` are no longer immutable. We don't use the `injectedJavaScript` prop, so this is fine. - v10: gradle: The Android Gradle plugin is only required when opening the project stand-alone, not when it is included as a dependency. [...] This doesn't sound like a description of a breaking change. As Greg said [0], "a thing is no longer required in a case where it previously was". We hesitate to take an x.0.0 version -- what if there are important bugfixes in minor/patch versions for issues introduced in that x.0.0 version? -- but they've made it a custom to post a note at the release if they know of major regressions, telling you to use a later version [1]. And no such note exists at v10.0.0. As a concrete example, we were concerned that the "bug fixes" identified in the v10.1.0 release might have been aimed at regressions introduced in v10.0.0. But following links to the mentioned PR and its corresponding issues (the second looks like a resumption of the first), it looks like a fix for a bug reported all the way back in version 5. So this isn't a reason to use v9.x.x instead of v10.0.0. Greg points out [2]: """ In a project that takes major vs. minor releases seriously, where a minor release means cherry-picked backported changes, I'd assume from this context that the major release introduced the issue. (Projects like that include RN itself, and the Zulip server.) But with this project I get the impression that their releases are all a linear stream of snapshots from master. Also that they do the flavor of "semantic versioning" that's basically "bump the major version number frequently and at random". So that makes this less clear. """ Ah, well. Also, update the libdef according to changes in `src/WebViewTypes.ts` in the `react-native-webview` repo. [0]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.2060.20upgrade.3A.20react-native-webview/near/893807 [1]: See, e.g., v10.1.0, at https://github.com/react-native-community/react-native-webview/releases/tag/v10.1.0. [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20v0.2E61.3A.20react-native-webview/near/901944

This is possible, now that the RN v0.60 -> v0.61 upgrade is complete! [1] Change the `@unimodules/core` version, following a valid instruction in bc27f2d: """ So, declare @unimodules/core in our own "dependencies", with the version specified by react-native-unimodules in its active version. This matching should be done each time we change versions of react-native-unimodules. """ Also, remove some now-unnecessary code in `android/build.gradle`, as foreshadowed in cb87f90: """ Second, specify a new dependency for `unimodules-react-native-adapter` in our own `android/build.gradle`, in a fix that is necessary because we're locked on version 0.6.0 of react-native-unimodules. Filed as https://github.com/unimodules/react-native-unimodules/issues/130. """ There are some reasonable, automatic changes to the version-controlled files that ensure that unimodules are automatically linked [2]: `ios/Podfile.lock` and `android/app/src/main/java/com/zulipmobile/generated/BasePackageList.java`. These look like some modules that are part of `react-native-unimodules`' core; these additions are caused by the upgrade and don't stem from running something like `yarn add expo-apple-authentication` to get a particular Unimodule. Also, run `yarn yarn-deduplicate && yarn` as prompted by `tools/test deps`. [1]: See discussion of the various obstacles at https://github.com/unimodules/react-native-unimodules/issues/97#issuecomment-637714639 and https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Podfile.20dependency.20of.20a.20dependency/near/892373. [2]: This is similar to but distinct from `react-native`'s "autolinking" feature that we activated in f460460 and a9a9ac7. See https://github.com/unimodules/react-native-unimodules/issues/75#issuecomment-536517253 for a comment on that. Fixes: zulip#4091

A bit frustratingly, we need to install react-native-web, because it's a peer dep of jest-expo, and react-dom, because that's a peer dep of react-native-web. It's likely that we'll never run any code in either module, since we don't care about the web. (Expo seems to be expanding React Native's original claim for multi-platform compatibility, namely iOS and Android, to include the web. `react-native-web` looks like a thing that lets you write React Native...for the web.) At least they don't complain if we add them as dev dependencies instead of regular dependencies. Also, run `yarn yarn-deduplicate && yarn` as prompted by `tools/test deps`.

This calls `react-native`'s Jest setup code before running its own. In particular, it seems to be aware of all the Expo modules we might want to add using `react-native-unimodules`, and mocks them [1]. Since we don't need our own mocks for these, remove them. It seems like we still need to add entries in our `transformModulesWhitelist`, ah, well. But it's good to weed out boring mocks from our `jestSetup.js`, and leave only those that are interesting [2]. Also, it seems like each time we add a module from Expo, there's a debugging process that can be confusing [3]; so, nice to avoid that. It looks like the preset does explicitly consider the bare, non-"managed" Expo workflow [4], which we use. If `jest-expo` turns out to be buggy, or the dependency requirements get even more tangled or burdensome, we should feel free to abandon this effort; it's not terrible to have to add boring mocks. [1]: https://github.com/expo/expo/blob/b8bd30697/packages/jest-expo/src/preset/expoModules.js [2]: Seems like a few remain that aren't related to Expo. Hmm. [3]: https://github.com/zulip/zulip-mobile/pull/4034/files#r445956933 [4]: https://github.com/expo/expo/blob/b8bd30697/packages/jest-expo/src/preset/setup.js#L130

I'm not sure when exactly it became unnecessary to mock "Linking", but it's taken care of now in React Native's Jest setup [1]. [1]: https://github.com/facebook/react-native/blob/v0.61.5/jest/setup.js#L153

This reverts commit c8d0c8d. As planned [1] [2], now that we've upgraded react-native-unimodules. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/apple.20purpose.20strings/near/869928 [2] zulip#4091 (comment)

gnprice · 2020-07-15T01:26:58Z

Merged! Thanks again.

I looked into the Babel-related changes in chat, and as discussed there I merged some changes before this that isolated the Babel upgrades and simplified that commit back again.

This is a lot at once, but all our tests pass, and manual testing confirms that basic functionality is preserved on Android and iOS. We should make sure this spends some time out in a beta release before going out to all users. I'm guessing the changes to generatedEs3.js (made by running `tools/generate-webview-js` after this upgrade) have something to do with Babel; we've encountered something like this before: zulip#4152 (comment) Greg points out that "it's more practical to seriously test an infrequent bigger upgrade than many small ones" [1], and that "if we have a regression that we bisect to a big upgrade commit, we can still do a bisect on the upgrades themselves within the commit" [2]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Dependabot/near/1202825 [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Dependabot/near/1202866

chrisbobbe added upstream: RN Issues related to an issue in React Native blocked on other work To come back to after another related PR, or some other task. labels Jun 11, 2020

chrisbobbe requested a review from gnprice June 11, 2020 22:55

chrisbobbe force-pushed the rn-61-followup-1 branch from 7fbfa20 to a5bb3f0 Compare June 11, 2020 23:16

chrisbobbe marked this pull request as draft June 11, 2020 23:52

chrisbobbe force-pushed the rn-61-followup-1 branch 2 times, most recently from c89ab71 to 64819e4 Compare June 12, 2020 21:39

chrisbobbe mentioned this pull request Jul 1, 2020

MessageList long loading logs #4180

Merged

chrisbobbe force-pushed the rn-61-followup-1 branch from 64819e4 to 9bb1c4d Compare July 9, 2020 19:29

chrisbobbe commented Jul 9, 2020

View reviewed changes

chrisbobbe force-pushed the rn-61-followup-1 branch from 9bb1c4d to 8bac857 Compare July 11, 2020 02:10

chrisbobbe mentioned this pull request Jul 11, 2020

RN v61 upgrade #4151

Merged

chrisbobbe marked this pull request as ready for review July 11, 2020 02:11

chrisbobbe removed the blocked on other work To come back to after another related PR, or some other task. label Jul 13, 2020

chrisbobbe force-pushed the rn-61-followup-1 branch from 8bac857 to f6f9c00 Compare July 13, 2020 19:43

chrisbobbe mentioned this pull request Jul 13, 2020

Upgrade to RN v0.62 #3782

Closed

chrisbobbe force-pushed the rn-61-followup-1 branch from 6abbdff to c34b45c Compare July 13, 2020 21:40

chrisbobbe force-pushed the rn-61-followup-1 branch from c34b45c to 33ff273 Compare July 13, 2020 23:23

chrisbobbe added 6 commits July 14, 2020 18:09

jest: Comment about boring mocks, remove one that's unnecessary.

c1c9575

I'm not sure when exactly it became unnecessary to mock "Linking", but it's taken care of now in React Native's Jest setup [1]. [1]: https://github.com/facebook/react-native/blob/v0.61.5/jest/setup.js#L153

gnprice force-pushed the rn-61-followup-1 branch from 33ff273 to e55739e Compare July 15, 2020 01:25

gnprice merged commit e55739e into zulip:master Jul 15, 2020

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

chrisbobbe mentioned this pull request May 25, 2021

Small cleanups: fix peer-dep warnings, yarn-deduplicate, use Flow's exact_by_default. g0v/vaccinate#83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RN v0.61 upgrade followup. #4152

RN v0.61 upgrade followup. #4152

chrisbobbe commented Jun 11, 2020 •

edited by gnprice

Loading

chrisbobbe Jul 9, 2020 •

edited

Loading

chrisbobbe Jul 11, 2020 •

edited

Loading

chrisbobbe commented Jul 13, 2020

chrisbobbe commented Jul 13, 2020

gnprice commented Jul 13, 2020

chrisbobbe commented Jul 13, 2020 •

edited

Loading

chrisbobbe commented Jul 13, 2020 •

edited

Loading

gnprice commented Jul 15, 2020

RN v0.61 upgrade followup. #4152

RN v0.61 upgrade followup. #4152

Conversation

chrisbobbe commented Jun 11, 2020 • edited by gnprice Loading

chrisbobbe Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe Jul 11, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Jul 13, 2020

chrisbobbe commented Jul 13, 2020

gnprice commented Jul 13, 2020

chrisbobbe commented Jul 13, 2020 • edited Loading

chrisbobbe commented Jul 13, 2020 • edited Loading

gnprice commented Jul 15, 2020

chrisbobbe commented Jun 11, 2020 •

edited by gnprice

Loading

chrisbobbe Jul 9, 2020 •

edited

Loading

chrisbobbe Jul 11, 2020 •

edited

Loading

chrisbobbe commented Jul 13, 2020 •

edited

Loading

chrisbobbe commented Jul 13, 2020 •

edited

Loading