Run `yarn upgrade`, after treating some deps individually. by chrisbobbe · Pull Request #4789 · zulip/zulip-mobile

chrisbobbe · 2021-06-08T14:53:46Z

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.

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

In this revision, tools/test --all passed for me on every commit.

For the react-native-safari-view -> expo-web-browser change, I tested social auth, a link within a message, and the "Forgot password?" link.

WesleyAC

Thanks! Looks good overall, just a few comments.

WesleyAC · 2021-06-08T18:36:47Z

  - RNReanimated (1.13.0):
    - React
-  - RNSentry (2.2.1):
+  - RNSentry (2.4.3):


We should have done this in 7dce4e7. It looks like it was missed
because the quality-of-life improvement in 683f387 came with the
side effect of skipping pod install even when autolinking wants to
upgrade the Podfile.lock because of an NPM package upgrade. Hmm.

Any ideas/plans on how to fix this in general?

Hmm. I think I'll revert 683f387 for now, and ponder a better fix later.

WesleyAC · 2021-06-08T18:37:49Z

    "prettier-eslint": "^12.0.0",
    "prettier-eslint-cli": "^5.0.0",
-    "react-dom": "16.11.0",
+    "react-dom": "16.13.1",


(see history)

(in the commit message) — can you give a commit hash to look at here?

I'll write down a git command to run to see the history of changes in package.json relevant to react-dom; I think that might be more helpful than a single commit.

WesleyAC · 2021-06-08T18:40:35Z

    "lodash.uniqby": "^4.4.0",
    "react": "16.13.1",
-    "react-intl": "5.8.6",
+    "react-intl": "5.20.1",


Did you test using the app in a different language?

Yes; I'll make a note that I did, in the commit message.

WesleyAC · 2021-06-08T18:43:59Z

-  version "1.18.2"
-  resolved "https://registry.yarnpkg.com/prettier/-/prettier-1.18.2.tgz#6823e7c5900017b4bd3acf46fe9ac4b4d7bda9ea"
-  integrity sha512-OeHeMc0JhFE9idD4ZdtNibzY0+TPHSpSSb9h8FqtP+YnoZZ1sl8Vc9b1sasjfymH3SonAF4QcA2+mzHPhMvIiw==
+  version "1.19.1"


Looks like this requires quite a few formatting changes, and I'm not
sure I like all of them -- in test files, especially, it's nice when
we can fit something simple onto a single line. Ah, well.

Can we change the settings to not require the changes that we don't like? I'd prefer to just drop this commit from this PR, so we can talk about formatting changes separately from the rest of these updates.

Sure. Prettier isn't very configurable (doc), and my hopes aren't very high that we'll be able to get what we want in Prettier's config. Possibly there's something we could do in tools/formatting.eslintrc.yaml, which (with prettier-eslint-cli) we use to overrule some of Prettier's opinions by running ESLint with some targeted rules.

For strategy on removing the commit from this PR (while keeping the headline big-upgrade commit):

I'm wary of making the catch-all upgrade commit more complicated; it would be great not to have to change the command there from yarn upgrade to something like yarn upgrade $(tools/deps | grep -vx -e prettier) and explain what prettier is doing in the command there.

One option would be to have a commit that pins prettier to its current version (using a range without a caret), and then yarn upgrade would see that and not try to upgrade it. Then after this PR is in, we can think about upgrading prettier and adding a caret back.

One option would be to have a commit that pins prettier to its current version (using a range without a caret), and then yarn upgrade would see that and not try to upgrade it. Then after this PR is in, we can think about upgrading prettier and adding a caret back.

Yeah, that'd be a fine alternative to the $(tools/deps | grep -v …) thing.

Also note that we can always disable Prettier locally with // prettier-ignore -- that applies to just the immediately following statement or expression, so it's generally pretty finely targeted. We have a few dozen of those already, and I think it's appropriate to have a fairly low threshold for applying it when Prettier tries to do something ugly.

gnprice

Thanks for working through these! All looks good modulo Wesley's comments above and a few small other comments.

For the react-native-safari-view -> expo-web-browser change, I tested social auth, a link within a message, and the "Forgot password?" link.

This tidbit would be helpful to add to that commit's commit message. For one thing, it helps us find that to jog our memory for things to test in future major upgrades to that library.

gnprice · 2021-06-09T06:29:27Z

-        map: Immutable.Map([['', 234], ['1', 123], ['1,2', 345]]),
+        map: Immutable.Map([
+          ['', 234],
+          ['1', 123],
+          ['1,2', 345],
+        ]),


(For when we do the Prettier upgrade, whether now or later:)

If you feel like just adding some prettier-ignore comments on this kind of diff hunk, to keep these blobs of test data compact, I'd be for it.

gnprice · 2021-06-09T06:31:13Z

-export const tryGetAuth: Selector<Auth | void> = createSelector(
-  tryGetActiveAccount,
-  account => {
-    if (!account || account.apiKey === '') {
-      return undefined;
-    }
-    return authOfAccount(account);
-  },
-);
+export const tryGetAuth: Selector<Auth | void> = createSelector(tryGetActiveAccount, account => {
+  if (!account || account.apiKey === '') {
+    return undefined;
+  }
+  return authOfAccount(account);
+});


This change OTOH seems like an improvement.

gnprice · 2021-06-09T06:32:30Z

+export const getAccountStatuses: Selector<
+  $ReadOnlyArray<AccountStatus>,
+> = createSelector(getAccounts, accounts =>
+  accounts.map(({ realm, email, apiKey }) => ({ realm, email, isLoggedIn: apiKey !== '' })),


And this change seems one step forward, one step back -- the more compact createSelector arguments seem nice, but the type parameter set off on its own line is silly. On net, probably not worth fighting Prettier over.

gnprice · 2021-06-09T06:35:51Z

-  version "1.18.2"
-  resolved "https://registry.yarnpkg.com/prettier/-/prettier-1.18.2.tgz#6823e7c5900017b4bd3acf46fe9ac4b4d7bda9ea"
-  integrity sha512-OeHeMc0JhFE9idD4ZdtNibzY0+TPHSpSSb9h8FqtP+YnoZZ1sl8Vc9b1sasjfymH3SonAF4QcA2+mzHPhMvIiw==
+  version "1.19.1"


Also note that we can always disable Prettier locally with // prettier-ignore -- that applies to just the immediately following statement or expression, so it's generally pretty finely targeted. We have a few dozen of those already, and I think it's appropriate to have a fairly low threshold for applying it when Prettier tries to do something ugly.

Looks like this requires some formatting changes. Some of them look OK, some neutral [1]. In a few places where we'd really like to keep test data compact, add some `prettier-ignore`s. Changelog: https://github.com/prettier/prettier/blob/main/CHANGELOG.md [1] zulip#4789 (comment)

chrisbobbe · 2021-06-09T18:48:57Z

Thanks for the reviews! Revision pushed.

gnprice · 2021-06-09T19:30:37Z

Thanks for the revision! All LGTM.

I'll let Wesley comment on whether they think the remaining Prettier format changes still need more discussion (#4789 (comment) ), but otherwise I think this is ready to merge.

WesleyAC · 2021-06-10T22:52:59Z

I'm fine to merge this with the prettier changes.

We should have done this in 7dce4e7. It looks like it was missed because the quality-of-life improvement in 683f387 came with the side effect of skipping `pod install` even when autolinking wants to upgrade the Podfile.lock because of an NPM package upgrade. Hmm.

This reverts commit 683f387. That quality-of-life change caused a problem, addressed in the previous commit, where we skipped running `pod install` when we should have run it. We should have run it because React Native's autolinking [1] needed to do its job following an update to an NPM package, by propagating Pod version changes from a .podspec file in node_modules/ to ios/Podfile.lock. The conditional wasn't checking for any relevant NPM package updates since the last time `pod install` was run; it was only checking for changes to ios/Podfile.lock. [1] https://github.com/react-native-community/cli/blob/master/docs/autolinking.md

This seems to work to unblock an error like unexpected element <queries> found in <manifest> when trying to upgrade a dependency with Android native code. And it's always nice to keep the Android Gradle Plugin up-to-date. From a Stack Overflow post: https://stackoverflow.com/a/62969918/6792075 Release notes: https://developer.android.com/studio/releases/gradle-plugin#3-5-0.

We don't actually use react-dom; it's there for a kind of silly reason (see 62621ef and related history: `git log --stat -p -G react-dom -- package.json`). But, as long as we include it, we should have a sensible plan for keeping it up-to-date. Might as well do that by having it track with react.

Ideally, we'd have written this in a way that pointed to the version we wanted, while also permitting upgrades [1]. There's no particular change we're interested in right now; we just want to upgrade for the sake of upgrading. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/non-caret.20dependency.20ranges/near/1203007

And make a libdef with flowgen. Despite saying that the project expects to maintain compatibility with (only) the latest version of React Native, react-native-safari-view's README has a badge saying react-native 0.40, which is outdated by multiple years, and there's been very little activity and few recent commits. So, move to the Expo ecosystem, hoping it's better maintained. While expo-web-browser is meant to work for both iOS and Android, take a conservative approach here and only replace the uses of SafariView. I tested social auth, a link within a message, and the "Forgot password?" link on my iOS device.

Like in 8038528, we restrict the version range to that single version (instead of using a caret or a tilde), since `react-intl` doesn't invite users to expect them to handle breaking changes well [1]. There's no particular change we're interested in right now; we just want to upgrade for the sake of upgrading. Nothing stands out in the changelog [2] as being a breaking change, but we don't really trust the package, as mentioned, so the easiest way to discover any will probably be to have it out in a beta for a little while. I did test switching to a different language, and things didn't seem to break when I did that. [1] formatjs/formatjs#2067 (comment). [2] https://github.com/formatjs/formatjs/blob/main/packages/react-intl/CHANGELOG.md

What we really mean, as Greg points out [1], is to avoid versions at 7.1.0 and above, because 7.1.0 introduced a bug. So, specify 7.1.0 as an upper bound, in the range, while keeping the lower bound at the currently installed version. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/non-caret.20dependency.20ranges/near/1202944

The tilde is to avoid taking 2.4.0; there's a deprecation notice on NPM [1]: > CI messed this up... it's actually 3.x.x [1] https://www.npmjs.com/package/react-native-image-picker/v/2.4.0

And add a peer dependency that's now required.

Looks like this requires some formatting changes. Some of them look OK, some neutral [1]. In a few places where we'd really like to keep test data compact, add some `prettier-ignore`s. Changelog: https://github.com/prettier/prettier/blob/main/CHANGELOG.md [1] zulip#4789 (comment)

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 · 2021-06-11T00:13:39Z

Great, thanks for the reviews! Merged.

Notably, this includes an update to Sentry's SDK for React Native that came out after the last `yarn upgrade` commit, in zulip#4789, was authored. I got an error -- ``` [!] CocoaPods could not find compatible versions for pod "Sentry": In snapshot (Podfile.lock): Sentry (= 6.1.4) In Podfile: RNSentry (from `../node_modules/@sentry/react-native`) was resolved to 2.5.0, which depends on Sentry (= 7.0.0) You have either: * out-of-date source repos which you can update with `pod repo update` or with `pod install --repo-update`. * changed the constraints of dependency `Sentry` inside your development pod `RNSentry`. You should run `pod update Sentry` to apply changes you've made. ``` -- so I ran `pod update Sentry`, and it went through. After that, I ran `yarn` (with `pod install` getting run in the postinstall script), and no further changes were made. We should aim to have this out in a beta for a little while, to shake out any bugs. In addition to this Sentry upgrade, quite a lot of other stuff seems to have been touched.

chrisbobbe added the dependencies Pull requests that update a dependency file label Jun 8, 2021

chrisbobbe requested review from WesleyAC and gnprice June 8, 2021 14:53

chrisbobbe force-pushed the pr-yarn-upgrade branch from 32a9d31 to 2b6051c Compare June 8, 2021 16:10

WesleyAC reviewed Jun 8, 2021

View reviewed changes

gnprice reviewed Jun 9, 2021

View reviewed changes

chrisbobbe force-pushed the pr-yarn-upgrade branch from 2b6051c to 83b1093 Compare June 9, 2021 18:48

chrisbobbe added 12 commits June 10, 2021 20:04

deps: Upgrade react-native-image-picker; use tilde.

08f74e6

The tilde is to avoid taking 2.4.0; there's a deprecation notice on NPM [1]: > CI messed this up... it's actually 3.x.x [1] https://www.npmjs.com/package/react-native-image-picker/v/2.4.0

deps: Upgrade flow-typed.

9e64279

And add a peer dependency that's now required.

chrisbobbe force-pushed the pr-yarn-upgrade branch from 83b1093 to 78a7763 Compare June 11, 2021 00:13

chrisbobbe merged commit 78a7763 into zulip:master Jun 11, 2021

chrisbobbe mentioned this pull request Jun 11, 2021

deps: Run yarn upgrade. #4792

Merged

gnprice mentioned this pull request Jun 15, 2021

changelog: Document changes since v27.163. #4800

Merged

chrisbobbe deleted the pr-yarn-upgrade branch July 30, 2021 17:47

Uh oh!

Conversation

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

Uh oh!

Uh oh!

WesleyAC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 8, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Jun 8, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jun 9, 2021

Uh oh!

gnprice commented Jun 9, 2021

Uh oh!

WesleyAC commented Jun 10, 2021

Uh oh!

chrisbobbe commented Jun 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrisbobbe commented Jun 8, 2021 •

edited

Loading

chrisbobbe Jun 8, 2021 •

edited

Loading

chrisbobbe Jun 8, 2021 •

edited

Loading

gnprice left a comment •

edited

Loading