Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to RN v0.64 #4426

Closed
gnprice opened this issue Jan 22, 2021 · 10 comments · Fixed by #4991
Closed

Upgrade to RN v0.64 #4426

gnprice opened this issue Jan 22, 2021 · 10 comments · Fixed by #4991
Labels
P1 high-priority upstream: RN Issues related to an issue in React Native

Comments

@gnprice
Copy link
Member

gnprice commented Jan 22, 2021

The next round after #4245. This is the first time I'm opening one of these when we're already on the previous latest release, and before the new one is even out yet!

The RN community release managers are working on the next release now, and it's anticipated in the next few weeks. Details at react-native-community/releases#214 and particularly at react-native-community/releases#214 (comment) .

There's already a release candidate, so that would be a way to get an early check on any issues we'll run into.

@gnprice gnprice added the upstream: RN Issues related to an issue in React Native label Jan 22, 2021
@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jan 25, 2021

We'll be jumping to Flow 0.137.0, which means (among other things) that something called "types-first" has been both introduced and enabled by default. We can turn it off, though, by adding types_first=false under [options] in our Flow config.

The thing we'll have to do to enable it is annotate almost all the exports of our modules:

The caveat of this new version is that it requires exported parts of the code to be annotated with types, or to be expressions whose type can be trivially inferred (for example numbers and strings).

Not annotating an export sufficiently will raise a signature-verification-failure error.

It looks like we'll be able to work toward that incrementally by selecting certain directories where we want to check the exports of the contained modules.

They mention a codemod, but I suspect, if we use it at all, we'll want to use it on small sections of the code, one at a time, and check its work.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jan 25, 2021
When we start using Flow v0.137, along with React Native v0.64
(that's issue zulip#4426), we'll be asked to specify what rule(s) we're
suppressing with each suppression comment. Conveniently, the
warnings will tell us the applicable rule(s).

So, using those warnings, get a head start on doing so. There will
probably be more of these (we haven't yet started enforcing the
requirement now, so some unmarked `$FlowFixMe`s will probably
accumulate), but we might as well get this chunk out of the way.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jan 25, 2021
When we start using Flow v0.137+, along with React Native v0.64
(that's issue zulip#4426), we'll be asked to specify what rule(s) we're
suppressing with each suppression comment. Conveniently, the
warnings will tell us the applicable rule(s) we need to write down.

So, having peeked ahead at the warnings seen in the RN 0.64.0-rc.2
world, get a head start on doing so. There will probably be more of
these (we haven't yet started enforcing the requirement now, so some
unmarked `$FlowFixMe`s will naturally accumulate), but we might as
well get this chunk out of the way.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jan 25, 2021
When we start using Flow v0.137+, along with React Native v0.64
(that's issue zulip#4426), we'll be asked to specify what rule(s) we're
suppressing with each suppression comment. Conveniently, the
warnings will tell us the applicable rule(s) we need to write down.

So, having peeked ahead at the warnings seen in the RN 0.64.0-rc.2
world, get a head start on doing so. There will probably be more of
these (we haven't yet started enforcing the requirement now, so some
unmarked `$FlowFixMe`s will naturally accumulate), but we might as
well get this chunk out of the way.
@gnprice
Copy link
Member Author

gnprice commented Jan 26, 2021

We'll be jumping to Flow 0.137.0, which means (among other things) that something called "types-first" has been both introduced and enabled by default

Cool! The performance benefits of that won't be as big for us as they are for a giant codebase like Facebook's, but we should see some. I think the quality of the type errors may improve, too.

Re the codemod, we'll definitely want to check its work: making readable commits and reviewing them normally. I suspect we'll want to start by trying it on relatively self-contained swathes of code like src/api/ or even src/third/; maybe then reducers+selectors, the other files that don't know about React, and finally swathes of React components. Depending on how much manual work is needed (and how much effort is needed to review the output), we can scale up or down how much code we cover per commit.

And if the codemod is really bad at its job, we can always ignore it and do the changes manually. But I'm hopeful that it'll be good enough, automating enough boring stuff accurately enough, that it'll be worth using.


FTR here's the full list of changes we'll see -- up to this point in the changelog:
https://github.com/facebook/flow/blob/master/Changelog.md#01370
from 0.122.0, which we're currently on.

Other changes I see in a quick scan which we might appreciate, or might have to adapt for:

  • Allow indexers in exact object annotations

This should let us clean up a few types.

  • Standardized error suppression syntax, added ability to suppress errors based on error codes

The second part of this is #4433. The first means we'll have to drop $FlowMigrationFudge and replace it with one of the standard names.

  • Enable LSP support for autofix exports by default.
  • Enabled support for JSDoc in some LSP results.
  • Show JSDoc documentation in hover, autocomplete, and signature help.

I'm hopeful these will translate to some nice improvements in the VS Code editing experience!

  • Fixed Object.freeze to no longer incorrectly convert an inexact object into an exact object.

Hmm, I wonder if this will defeat the workaround we've used for a Flow bug involving {} as an "unsealed" object type.

@gnprice
Copy link
Member Author

gnprice commented Jan 26, 2021

We can turn it off, though, by adding types_first=false under [options] in our Flow config.

Oh also: this might be exactly the right thing to do for the RN upgrade. We'll want to do this if the types-first migration seems to be any substantial amount of work, I think.

But then we'll want to follow up by doing the types-first migration. A newer version of Flow (v0.143.0, out just this month) makes that the only available mode, so we'll presumably get that change in the next version of RN, v0.65.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jan 27, 2021
When we start using Flow v0.137+, along with React Native v0.64
(that's issue zulip#4426), we'll be asked to specify what rule(s) we're
suppressing with each suppression comment. Conveniently, the
warnings will tell us the applicable rule(s) we need to write down.

So, having peeked ahead at the warnings seen in the RN 0.64.0-rc.2
world, get a head start on doing so. There will probably be more of
these (we haven't yet started enforcing the requirement now, so some
unmarked `$FlowFixMe`s will naturally accumulate), but we might as
well get this chunk out of the way.
chrisbobbe added a commit that referenced this issue Jan 27, 2021
When we start using Flow v0.137+, along with React Native v0.64
(that's issue #4426), we'll be asked to specify what rule(s) we're
suppressing with each suppression comment. Conveniently, the
warnings will tell us the applicable rule(s) we need to write down.

So, having peeked ahead at the warnings seen in the RN 0.64.0-rc.2
world, get a head start on doing so. There will probably be more of
these (we haven't yet started enforcing the requirement now, so some
unmarked `$FlowFixMe`s will naturally accumulate), but we might as
well get this chunk out of the way.
Gautam-Arora24 pushed a commit to Gautam-Arora24/zulip-mobile that referenced this issue Feb 2, 2021
When we start using Flow v0.137+, along with React Native v0.64
(that's issue zulip#4426), we'll be asked to specify what rule(s) we're
suppressing with each suppression comment. Conveniently, the
warnings will tell us the applicable rule(s) we need to write down.

So, having peeked ahead at the warnings seen in the RN 0.64.0-rc.2
world, get a head start on doing so. There will probably be more of
these (we haven't yet started enforcing the requirement now, so some
unmarked `$FlowFixMe`s will naturally accumulate), but we might as
well get this chunk out of the way.
@chrisbobbe
Copy link
Contributor

And, v0.64 was released yesterday! Here's the blog post: https://reactnative.dev/blog/2021/03/11/version-0.64

@gnprice
Copy link
Member Author

gnprice commented Jun 22, 2021

One small followup to do after this upgrade (or the relevant part of it): the upgraded Metro should fix the issue that #4819 worked around. So then we should revert the workaround.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
To get a version that has the React Native peer dep range bumped to
include React Native v0.64, which we hope to upgrade to soon
(zulip#4426).

There is one announced breaking change for Android; the
`setSupportMultipleWindows` prop is introduced, defaulting to
`true` [1]. This is to "mitigate the security advisory
CVE-2020-6506". The advisory says, "This vulnerability affects React
Native apps which use a react-native-webview that allows navigation
to arbitrary URLs, and when that app runs on systems with an Android
WebView version prior to 83.0.4103.106."

I'm skeptical that we were affected, because I don't think we allow
navigation to arbitrary URLs; see our comments on our use of the
`originWhitelist` and `onShouldStartLoadWithRequest`. But good that
they're addressing reported vulnerabilities.

[1] https://github.com/react-native-webview/react-native-webview/releases/tag/v11.0.0
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 1, 2021
To get a version that has the React Native peer dep range bumped to
include React Native v0.64, which we hope to upgrade to soon
(zulip#4426).

There is one announced breaking change for Android; the
`setSupportMultipleWindows` prop is introduced, defaulting to
`true` [1]. This is to "mitigate the security advisory
CVE-2020-6506". The advisory says, "This vulnerability affects React
Native apps which use a react-native-webview that allows navigation
to arbitrary URLs, and when that app runs on systems with an Android
WebView version prior to 83.0.4103.106."

I'm skeptical that we were affected, because I don't think we allow
navigation to arbitrary URLs; see our comments on our use of the
`originWhitelist` and `onShouldStartLoadWithRequest` props. But good
that they're addressing reported vulnerabilities.

[1] https://github.com/react-native-webview/react-native-webview/releases/tag/v11.0.0
@chrisbobbe
Copy link
Contributor

Just opened #4843 to get to a version of react-native-webview whose peer-dep range for React Native include v0.64.

I didn't notice any other current third-party dependencies we'll have to do this for! 🎉

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 2, 2021
To get a version that has the React Native peer dep range bumped to
include React Native v0.64, which we hope to upgrade to soon
(zulip#4426).

There is one announced breaking change for Android; the
`setSupportMultipleWindows` prop is introduced, defaulting to
`true` [1]. This is to "mitigate the security advisory
CVE-2020-6506". The advisory says, "This vulnerability affects React
Native apps which use a react-native-webview that allows navigation
to arbitrary URLs, and when that app runs on systems with an Android
WebView version prior to 83.0.4103.106."

I'm skeptical that we were affected, because I don't think we allow
navigation to arbitrary URLs; see our comments on our use of the
`originWhitelist` and `onShouldStartLoadWithRequest` props. But good
that they're addressing reported vulnerabilities.

[1] https://github.com/react-native-webview/react-native-webview/releases/tag/v11.0.0
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 3, 2021
To get a version that has the React Native peer dep range bumped to
include React Native v0.64, which we hope to upgrade to soon
(zulip#4426).

There is one announced breaking change for Android; the
`setSupportMultipleWindows` prop is introduced, defaulting to
`true` [1]. This is to "mitigate the security advisory
CVE-2020-6506". The advisory says, "This vulnerability affects React
Native apps which use a react-native-webview that allows navigation
to arbitrary URLs, and when that app runs on systems with an Android
WebView version prior to 83.0.4103.106."

I'm skeptical that we were affected, because I don't think we allow
navigation to arbitrary URLs; see our comments on our use of the
`originWhitelist` and `onShouldStartLoadWithRequest` props. But good
that they're addressing reported vulnerabilities.

[1] https://github.com/react-native-webview/react-native-webview/releases/tag/v11.0.0
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 1, 2021
Just while we wait for the code in this package to be error-free
when exact_by_default is enabled [1]. We plan to enable it for the
RN v0.64 upgrade (zulip#4426). The PR is
  react-native-cameraroll/react-native-cameraroll#328.

It's very possible that some of those object types would be better
as exact instead of inexact. But:

1. Just getting the library to pass the implicit-inexact-object lint
   is enough to unblock switching on exact_by_default (for us and
   for other projects that want to do that too).

2. I guess we could go through and decide what should stay inexact
   and what should be made exact. But then each of those choices
   would have to be approved, and some of them would probably be
   breaking changes that we'd need to write changelog entries for.
   (Not ones that would affect an app at runtime -- but still, apps
   that use Flow would predictably have to change their code to
   adapt.)

[1] https://medium.com/flow-type/how-to-upgrade-to-exact-by-default-object-type-syntax-7aa44b4d08ab
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 1, 2021
Just while we wait for the code in this package to be error-free
when exact_by_default is enabled [1]. We plan to enable it for the
RN v0.64 upgrade (zulip#4426). The PR is
  react-native-cameraroll/react-native-cameraroll#328.

It's very possible that some of those object types would be better
as exact instead of inexact. But:

1. Just getting the library to pass the implicit-inexact-object lint
   is enough to unblock switching on exact_by_default (for us and
   for other projects that want to do that too).

2. I guess we could go through and decide what should stay inexact
   and what should be made exact. But then each of those choices
   would have to be approved, and some of them would probably be
   breaking changes that we'd need to write changelog entries for.
   (Not ones that would affect an app at runtime -- but still, apps
   that use Flow would predictably have to change their code to
   adapt.)

So instead, the PR just makes all the implicit inexact object types
explicitly inexact.  It's based directly on v4.0.4, the version we've
been using, so it doesn't pull in any other changes.

[1] https://medium.com/flow-type/how-to-upgrade-to-exact-by-default-object-type-syntax-7aa44b4d08ab
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 3, 2021
From Flow's perspective, we're fine to delay types-first until we're
on v0.143, which we'd normally take along with the RN v0.65 upgrade.

But, from experimentation, React Native v0.64's internal code seems
to have been written with Types-First, such that a few Flow errors
would show up if we tried to use RN v0.64 without Types-First
enabled. So, enable it, now that we can, and now that the RN v0.64
upgrade is on the horizon (zulip#4426).

Fixes: zulip#4907
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 3, 2021
From Flow's perspective, we're fine to delay types-first until we're
on v0.143, which we'd normally take along with the RN v0.65 upgrade.

But, from experimentation, React Native v0.64's internal code seems
to have been written with Types-First, such that a few Flow errors
would show up if we tried to use RN v0.64 without Types-First
enabled. So, enable it, now that we can, and now that the RN v0.64
upgrade is on the horizon (zulip#4426).

Fixes: zulip#4907
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 8, 2021
…v0.64.

This would be handled already if our transformIgnorePatterns
extended the transformIgnorePatterns in React Native's Jest preset,
because that preset was adapted in facebook/react-native@a77f2c40d,
released in RN v0.64.

It would also be handled already if our transformIgnorePatterns
extended the transformIgnorePatterns in `jest-expo`'s Jest preset
after *that* preset was adapted, in the (as-yet-unreleased) change
in expo/expo@24bce422c.

But we ignore and clobber transformIgnorePatterns in those presets,
so we add this line ourselves.

If we upgrade RN before taking that `jest-expo` change, we'll see
the following harmless warning when we run Jest:

  react-native/jest-preset contained different
    transformIgnorePatterns than expected.

It's harmless because we don't use `jest-expo`'s
transformIgnorePatterns anyway; we fully specify our own instead.

Related: zulip#4426
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 8, 2021
In this commit:

- Update `react-native`, `react`, and `flow-bin` in package.json,
  and bump the Flow version in the Flow config. All of this is
  normal.

- Add a `resolutions` line for `react-test-renderer`, while we wait
  for `jest-expo` to officially target RN v0.64. I'd opened the
  relevant change in expo/expo#13549, but it landed first in
  expo/expo@d49e7070a. As of 2021-09-03, that change hasn't been
  released, but it might be soon.

  This line raises the following warning from Yarn:

    warning Resolution field "react-test-renderer@17.0.1" is
      incompatible with requested version
      "react-test-renderer@~16.11.0"

  This is expected. The doc says [1], "You will receive a warning if
  your resolution version or range is not compatible with the
  original version range." We've managed to get away with
  incompatible `resolutions` lines in the past; I guess that's a
  Yarn bug that we're not running into this time.

- Enable `exact_by_default`, to follow
  facebook/react-native@050a7dd01. (We'd done a lot of work to
  prepare for this.) We couldn't enable it earlier because RN v0.63
  didn't pass type-checking with it on.

- Do two find-and-replace-all tasks, for some places where we're
  importing from React Native's internals and their paths have
  changed:

  'react-native/Libraries/Animated/src/nodes/AnimatedValue' ->
    'react-native/Libraries/Animated/nodes/AnimatedValue', from
    facebook/react-native@9c9e67791.

  'react-native/Libraries/StyleSheet/StyleSheetTypes' ->
    'react-native/Libraries/StyleSheet/StyleSheet' from
    facebook/react-native@0a6713312. And, for this one, they give
    the reason as "to encourage its use in product code"! 🎉

- And that's it!

[1] https://classic.yarnpkg.com/en/docs/selective-version-resolutions/#toc-tips-tricks

Fixes: zulip#4426
@gnprice
Copy link
Member Author

gnprice commented Sep 8, 2021

Since we're about to merge #4991 to complete this upgrade 🎉 , I just scanned this thread to look for follow-ups. It looks like there's just one:

One small followup to do after this upgrade (or the relevant part of it): the upgraded Metro should fix the issue that #4819 worked around. So then we should revert the workaround.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Sep 8, 2021
…v0.64.

This would be handled already if our transformIgnorePatterns
extended the transformIgnorePatterns in React Native's Jest preset,
because that preset was adapted in facebook/react-native@a77f2c40d,
released in RN v0.64.

It would also be handled already if our transformIgnorePatterns
extended the transformIgnorePatterns in `jest-expo`'s Jest preset
after *that* preset was adapted, in the (as-yet-unreleased) change
in expo/expo@24bce422c.

But we ignore and clobber transformIgnorePatterns in those presets,
because the API just isn't suitable to taking a list from elsewhere
and extending it [1]. So we add this line ourselves.

Since we're upgrading RN before taking that `jest-expo` change,
we'll see the following harmless warning for a while when we run
Jest:

  react-native/jest-preset contained different
    transformIgnorePatterns than expected.

It's harmless because we don't use `jest-expo`'s
transformIgnorePatterns anyway; we fully specify our own instead.

[1] zulip#4991 (comment)

Related: zulip#4426
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've been frustrated
for some time about the quality of KeyboardAvoidingView. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

We haven't yet successfully forked react-native; my current
understanding is that that'd be kind of a project, but I could be
wrong.

None of my PRs can be accepted until RN starts checking its
CLA-related mail; I should be covered by the corporate CLA signed by
Kandra.

RN upgrades take a while for us to do; see our most recent, to RN
v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

We haven't yet successfully forked react-native; my current
understanding is that that'd be kind of a project, but I could be
wrong.

None of my PRs to React Native can be accepted until RN starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

We haven't yet successfully forked react-native; my current
understanding is that that'd be kind of a project, but I could be
wrong.

None of my PRs to React Native can be accepted until RN starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Tested on my iPhone 13 Pro running iOS 15, with and without the
"Prefer Cross-Fade Transitions" setting (for the sake of zulip#5162).

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

My current understanding is that the initial setup for maintaining
our own react-native fork would be kind of a big project, but I
could be wrong.

None of my PRs to React Native can be accepted until RN starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Tested on my iPhone 13 Pro running iOS 15, with and without the
"Prefer Cross-Fade Transitions" setting (for the sake of zulip#5162).

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView
  including these two, which raise the same issue as zulip#5162 and
  haven't been resolved:
    facebook/react-native#29974
    facebook/react-native#31484
  (patch-package isn't a resolution)

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

My current understanding is that the initial setup for maintaining
our own react-native fork would be kind of a big project, but I
could be wrong.

None of my PRs to React Native can be accepted until RN starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Tested on my iPhone 13 Pro running iOS 15, with and without the
"Prefer Cross-Fade Transitions" setting (for the sake of zulip#5162).

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView
  including these two, which raise the same issue as zulip#5162 and
  haven't been resolved:
    facebook/react-native#29974
    facebook/react-native#31484
  (patch-package isn't a resolution)

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

My current understanding is that the initial setup for maintaining
our own react-native fork would be kind of a big project, but I
could be wrong.

None of my PRs to React Native can be accepted until Facebook starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Tested on my iPhone 13 Pro running iOS 15, with and without the
"Prefer Cross-Fade Transitions" setting (for the sake of zulip#5162).

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView
  including these two, which raise the same issue as zulip#5162 and
  haven't been resolved:
    facebook/react-native#29974
    facebook/react-native#31484
  (patch-package isn't a resolution)

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

My current understanding is that the initial setup for maintaining
our own react-native fork would be kind of a big project, but I
could be wrong.

None of my PRs to React Native can be accepted until Facebook starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Tested on my iPhone 13 Pro running iOS 15, with and without the
"Prefer Cross-Fade Transitions" setting (for the sake of zulip#5162).

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView
  including these two, which raise the same issue as zulip#5162 and
  haven't been resolved:
    facebook/react-native#29974
    facebook/react-native#31484
  (patch-package isn't a resolution)

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

My current understanding is that the initial setup for maintaining
our own react-native fork would be kind of a big project, but I
could be wrong.

None of my PRs to React Native can be accepted until Facebook starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Tested on my iPhone 13 Pro running iOS 15, with and without the
"Prefer Cross-Fade Transitions" setting (for the sake of zulip#5162).

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 9, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView
  including these two, which raise the same issue as zulip#5162 and
  haven't been resolved:
    facebook/react-native#29974
    facebook/react-native#31484
  (patch-package isn't a resolution)

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

My current understanding is that the initial setup for maintaining
our own react-native fork would be kind of a big project, but I
could be wrong.

None of my PRs to React Native can be accepted until Facebook starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Tested on my iPhone 13 Pro running iOS 15, with and without the
"Prefer Cross-Fade Transitions" setting (for the sake of zulip#5162).

Fixes: zulip#5162
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Dec 10, 2021
…ngView

The immediate catalyst for this is zulip#5162, but I've had earlier
encounters with React Native's KeyboardAvoidingView, and I don't
love it. See
- our 70eca07; that took a lot of painstaking investigation that
  would have been unnecessary if `keyboardVerticalOffset` had had a
  clear design from the beginning
- the implementation, at
    https://github.com/facebook/react-native/blob/v0.64.2/Libraries/Components/Keyboard/KeyboardAvoidingView.js
- the long list of open issues in RN that mention it:
    https://github.com/facebook/react-native/issues?q=is%3Aissue+is%3Aopen+KeyboardAvoidingView
  including these two, which raise the same issue as zulip#5162 and
  haven't been resolved:
    facebook/react-native#29974
    facebook/react-native#31484
  (patch-package isn't a resolution)

I'm not optimistic that we can get zulip#5162 fixed in bounded time
unless we abandon KeyboardAvoidingView and implement the necessary
logic ourselves. Fortunately, that seems pretty doable.

I don't yet see a quick fix in KeyboardAvoidingView that doesn't
risk making its code even more brittle and hard to reason about. My
threshold for that is pretty low in this case: with the code in its
current state, each additional piece of logic (even sound logic)
comes with an unusually large cost of understanding/maintaining it.

My current understanding is that the initial setup for maintaining
our own react-native fork would be kind of a big project, but I
could be wrong.

None of my PRs to React Native can be accepted until Facebook starts
checking its CLA-related mail; I should be covered by the corporate
CLA signed by Kandra. See, e.g.,
  facebook/react-native#29926 (comment) .

RN upgrades tend to take a while for us to do; see our most recent,
to RN v0.64: zulip#4426. RN v0.67 is already doing release candidates.

Tested on my iPhone 13 Pro running iOS 15, with and without the
"Prefer Cross-Fade Transitions" setting (for the sake of zulip#5162).

Fixes: zulip#5162
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 high-priority upstream: RN Issues related to an issue in React Native
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants