RN v0.61 -> v0.62 upgrade #4247

chrisbobbe · 2020-09-02T18:11:07Z

~~To follow ~~#4244 and~~ the react-navigation v4 upgrade (#4248); see my note at #4248 (comment).~~

~~Currently a draft, since it's quite incomplete. But I'm sending this now so I can conveniently point to things in it.~~

Ready for review! 🙂

Fixes: #3782

gnprice

Thanks @chrisbobbe ! This looks great. Small comments below.

src/api/transport.js

src/api/apiFetch.js

src/reactions/MessageReactionList.js

gnprice · 2020-09-18T20:35:55Z

.flowconfig

+# Temporarily disabled to smooth the RN upgrade.
+# include_warnings=true


I think you want this a few commits earlier in the branch, before "flagsReducer: Add a $FlowFixMe for something Flow v0.113 will catch."

How about moving

"flagsReducer: Add a $FlowFixMe for something Flow v0.113 will catch."

later in the branch? I think this is the only one that's strayed outside the unused-fixmes-allowed window.

Oh, hang on—actually I think there's a subtlety that I didn't describe too well. If the Flow upgrade proceeded without the changes in

"flagsReducer: Add a $FlowFixMe for something Flow v0.113 will catch."

, there would be an error at a different site than where that $FlowFixMe is:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/chat/flagsReducer.js:48:10 Cannot return object literal because inexact object literal [1] is incompatible with exact FlagsState [2]. [2] 33│ ): FlagsState => { : 45│ }); 46│ }); 47│ [1] 48│ return { 49│ ...state, 50│ ...newState, 51│ }; 52│ }; 53│ 54│ const removeFlagForMessages = (state: FlagsState, messages: number[], flag: string): FlagsState => {

The way I suppress that error is to pretend that {} for newState is a FlagsState, which is a lie, and that lie gets caught even before v0.113. The particular $FlowFixMe that I added makes Flow ignore that lie, so it isn't a case of a temporarily-unnecessary suppression that would become necessary (all else being equal) across the upgrade.

Mmm, I see.

Yeah, that strategy makes sense. In describing it, I'd just emphasize that you're casting these values, lying about their types. The change isn't primarily about adding a fixme (as it might be for covering an existing lie / type error that Flow is about to start spotting) but rather adding a new lie, with a fixme to cover it up.

src/common/Input.js

chrisbobbe · 2020-09-18T22:19:27Z

Thanks for the review! I just have one query, at #4247 (comment). (Basically: should I change my strategy, or just describe it better?)

…te`. And tell Flow to ignore the lie. Obviously, we want to address this; I think zulip#4252 (using Immutable for this bit of state) is a good place to start. See discussion [1] for how this lie helps us in the short term. [1] zulip#4247 (comment)

chrisbobbe · 2020-09-18T22:46:06Z

Thanks for the review! I just pushed my latest changes.

In the upcoming Flow v0.113 upgrade, Flow will start taking issue with the spreading of `data` after other properties it might end up overriding, making the types of those properties ambiguous. So, spread `data` first, and use something other than spreading to say that we want `code` to default to `'BAD_REQUEST'`. [1] Also, make and use local `const` bindings for those properties so that guard conditions on their values consistently affect their types. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Flow.20v0.2E113.20upgrade/near/1010610

It's not a mystery which auth headers we might want to get from this function; there's no reason we have to use `{ [string]: string }` here. We just have to do something a bit silly in the fix, like we did in d141be7, and make a note of the corresponding Flow issue in a comment.

The `{}` expression in `= {}` was never being evaluated or used, since the argument was always being passed. If it weren't passed, somewhere, there would be a Flow error because the parameter is not optional.

With facebook/react-native#29866, `fetch` and a few of the major things it uses are totally untyped. But react-native doesn't suppress `RequestOptions` [1], so we can still use that to help make sure we're passing around the right kinds of stuff that will eventually be fed to `fetch`. Also, it looks like the functionality of `getFetchParams` would break if a caller included `headers`, so make the type not allow that. A type-cast and a $FlowFixMe on it at the top of `getFetchParams` can fall away naturally. [1] See it at https://www.saltycrane.com/cheat-sheets/flow-type/latest/

…te`. And tell Flow to ignore the lie. Obviously, we want to address this; I think zulip#4252 (using Immutable for this bit of state) is a good place to start. See discussion [1] for how this lie helps us in the short term. [1] zulip#4247 (comment)

I should have done this when I first started using a params object here, in d63d931.

This reminds me a lot of d141be7, except that it hasn't surfaced for us until the RN v0.61 -> v0.62 upgrade, which comes with upgrading Flow to v0.113.

Modern versions of react-navigation have `style` typed better than `{ ... }`, so we might as well too. It's odd that we have to add some type-casts; the values at those properties that may be clobbered by `...style` are subtypes of the corresponding properties on `ViewStyle`. Anyway, we'll be able to remove them following the RN v0.61 -> v0.62 upgrade, when we get Flow v0.113. This also lets us remove a flowlint suppression; not sure why.

In particular, this gets us a fix for @expo/react-native-action-sheetzulip#161, which landed in @expo/react-native-action-sheetb20bf029e, released in v3.6.0. On RN v0.62, which we're about to upgrade to, we'd otherwise see crashes on Android.

This lets us insert fixmes in a separate commit from the upgrade that makes them necessary. Like we did in c542823 and a68c15b.

The Flow bug in question is facebook/flow#8178; it's reportedly fixed in Flow v0.115; we'll get v0.122 with RN v0.63 (zulip#4245).

…rade.

Part of the RN v0.61 -> v0.62 changes to the template app [1], corresponding to the insertion in the Flow config in facebook/react-native@e54ecf907. After the main upgrade commit, we'll finish following facebook/react-native@e54ecf907 by making its two deletions in the Flow config. The deleted lines point to the old, then-incorrect location of the file. [1] https://react-native-community.github.io/upgrade-helper/?from=0.61.5&to=0.62.2

In this commit: - Upgrade `react-native`, `react`, and `flow-bin` following the template. - Set the Flow version in `.flowconfig` to match the version of `flow-bin` we're now using. - Rename JSCallInvoker{,Holder} to CallInvoker{,Holder} (facebook/react-native@4c998fd05). This must be done in the main upgrade commit because the dependency lives in node_modules/react-native, and we need to follow its name change in there. - Upgrade `react-native-webview` from ~10.0.0 to ^10.1.0. At 10.1.0, RN v0.62 is newly supported and earlier RN versions are no longer supported; this is expressed as a change in the `react` peer dependency's version range. See discussion [1] and an issue that flagged this as a semver violation [2]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/RN.20v0.2E61.3A.20react-native-webview/near/901930 [2] react-native-webview/react-native-webview#1445

Part of the RN v0.61 -> v0.62 changes to the template app [1], corresponding to the two deletions in the Flow config in facebook/react-native@e54ecf907. We pointed to the file's new location in a recent commit before the main upgrade commit; now, we finish following facebook/react-native@e54ecf907 by removing the references to the old, now-incorrect location of the file. [1] https://react-native-community.github.io/upgrade-helper/?from=0.61.5&to=0.62.2

We add `typeof` in several places to address this Flow error that starts appearing at the RN and Flow upgrade: ``` Cannot use `TextInput` as a type. A name can be used as a type only if it refers to a type definition, an interface definition, or a class definition. To get the type of a non-class value, use `typeof`. ``` I'm not sure if this is due to the Flow upgrade or to changes React Native made to `TextInput`, or both. Several changes to `TextInput` are announced in the RN changelog [1], including three that are breaking, but I haven't been able to identify any in particular that would start giving us that error. With that dealt with, we also get this new error on calling various methods on the instance stored at a `TextInput` ref (e.g., `textInputRef.current.focus()`): ``` Cannot call textInputRef.current.focus because: • Either property focus is missing in AbstractComponent [1]. • Or property focus is missing in object type [2]. ``` At first, I thought something was wrong with how we're annotating the variable storing the ref, or that Flow didn't fully understand the `React.createRef` API (we started using that in a recent commit before the main upgrade commit). But rather, it seems to be an issue that's known to occur at RN v0.61.1, and which didn't occur on `master` as of 2020-04-06 [2]. Checking commits around that date in `react-native`, I'm pretty sure we'll have a fix in RN v0.63 (zulip#4245). I posted at the RN v0.63 upgrade issue (zulip#4245) with these observations [3]. [1] https://github.com/react-native-community/releases/blob/master/CHANGELOG.md#0620 [2] See point 2 at facebook/react-native#28459 (comment). [3] zulip#4245 (comment)

This also lets us re-enable warnings on unused fixmes.

Lots of things changed with how spreads are handled in the recent Flow v0.105 -> v0.113 upgrade, but I'm not sure what particular change means we no longer need these. Anyway, the most natural thing is to not have the type-casts, so, remove them.

Part of the RN v0.61 -> v0.62 changes to the template app [1], corresponding to facebook/react-native@a0d874087. This must happen at or after the main upgrade commit because the field is newly optional at the new version. There isn't a clear functional advantage to doing it this way. In fact, the maintainer who merged it said he was "not a big fan" of the change [2]. But he merged it anyway, on the grounds that it makes the experience more consistent with iOS. So, I suppose it does that for us, too, and in any case we're now more consistent with the template app. [1] https://react-native-community.github.io/upgrade-helper/?from=0.61.5&to=0.62.2 [2] facebook/react-native#26769 (review)

Call the appropriate initialize-Flipper function in native runtime code. Part of the RN v0.61 -> v0.62 changes to the template app [1], corresponding to facebook/react-native@05f5cb534. This should be fine to do after the main upgrade commit; nothing in React Native's internals should depend on Flipper being enabled [2]. On iOS, use the form of the initialize-Flipper function as it was updated in facebook/react-native@b4d1fcfb2 and wrap the call site in a conditional as done there. [1] https://react-native-community.github.io/upgrade-helper/?from=0.61.5&to=0.62.2 [2] zulip#4244 (comment)

gnprice · 2020-09-18T23:30:35Z

Looks good! Merged.

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 Sep 2, 2020

chrisbobbe mentioned this pull request Sep 2, 2020

Pre-RN v0.61 -> v0.62 upgrade changes #4244

Merged

chrisbobbe force-pushed the rn-62 branch 2 times, most recently from 24bec42 to 8fc1401 Compare September 4, 2020 22:45

chrisbobbe force-pushed the rn-62 branch from 8fc1401 to f8a1d20 Compare September 9, 2020 23:25

chrisbobbe marked this pull request as ready for review September 10, 2020 00:16

chrisbobbe force-pushed the rn-62 branch from f8a1d20 to 3e6f33b Compare September 11, 2020 22:10

chrisbobbe requested a review from gnprice September 15, 2020 18:07

chrisbobbe force-pushed the rn-62 branch from 3e6f33b to d9f9f18 Compare September 17, 2020 01:30

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

chrisbobbe force-pushed the rn-62 branch from d9f9f18 to 76563f5 Compare September 17, 2020 21:50

chrisbobbe mentioned this pull request Sep 18, 2020

RN v0.61 -> v0.62 upgrade followup #4261

Merged

gnprice reviewed Sep 18, 2020

View reviewed changes

chrisbobbe force-pushed the rn-62 branch from 76563f5 to d8412bb Compare September 18, 2020 22:43

chrisbobbe added 12 commits September 18, 2020 16:28

apiFetch [nfc]: Remove default value for non-optional parameter.

6c94626

The `{}` expression in `= {}` was never being evaluated or used, since the argument was always being passed. If it weren't passed, somewhere, there would be a Flow error because the parameter is not optional.

fetchActions types: Make fetchArgs in fetchMessages exact.

00ace9f

I should have done this when I first started using a params object here, in d63d931.

MessageReactionList types: Respond to possible Flow bug.

38f2238

This reminds me a lot of d141be7, except that it hasn't surfaced for us until the RN v0.61 -> v0.62 upgrade, which comes with upgrading Flow to v0.113.

flow: Allow unused fixmes, for now.

43fb127

This lets us insert fixmes in a separate commit from the upgrade that makes them necessary. Like we did in c542823 and a68c15b.

message, presence types: Add some fixmes.

e020dec

The Flow bug in question is facebook/flow#8178; it's reportedly fixed in Flow v0.115; we'll get v0.122 with RN v0.63 (zulip#4245).

flow: Add temp fixmes for TextInput that we'll remove after the upg…

b491a17

…rade.

chrisbobbe added 8 commits September 18, 2020 16:28

flow: Remove some suppressions, as allowed by Flow v0.113.

78a25c9

This also lets us re-enable warnings on unused fixmes.

gnprice force-pushed the rn-62 branch from d8412bb to 7c72715 Compare September 18, 2020 23:30

gnprice merged commit 7c72715 into zulip:master Sep 18, 2020

chrisbobbe deleted the rn-62 branch November 5, 2021 00:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RN v0.61 -> v0.62 upgrade #4247

RN v0.61 -> v0.62 upgrade #4247

chrisbobbe commented Sep 2, 2020 •

edited

gnprice left a comment

gnprice Sep 18, 2020

chrisbobbe Sep 18, 2020 •

edited

chrisbobbe Sep 18, 2020 •

edited

gnprice Sep 18, 2020 •

edited

chrisbobbe commented Sep 18, 2020 •

edited

chrisbobbe commented Sep 18, 2020

gnprice commented Sep 18, 2020

		# Temporarily disabled to smooth the RN upgrade.
		# include_warnings=true

RN v0.61 -> v0.62 upgrade #4247

RN v0.61 -> v0.62 upgrade #4247

Conversation

chrisbobbe commented Sep 2, 2020 • edited

gnprice left a comment

Choose a reason for hiding this comment

gnprice Sep 18, 2020

Choose a reason for hiding this comment

chrisbobbe Sep 18, 2020 • edited

Choose a reason for hiding this comment

chrisbobbe Sep 18, 2020 • edited

Choose a reason for hiding this comment

gnprice Sep 18, 2020 • edited

Choose a reason for hiding this comment

chrisbobbe commented Sep 18, 2020 • edited

chrisbobbe commented Sep 18, 2020

gnprice commented Sep 18, 2020

chrisbobbe commented Sep 2, 2020 •

edited

chrisbobbe Sep 18, 2020 •

edited

chrisbobbe Sep 18, 2020 •

edited

gnprice Sep 18, 2020 •

edited

chrisbobbe commented Sep 18, 2020 •

edited