Upgrade React Navigation v3 and v4. #4249
Conversation
chrisbobbe
added
dependencies
|
Posting some details on my v3 upgrade procedure that seem too long to fit into the main commit message for that upgrade, which is where they apply: Update / Add DepsWe start by just With that, we get the following peer-dep output: We upgrade react-navigation-tabs to ~1.2.0, which we choose because our new react-navigation v3 has that as a direct dependency (more on this below). The react-navigation v4.x [sic] upgrade guide [3] recommends starting that upgrade by adding three direct dependencies, for an interesting reason:
That reason is, """ So it seems like we'll want (or need) to depend on these three directly, in react-navigation v4, probably because they'll newly be peer-deps of react-navigation. Might as well keep react-navigation-tabs (and close #4114), update its version, and add the other two right now. However, the indicated version ranges aren't quite right for v3 compatibility; they would resolve react-navigation-stack to 1.10.3, which appears to require react-navigation 4.x (expressed as a peer-dep). To be safe, we just pick the version ranges straight from react-navigation v3's direct dependencies:
At this point, the peer-dep output is the following: For each package name, we take the intersection of its valid ranges, adding
at which point there are no more peer-dep warnings. The 3.x installation guide also says we need to add
Next, the doc says we need to add some Java to our MainActivity.java, to get react-native-gesture-handler up and running. Since bdce7d0, we don't have a MainActivity.java; ours was converted to Kotlin. The react-native-gesture-handler setup doc [5] doesn't have a Kotlin version of the added code either, so I temporarily revived MainActivity.java from bdce7d0^, added it in there, used Android Studio to convert the file to Kotlin, and inserted the translated new Kotlin into our MainActivity.kt. At this point, Jest fails because react-native-gesture-handler depends on a native module that won't be available in Jest. So, follow a recommendation [6] to run that package's own Jest setup, as a fix. (I wish more packages shipped with these and suggested this approach!) Even though we're trying to stop using react-navigation-redux-helpers (and we'll need to by react-navigation v5; see #3804), we haven't yet. And we have to take its v3 and handle the breaking changes [7]. All three breaking changes apply to us, but they're small and simple. Also, removing this library from our Flow config's
Also, run LibdefsA react-navigation v3 libdef from FlowTyped exists, and it looks good! This saves us hours of work. Particularly helpful and admirable is the fact that they've gone and painstakingly copied some things over from other modules (with a few minor errors, some or all of which will be fixed in v4; see above), since they can't be imported [8]. There is a small hiccup (not that libdef's fault); For the other dependencies added in this commit:
Breaking changesNow all we have to do is handle the announced breaking changes [2], as follows:
[1] https://reactnavigation.org/docs/3.x/getting-started/ |
In this commit: - Update react-navigation and its libdef, handle breaking changes in code - Update react-navigation-redux-helpers to keep compatibility; handle breaking changes and let it wrap AppNavigator in its preferred way - Add react-navigation-stack and react-navigation-drawer to prepare for the requirement in v4 that we depend on these directly. Match the versions of these (and that of react-navigation-tabs, which we're already depending on) to the versions react-navigation v3 has for these in its `dependencies` in its `package.json`. Grab a compatible libdef for react-navigation-stack. - Add @react-navigation/core, @react-navigation/native, and react-native-gesture-handler to satisfy peer dependencies. Follow instructions for some additional setup for react-native-gesture-handler in Jest config and `MainActivity.kt`. - Add a note about a console error that we're getting, which will go away in react-navigation v4. It doesn't seem to break any functionality. See more detail on GitHub at zulip#4249 (comment). Fixes: zulip#3649
chrisbobbe
force-pushed
the
pr-react-navigation-3-and-4
branch
from
ef7acf3 to
f137773
Compare
My notes on the v4 upgrade were smaller, so I just have those in the commit message. |
In this commit: - Update react-navigation and its libdef, handle breaking changes in code - Update react-navigation-redux-helpers to keep compatibility; handle breaking changes and let it wrap AppNavigator in its preferred way - Add react-navigation-stack and react-navigation-drawer to prepare for the requirement in v4 that we depend on these directly. Match the versions of these (and that of react-navigation-tabs, which we're already depending on) to the versions react-navigation v3 has for these in its `dependencies` in its `package.json`. Grab a compatible libdef for react-navigation-stack. - Add @react-navigation/core, @react-navigation/native, and react-native-gesture-handler to satisfy peer dependencies. Follow instructions for some additional setup for react-native-gesture-handler in Jest config and `MainActivity.kt`. - Add a note about a console error that we're getting, which will go away in react-navigation v4. It doesn't seem to break any functionality. Also, run `yarn yarn-deduplicate && yarn` as prompted by `tools/test deps`. See more detail on GitHub at zulip#4249 (comment). Fixes: zulip#3649
chrisbobbe
force-pushed
the
pr-react-navigation-3-and-4
branch
from
f137773 to
8d4cb27
Compare
|
Just fixed a conflict. |
|
Thanks @chrisbobbe ! These changes all look good. Now trying it out for some manual testing.
Eep, sorry for the runaround. One thing I actually quite appreciated about Kotlin when I first started really using it was that because its designers have taken great care over their smooth interoperation with Java, it's very straightforward to translate Java code into Kotlin. I found in particular that I could search the web for how to do something in Android, find some possible solution on StackOverflow that was in Java (because until recent years Java was the one primary language for Android), and look at the Java code and transcribe it as Kotlin into my editor just about as fast as I could have simply retyped it as Java. (Then in many cases there are fancier Kotlin features you could use to improve the code from there -- but there's always a perfectly reasonable Kotlin version that is just a straightforward translation from Java.) But naturally that only works once you have some level of familiarity with both Java and Kotlin, which I think you haven't yet had occasion to really work with. That's a clever workaround you found, and I'm glad it worked out smoothly! |
|
Some observations from playing with this branch, on Android both on a physical and an emulated device:
When you try it out, do you reproduce these? I don't think any of these are a blocker, if we can't find a way to avoid them while upgrading this library. But the scrolling issue seems like it'll be pretty annoying in future development on Android with the emulator (which is the environment I use most often.) And the "wipe" animation seems like a regression, though a subtle one, in the UX. So if we can, I'd like to find a way to address each of those. |
|
I did a bit of investigation on the "wipe animation" issue. This source file looks relevant: /**
* Standard Android-style reveal from the bottom for Android Pie.
*/
export function forRevealFromBottomAndroid({An earlier version used the term "wipe" instead of "reveal", which helped me find it. And, poking around at stock apps on my (emulated) Android 9 P device, I can indeed find some examples of a wipe/reveal animation! Specifically it's how the system settings app brings up new screens. But there are also lots of different animations that appear in other circumstances in stock apps. In particular I can't find any examples of a wipe/reveal animation where the old and new screens don't have the same style of app bar at the top, and the same background color both for the app bar and the rest of the screen respectively. We have lots of transitions that don't fit that description, thanks to the stream-colored app bars as well as the lightbox. Also, I'm seeing this same wipe/reveal animation when I use the app on Android 10 Q. And it is not normal there. Perhaps we can resolve this by simply picking a different type of animation? They do seem to have a nice selection of them, here: |
No problem! 🙂 |
We can offer swiping between tabs! It's a prop,
Sure, looks like we can do this! We only have one stack navigator. In it, we can set one of those animation presets, either in |
Hmm, I'm not seeing this in the Android emulator. It seems as smooth as ever, given that for me it's always faster on a physical device. I'm not seeing an abrupt turnaround when I do a "coasting" scroll. |
|
Hmm, one thing I noticed on iOS: the navigation away from the |
In one of the recent React Navigation upgrades, the animation for a new screen entering and leaving the stack, on Android, changed. We like how it was before [1]. So, put it back. [1] zulip#4249 (comment)
In one of the recent React Navigation upgrades, a rapid sideways scroll animation was added for when you tap another tab to switch to it. This feels better than having no animation, but it really suggests that swiping ought to work to switch between tabs [1]. We haven't been offering that; so, do that. [1] zulip#4249 (comment)
|
OK, I just pushed a revision that should address everything except the scrolling issues you saw on the Android emulator (I didn't reproduce these) and the new slide-from-right animation when leaving the I added two new commits. One thing I could do that I haven't done yet is identify whether the issues they address were introduced with the v3 upgrade or the v4 upgrade; this would be useful if we want to plan for urgently reverting back to v3 and staying there for some time, and perhaps not very useful otherwise. |
Well, and now I'm seeing the same symptom on master. 🙂 So I guess it's not about the new gesture handler after all. I'm also not seeing the symptom when I go use a different emulated device, at least on master. (I should go try it on this branch, I guess.) Specifically:
edit: Yup, and same result on this branch -- on that second emulated device, scrolling is very nice and smooth. |
There was a problem hiding this comment.
We can offer swiping between tabs! It's a prop,
swipeEnabled. Might as well?
Sounds good to me! And I just played with it on both the streams tabs and the message-reaction list, and it felt like a good interaction.
I think that fixes #3997, too.
Hmm, one thing I noticed on iOS: the navigation away from the
LoadingScreen(seen on startup) is now a slide-from-right, which looks a little odd—but maybe just because I'm not used to seeing it?
I agree that sounds odd. What we've previously had (and I just fact-checked this on my iPad, which is running v26.29.152) is that when we're done loading it goes immediately to the main screen, with no transition. That feels like the right behavior -- anything else just prolongs the effect of the loading screen. So fixing that would be a good followup task.
On Android, similarly, what I see in the release on my phone is an instant switch with no transition. I'm not quite sure if this branch introduces a transition there or not; I'd guess it does, but I'm not clearly seeing it. But then the normal "fade from bottom" transition is quick enough that I'm not sure that means it isn't happening.
src/nav/AppNavigator.js
Outdated
| defaultNavigationOptions: { | ||
| ...Platform.select({ | ||
| android: TransitionPresets.FadeFromBottomAndroid, | ||
| ios: TransitionPresets.SlideFromRightIOS, |
There was a problem hiding this comment.
Perhaps ios: TransitionPresets.DefaultTransition? That way it's explicit that we're not intending to change what we get in this case, only in the Android case.
I think we can leave that to be done lazily if needed. If we do end up doing such a revert for whatever reason, then in the worst case we'll reintroduce these issues. Both are subtle UI polish issues that don't break any functionality. See my small code comment above, and I think you'll want to mark the PR and that other new commit as fixing #3997. Then please merge at will! |
This is the recommended thing to do when it's necessary to defy react-navigation's warnings [1] against "explicitly rendering more than one navigator". In the upcoming react-navigation v2 -> v3 upgrade, this would otherwise produce a crashing error; see discussion [2]. [1] https://reactnavigation.org/docs/2.x/common-mistakes/#explicitly-rendering-more-than-one-navigator [2] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Dynamic.20routes.20in.20react-navigation/near/1008268
This has no effect.
This will prevent a crashing error on iOS and Android that would otherwise be introduced in the upcoming react-navigation v2 -> v3 upgrade, on opening the message-reaction list. See discussion [1]. [1] https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Dynamic.20routes.20in.20react-navigation/near/1008268
In this commit: - Update react-navigation and its libdef, handle breaking changes in code - Update react-navigation-redux-helpers to keep compatibility; handle breaking changes and let it wrap AppNavigator in its preferred way - Add react-navigation-stack and react-navigation-drawer to prepare for the requirement in v4 that we depend on these directly. Match the versions of these (and that of react-navigation-tabs, which we're already depending on) to the versions react-navigation v3 has for these in its `dependencies` in its `package.json`. Grab a compatible libdef for react-navigation-stack. - Add @react-navigation/core, @react-navigation/native, and react-native-gesture-handler to satisfy peer dependencies. Follow instructions for some additional setup for react-native-gesture-handler in Jest config and `MainActivity.kt`. - Add a note about a console error that we're getting, which will go away in react-navigation v4. It doesn't seem to break any functionality. Also, run `yarn yarn-deduplicate && yarn` as prompted by `tools/test deps`. See more detail on GitHub at zulip#4249 (comment). Fixes: zulip#3649
v4 includes lots of housekeeping changes that aim to make
maintenance easier [1].
In particular, we no longer grab stack, tab, or drawer navigation
logic from react-navigation itself; we must now depend directly on
separate libraries that provide these and import from them.
- We got a head start on this in the v3 upgrade (in a recent commit)
by ensuring we had react-navigation-{tabs,stack,drawer} installed
at v3-compatible versions.
- Now, we bump these to their latest versions (no peer-dep warnings
when we do this!) and default to using carets in their version
ranges since we see no indication that we can't.
- This means installing react-native-reanimated, as we knew we'd
need to. (We mock it in our Jest setup, following some
instructions [2].) For react-navigation-stack, we add peer
dependencies react-native-safe-area-context and
@react-native-community/masked-view.
The upgrade guide asks us to stop depending on
@react-navigation/core and @react-navigation/native and change any
imports of those to react-navigation imports instead. We don't have
any imports to change, and we can freely remove
@react-navigation/native. We can't yet remove @react-navigation/core
because it's still a peer dependency of
react-navigation-redux-helpers [3]. So we keep it, and align its
version range with the one in react-navigation's dependencies.
There's a long list of identifiers whose usage has changed.
Searching our code for each one, these are the interesting ones:
- `create*Navigator` being imported from the respective
react-navigation-{tabs,stack,drawer} library; quite easy to handle
- `cardStyle` (in stack nav config) being moved to
`navigationOptions` (or `defaultNavigationOptions`). We deleted
our only use of `cardStyle` in the recent react-navigation v3
upgrade because we only used it to make the background color white
and, white became the default background color in that upgrade.
Finally, attempt to upgrade libdefs for the dependencies we touched,
if we import from them directly. We get new version-appropriate
libdefs for react-navigation, react-navigation-drawer, and
react-navigation-tabs. Unfortunately react-navigation-stack only has
a v1 libdef.
[1] https://reactnavigation.org/docs/4.x/upgrading-from-3.x/
[2] See https://reactnavigation.org/docs/testing/. We don't include
the following line also recommended there:
```
// Silence the warning: Animated: `useNativeDriver` is not
// supported because the native animated module is missing
jest.mock('react-native/Libraries/Animated/src/NativeAnimatedHelper');
```
We don't get that warning in the first place, and it's not clear
what mock is being applied (apart from React Native's own
default one). If we need to mock `NativeAnimatedHelper` in the
future, we shouldn't point to the internal path in react-native;
it should go in our existing react-native mock (alongside
NativeModules, etc.).
[3] zulip#3804 (comment)
Fixes: zulip#4248
In one of the recent React Navigation upgrades, the animation for a new screen entering and leaving the stack, on Android, changed. We like how it was before [1]. So, put it back. [1] zulip#4249 (comment)
In one of the recent React Navigation upgrades, a rapid sideways scroll animation was added for when you tap another tab to switch to it. This feels better than having no animation, but it really suggests that swiping ought to work to switch between tabs [1]. We haven't been offering that; so, do that. [1] zulip#4249 (comment) Fixes: zulip#3997
chrisbobbe
force-pushed
the
pr-react-navigation-3-and-4
branch
from
0833e76 to
ad0b5f3
Compare
New in the recent React Navigation upgrades to v3 and v4, there's now a transition animation when the loading screen exits. We don't want to prolong the effect of the loading screen [1], so, get rid of that exit animation. Most accurately, we want to target the exit of the loading screen and disable the animation there. But it seems that the `animationEnabled` switch in `navigationOptions` operates on the *entrance* of particular screen; I haven't found a similar option for the *exit* of a particular screen. A good workaround, to the extent that it's practical, would be to tell all screens to disable their entrance animations if and only if they're being reached directly from the loading screen. The only reasonably simple way I can think of to implement this would be to set `defaultNavigationOptions` to a function instead of an object [2] and use that function's inputs to conditionalize on the identity of the route being navigated *from*, i.e., the previous route. Unfortunately, that doesn't seem possible; the `navigation` object is the most likely-looking of those inputs, and logging shows that it doesn't have data about the previous route, only the current route (in `navigation.state`). So, we use a heuristic: always cancel the entrance animation of the main-tabs screen. This is supported by the following observations (but naturally there may be edge cases I haven't considered): - An extremely frequent and high-visibility action is to navigate from the loading screen to the main-tabs screen. - We don't generally navigate from the loading screen to anywhere other than the main-tabs navigator. - We don't generally navigate to the main-tabs screen from anywhere other than the loading screen. [1] zulip#4249 (review) [2] The best link I've found for this is https://reactnavigation.org/docs/4.x/headers/#using-params-in-the-title.
OK, done! Thanks for the review.
OK, just sent #4260 for this. |
New in the recent React Navigation upgrades to v3 and v4, there's now a transition animation when the loading screen exits. We don't want to prolong the effect of the loading screen [1], so, get rid of that exit animation. Most accurately, we want to target the exit of the loading screen and disable the animation there. But it seems that the `animationEnabled` switch in `navigationOptions` operates on the *entrance* of particular screen; I haven't found a similar option for the *exit* of a particular screen. A good workaround, to the extent that it's practical, would be to tell all screens to disable their entrance animations if and only if they're being reached directly from the loading screen. The only reasonably simple way I can think of to implement this would be to set `defaultNavigationOptions` to a function instead of an object [2] and use that function's inputs to conditionalize on the identity of the route being navigated *from*, i.e., the previous route. Unfortunately, that doesn't seem possible; the `navigation` object is the most likely-looking of those inputs, and logging shows that it doesn't have data about the previous route, only the current route (in `navigation.state`). So, we use a heuristic: always cancel the entrance animation of the main-tabs screen. This is supported by the following observations (but naturally there may be edge cases I haven't considered): - An extremely frequent and high-visibility action is to navigate from the loading screen to the main-tabs screen. - We don't generally navigate from the loading screen to anywhere other than the main-tabs screen. - We don't generally navigate to the main-tabs screen from anywhere other than the loading screen. [1] zulip#4249 (review) [2] The best link I've found for this is https://reactnavigation.org/docs/4.x/headers/#using-params-in-the-title.
This is blocking the RN v0.61 -> v0.62 upgrade (#3782); see discussion.
This supersedes #3502, which has gone stale. One part of that PR that stands out as not covered here (or in prior merged work on upgrading to v2):
9b101f7 navigation: Replace
getSameRoutesCountwithpopToTopThis looks like a followup to the v2 upgrade, using something that wasn't available in v1. (Seems like
props.navigation.popToTopwas available, but notStackActions.popToTop.) We could do that in a followup, or alongside other work in reshaping our navigation logic (like #3954 or #3804), or we could lump it in here.Fixes: #3649
Fixes: #4248
Fixes: #3997