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.

Sign up for GitHub

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

Jump to bottom

Upgrade to React Navigation v6. #4936

Open
chrisbobbe opened this issue Aug 3, 2021 · 4 comments
Open

Upgrade to React Navigation v6. #4936

chrisbobbe opened this issue Aug 3, 2021 · 4 comments
Assignees
@gnprice
Labels
a-navigation P1 high-priority

Comments

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Aug 3, 2021 •
edited
Loading

v5 was #4296. v6 was released just a few hours ago; I bet they'll have a blog post soon. Edit: https://reactnavigation.org/blog/2021/08/14/react-navigation-6.0/

From their handy "Upgrading from 5.x" guide:

React Navigation 6 keeps the same API as React Navigation 5, however there are some breaking changes to make the API more consistent, more flexible and less confusing.

This guide lists all the changes that you need to keep in mind when upgrading.

There are still several TODOs in that guide, so let's maybe come back to this after those have been dealt with.
@Mahyar1982

This comment was marked as outdated.

Sign in to view
@gnprice gnprice self-assigned this Apr 7, 2022
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 28, 2022
@chrisbobbe
deps: Temporarily use fork for @react-navigation/bottom-tabs
8174d54 
To get chrisbobbe/react-navigation@b2343cb25, which I've published
to NPM in @zulip/react-navigation-bottom-tabs@5.11.16-0.zulip.1.

It cherry-picks react-navigation/react-navigation@d87857e5d, which
is a compatibility fix for RN v0.65 that React Navigation published
in v6 but didn't backport to v5.

So this fix will become unnecessary when we're on React Navigation
6; that's issue zulip#4936.

A small handful of things went wrong when building the .d.ts files
for the publish to NPM, so some TypeScript types are lost, which is
fine because we don't use TypeScript. But the runtime code is
updated in the ways we'd want and expect.
@chrisbobbe chrisbobbe mentioned this issue Apr 28, 2022
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 28, 2022
@chrisbobbe
deps: Temporarily use fork for @react-navigation/bottom-tabs
e2191ac 
To get chrisbobbe/react-navigation@b2343cb25, which I've published
to NPM in @zulip/react-navigation-bottom-tabs@5.11.16-0.zulip.1.

It cherry-picks react-navigation/react-navigation@d87857e5d, which
is a compatibility fix for RN v0.65 that React Navigation published
in v6 but didn't backport to v5.

So this fix will become unnecessary when we're on React Navigation
6; that's issue zulip#4936.

A small handful of things went wrong when building the .d.ts files
for the publish to NPM, so some TypeScript types are lost, which is
fine because we don't use TypeScript. But the runtime code looks
like it's updated in the ways we'd want and expect.
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 28, 2022
@chrisbobbe @gnprice
deps: Temporarily use fork for @react-navigation/bottom-tabs
c0b17bd 
To get chrisbobbe/react-navigation@b2343cb25, which I've published
to NPM in @zulip/react-navigation-bottom-tabs@5.11.16-0.zulip.1.

It cherry-picks react-navigation/react-navigation@d87857e5d, which
is a compatibility fix for RN v0.65 that React Navigation published
in v6 but didn't backport to v5.

So this fix will become unnecessary when we're on React Navigation
6; that's issue zulip#4936.

A small handful of things went wrong when building the .d.ts files
for the publish to NPM, so some TypeScript types are lost, which is
fine because we don't use TypeScript. But the runtime code looks
like it's updated in the ways we'd want and expect.
@gnprice gnprice mentioned this issue May 25, 2022
Closed
@gnprice
Copy link
Member

gnprice commented May 25, 2022

We'll do #5391 first as a step toward this.

@gnprice gnprice added the blocked on other work To come back to after another related PR, or some other task. label May 25, 2022
@gnprice gnprice removed the blocked on other work To come back to after another related PR, or some other task. label Jun 8, 2022
@gnprice gnprice mentioned this issue Jun 8, 2022
Merged
@gnprice gnprice mentioned this issue Jul 29, 2022
Merged
@chrisbobbe
Copy link
Contributor Author

I expect this would fix the P1 bug #5486; details at #5486 (comment).

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Mar 13, 2023 •
edited
Loading

I think the biggest obstacle here will be updating the Flow type definitions. That task is made much easier by TsFlower, but in an experiment this morning I found there are lots of merge conflicts with our existing patches, and also the definitions in types/@react-navigation/ start importing types from @react-navigation/elements and react-native-pager-view, so we'll need to start running TsFlower on those, and that will probably require new patches.

The upgrade guide does say you can mix v5 and v6 packages; e.g., we could bump @react-navigation/stack to v6 and keep everything else on v5. But the navigator packages (like @react-navigation/stack) seem to lock to the corresponding version of the core package @react-navigation/native in the type imports in their type definition files, so it seems smoothest just to upgrade everything together.

Other than that, the parts of the upgrade guide that apply to us hopefully pretty easy:

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 13, 2023
@chrisbobbe
deps: Temporarily use fork for @react-navigation/stack
77c9eda 
(Much like we did for @react-navigation/bottom-tabs in c0b17bd.)

To get chrisbobbe/react-navigation@a588dcee9, which I've published
to NPM in @zulip/react-navigation-stack@5.14.10-0.zulip.1.

It cherry-picks react-navigation/react-navigation@5a1987708, which
fixes zulip#5486, as noted there:
  zulip#5486 (comment)

So this fix will become unnecessary when we're on React Navigation
6; that's issue zulip#4936.

Tested the bugfix on my iPhone 13 Pro running iOS 16.1, with and
without "Debug with Chrome". The bug was fixed, and back navigation
worked as expected, both with the swipe gesture and the back button.

Fixes: zulip#5486
@chrisbobbe chrisbobbe mentioned this issue Mar 13, 2023
Merged
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue Mar 13, 2023
@chrisbobbe @gnprice
deps: Temporarily use fork for @react-navigation/stack
705c0b0 
(Much like we did for @react-navigation/bottom-tabs in c0b17bd.)

To get chrisbobbe/react-navigation@a588dcee9, which I've published
to NPM in @zulip/react-navigation-stack@5.14.10-0.zulip.1.

It cherry-picks react-navigation/react-navigation@5a1987708, which
fixes zulip#5486, as noted there:
  zulip#5486 (comment)

So this fix will become unnecessary when we're on React Navigation
6; that's issue zulip#4936.

Tested the bugfix on my iPhone 13 Pro running iOS 16.1, with and
without "Debug with Chrome". The bug was fixed, and back navigation
worked as expected, both with the swipe gesture and the back button.

Fixes: zulip#5486
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 15, 2024
@chrisbobbe
deps: Upgrade @react-navigation/material-top-tabs to 6.6.13 (latest 6.x)
ae5b9b1 
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
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Apr 15, 2024
@chrisbobbe
deps: Upgrade @react-navigation/bottom-tabs to 6.5.20 (latest 6.x)
e4c6c92 
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 mentioned this issue Apr 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gnprice gnprice

Labels
a-navigation P1 high-priority
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@gnprice @chrisbobbe @Mahyar1982