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

deps: Upgrade @react-navigation/{bottom,material-top}-tabs to 6.x #5857

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chrisbobbe
Copy link
Contributor

@chrisbobbe chrisbobbe commented Apr 15, 2024

The @react-navigation/material-top-tabs upgrade is done to get us off of react-native-reanimated, which should help us with

Then the @react-navigation/bottom-tabs upgrade was done because I was optimistic about swiftly finishing

. But I didn't manage to finish that off; it turned out to be more work than I have time for right now (in particular, upgrading @react-navigation/native and @react-navigation/stack). But the bottom-tabs upgrade, included here, should be ready to go.

Screenshots coming, to demonstrate that the appearance doesn't change except in one small way where we were forced to make an adjustment:

share-to: Move ScrollView inward, to ShareTo{Stream,Pm} tabs

Related: #5847
Fixes-partly: #4936

This version should correspond better with RN 68, which is what
we're on.
When we upgrade @react-navigation/material-top-tabs soon, it will no
longer support putting tabs inside a ScrollView:
  react-navigation/react-navigation#11067 (comment)

So, wrap the content of each tab, instead of wrapping the whole tab
navigator, taking care to copy the ScrollView's effective
configuration.
This unfortunately causes a peer-dep warning:

  warning " > @react-navigation/material-top-tabs@6.6.13" has
    incorrect peer dependency "@react-navigation/native@^6.0.0".

But no issues have yet appeared in manual testing, and that's
consistent with the note on the React Navigation upgrade guide that
says, "To make upgrading easier, it is possible to mix packages from
the `6.x.x` and `5.x.x` version ranges.":
  https://reactnavigation.org/docs/upgrading-from-5.x#note-on-mixing-react-navigation-5-and-react-navigation-6-packages

Changelog:
  https://github.com/react-navigation/react-navigation/blob/main/packages/material-top-tabs/CHANGELOG.md

Done by reading and following the relevant parts of the React Nav
upgrade guide, including general information at the top and also the
section specific to Material Top Tabs:
  https://reactnavigation.org/docs/upgrading-from-5.x/#material-top-tab-navigator

Done now because it lets us get rid of react-native-reanimated, as
foreshadowed in our React Nav 6 upgrade issue:
  zulip#4936 (comment)

That's helpful because the old version of react-native-reanimated
that we're on uses a certain iOS API that Apple identifies as
privacy-sensitive, triggering a "Privacy Manifest" requirement:
  software-mansion/react-native-reanimated#5819
For zulip#5847, we're working on reducing and handling such requirements.

That API usage is removed in v2.9.0-rc.0 of Reanimated (see
just-linked PR), but I didn't pursue upgrading it because that path
seems to require abandoning remote JS debugging, at least according
to a note from 2022. For details on that, search for
"react-native-reanimated" here:
  zulip#5441

Related: zulip#5847
Fixes-partly: zulip#4936
This resolves a helpful performance warning logged to the console
while the app runs:

('upside_down' is the name of the upside-down smiley face emoji;
it's one of the tabs on the reactions screen for a particular
message.)

  Looks like you're passing an inline function for 'component' prop
  for the screen 'upside_down' (e.g.
  component={() => <SomeComponent />}).
  Passing an inline function will cause the component state to be
  lost on re-render and cause perf issues since it's re-created
  every render. You can pass the function as children to 'Screen'
  instead to achieve the desired behaviour.
…m tabs

This was quietly dropped in v5, it seems:
  https://reactnavigation.org/docs/4.x/bottom-tab-navigator/
  https://reactnavigation.org/docs/5.x/bottom-tab-navigator/

The icons are still shown in manual testing, thankfully.

While we're at it, make BottomTabBarOptions exact in types/ using a
TsFlower patch.
This unfortunately adds two more peer-dep warnings to the one that
appeared when we upgraded @react-navigation/material-top-tabs,
earlier in this series. Here are all three together:

  warning " > @react-navigation/bottom-tabs@6.5.20" has incorrect peer
    dependency "@react-navigation/native@^6.0.0".
  warning "@react-navigation/bottom-tabs >
    @react-navigation/elements@1.3.30" has incorrect peer dependency
    "@react-navigation/native@^6.0.0".
  warning " > @react-navigation/material-top-tabs@6.6.13" has
    incorrect peer dependency "@react-navigation/native@^6.0.0".

But again, no issues have appeared in manual testing; see note in
that earlier commit.

Changelog:
  https://github.com/react-navigation/react-navigation/blob/main/packages/bottom-tabs/CHANGELOG.md

For why it's OK to abandon our own custom version of this
dependency, see the commit where we started using it:

c0b17bd
>  So this fix will become unnecessary when we're on React
>  Navigation 6

Done by reading and following the relevant parts of the React Nav
upgrade guide, including general information at the top and also the
section specific to Bottom Tabs:
  https://reactnavigation.org/docs/upgrading-from-5.x#bottom-tab-navigator

Done with the hope that it wouldn't take too much effort to get to
the finish line of the React Nav v6 upgrade, zulip#4936. But when I tried
upgrading @react-navigation/stack and @react-navigation/native, I
had a lot of trouble working out the right Flow types and TsFlower
patches, and this is a legacy codebase. But this part was doable.

Fixes-partly: zulip#4936
@chrisbobbe chrisbobbe added the dependencies Pull requests that update a dependency file label Apr 15, 2024
@chrisbobbe
Copy link
Contributor Author

I took screenshots on my iPhone yesterday, and I forgot which ones were "before" and which ones were "after" shots. I'll include both sets anyway and call them "A" and "B". If we spot important differences in how the UI looks between A and B, I can find out which is "before" and "after" by building the app again. But I think I can save quite a bit of time by just posting the ones I have now.

Bottom tabs:

A B
4E983C46-B56C-43D4-AC44-4665B6D6D078 D6F9F6B0-2F09-4FF4-A222-51F04508D459

Reactions:

A B
8F0F4135-AB43-4DDD-86BA-910A50035D8A 12BAE32B-B144-49CF-941E-DFF839C68AA0

Android screenshots coming next.

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Apr 16, 2024

Android screenshots:

Before After
image image
image image
Apr-15-2024 18-00-33 Apr-15-2024 17-54-35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant