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

Enable react-native-screens. #4111

Closed
chrisbobbe opened this issue May 14, 2020 · 4 comments · Fixed by #5421
Closed

Enable react-native-screens. #4111

chrisbobbe opened this issue May 14, 2020 · 4 comments · Fixed by #5421
Assignees
@gnprice
Labels
a-navigation

Comments

@chrisbobbe
Copy link
Contributor

chrisbobbe commented May 14, 2020 •
edited
Loading

This issue should start with a code change that's pretty small and straightforward~~, after "autolinking", which is #4026. I'll mark this issue as blocked on that work.~~ The bulk of the work required will be stress-testing to see if anything breaks, and perhaps collecting some performance data.

As foreshadowed in this blog post, and discussed in this talk (notable timestamps are 18:43 and 27:04), there's supposed to be a way we can substantially free up resources, to make our navigation go a lot faster. More discussion here and following.

Assuming we're still on React Navigation v2, just follow this doc, choosing the "Setup in normal react-native applications" path. Instead of following the link to the instructions for the latest version of react-native-screens, you'll want to find the doc corresponding to the version we're using, which is 1.0.0-alpha.23 as of this writing. To do that, make sure you've run yarn, and open node_modules/react-native-screens/README.md. Look at the instructions under "Usage with react-navigation (without Expo)".

Skip the yarn add instruction unless you have a particular react-native-screens version in mind that will satisfy all peer dependency constraints. We did that step in 0d2066f.

Skip the react-native link instruction; this is taken care of by "autolinking". Actually, we'll want to edit our react-native.config.js to not blacklist react-native-screens for Android and/or iOS.

Do everything else.
@chrisbobbe chrisbobbe added blocked on other work To come back to after another related PR, or some other task. a-navigation labels May 14, 2020
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented May 14, 2020 •
edited
Loading

Once it's all set up (you can refer to this doc for build failures, hopefully they won't happen), we'll want to make sure things haven't broken.

The main claim of this optimization is to free up resources on screens that aren't visible. There may be a snag for us, here, since we've never had to think through anything on a screen getting garbage collected if it's still in the navigation stack but not currently visible. In particular, we know it's possible to push an unbounded number of MessageLists to the history stack. So, try pushing a bunch of those (following links to other narrows is a good way of doing that), and then return to one of them that's kind of buried deep (not really sure how to do that yet; for a back button that works more intuitively, discussion is ongoing at #3954 and in a CZO stream linked from there). See if it's totally broken and the app crashes, or if we merely lose our scroll position, etc.

A nice next step would be to see if it's very clearly a performance improvement. If so, maybe some GIFs could be posted here. If it's still an improvement, but subtle, maybe we can do some programmatic stopwatch measurements. But the more interesting result, which would be great to have even before collecting performance data, is whether you can get the app to crash or do anything else completely unexpected, as a result of this change.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 15, 2020
@chrisbobbe
ios deps [nfc]: Mark two deps that we're explicitly not linking.
ce959bb 
In 33f4b41 (the CocoaPods commit), we sectioned off a "Pods we
need that depend on React Native" list. First, we accumulated these
dependencies one by one as we observed build failures caused by
their absence. (It's self-evident that we did this, at least, but
I'm fairly certain we also went down a list in the Xcode UI of
libraries that were linked.) Then, we checked this list in the
Podfile against a search of libraries in node_modules with a
"podspec" file that declared a dependency on React Native.

From that commit:

"""
We also added a number of dependencies that depend on React Native
(react-native-image-picker, etc.).
`grep -Rlw 'React' --include=\*.podspec node_modules/` verified
that they do indeed depend on the 'React' pod, which is React
Native.
"""

Now, while working on the RN v0.60.0 upgrade, I decided to re-check
that list with a more precise search:

```
grep -Rl dependency.*React --include=\*.podspec --exclude="node_modules/react-native/*" node_modules
```

-- we were looking for non-RN libraries that have a podspec with a
line like `s.dependency 'React'`.

The results of this search are much cleaner. It

    (a) excludes podspec files in `react-native`, and

    (b) excludes offhand, code-comment mentions of React.

(b) didn't change the result set at all, but (a) reduced a lot of
noise. Following cb87f90, there's a result from `@unimodules`
which we can ignore; that's handled by `use_unimodules!`.

I re-checked that everything in that list in our Podfile matched one
of these search results; so far, so good. But with this cleaner
result set, I had the idea to check the reverse: that each item in
the result set (except the irrelevant `@unimodules` one) was
represented in that list in the Podfile.

Two results were not: `react-native-document-picker` and
`react-native-screens`.

I verified that, in fact, we weren't linking either of these before
the CocoaPods commit. But it's best to mark this explicitly, with my
(new, alas) knowledge that we're excluding them intentionally.

Some background on why we didn't link them --

On iOS, there are currently no uses of
`react-native-document-picker`; it's only imported in
`src/compose/ComposeMenu.js`, and there, only to enable file uploads
on Android. See d9bbb5e, and the discussion at zulip#3490, where it
looks like we explicitly decided *not* to link it on iOS -- some
stuff in `project.pbxproj` was auto-generated, as we'd expect before
CocoaPods, and Greg removed it as unnecessary.

Piecing together the `react-native-screens` story was more
interesting, and it resulted in filing #M4111. See also discussion
in chat at
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877096.
We haven't really known what this library can do for us, but now we
sort of do. Anyway, we explicitly decided not to link it during work
on 0d2066f; see
https://chat.zulip.org/#narrow/stream/48-mobile/topic/Windows.20build/near/794533;
that will change during work on zulip#4111.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 15, 2020
@chrisbobbe
ios deps [nfc]: Mark two deps that we're explicitly not linking.
b5b1898 
In 33f4b41 (the CocoaPods commit), we sectioned off a "Pods we
need that depend on React Native" list. First, we accumulated these
dependencies one by one as we observed build failures caused by
their absence. (It's self-evident that we did this, at least, but
I'm fairly certain we also went down a list in the Xcode UI of
libraries that were linked.) Then, we checked this list in the
Podfile against a search of libraries in node_modules with a
"podspec" file that declared a dependency on React Native.

From that commit:

"""
We also added a number of dependencies that depend on React Native
(react-native-image-picker, etc.).
`grep -Rlw 'React' --include=\*.podspec node_modules/` verified
that they do indeed depend on the 'React' pod, which is React
Native.
"""

Now, while working on the RN v0.60.0 upgrade, I decided to re-check
that list with a more precise search:

```
grep -Rl dependency.*React --include=\*.podspec --exclude="node_modules/react-native/*" node_modules
```

-- we were looking for non-RN libraries that have a podspec with a
line like `s.dependency 'React'`.

The results of this search are much cleaner. It

    (a) excludes podspec files in `react-native`, and

    (b) excludes offhand, code-comment mentions of React.

(b) didn't change the result set at all, but (a) reduced a lot of
noise. Following cb87f90, there's a result from `@unimodules`
which we can ignore; that's handled by `use_unimodules!`.

I re-checked that everything in that list in our Podfile matched one
of these search results; so far, so good. But with this cleaner
result set, I had the idea to check the reverse: that each item in
the result set (except the irrelevant `@unimodules` one) was
represented in that list in the Podfile.

Two results were not: `react-native-document-picker` and
`react-native-screens`.

I verified that, in fact, we weren't linking either of these before
the CocoaPods commit. But it's best to mark this explicitly, with my
(new, alas) knowledge that we're excluding them intentionally.

Some background on why we didn't link them --

On iOS, there are currently no uses of
`react-native-document-picker`; it's only imported in
`src/compose/ComposeMenu.js`, and there, only to enable file uploads
on Android. See d9bbb5e, and the discussion at zulip#3490, where it
looks like we explicitly decided *not* to link it on iOS -- some
stuff in `project.pbxproj` was auto-generated, as we'd expect before
CocoaPods, and Greg removed it as unnecessary.

Piecing together the `react-native-screens` story was more
interesting, and it resulted in filing #M4111. See also discussion
in chat at
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877096.
We haven't really known what this library can do for us, but now we
sort of do. Anyway, we explicitly decided not to link it during work
on 0d2066f; see
https://chat.zulip.org/#narrow/stream/48-mobile/topic/Windows.20build/near/794533;
that will change during work on zulip#4111.
@chrisbobbe chrisbobbe mentioned this issue May 15, 2020
Merged
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 15, 2020
@chrisbobbe
deps: Remove react-navigation-tabs.
e04d27b 
In 0d2066f, we followed a warning to use the `react-native-tabs`
package:

```
console.warn node_modules/react-navigation/src/react-navigation.js:180
  TabBarBottom is deprecated. Please use the react-navigation-tabs package
  instead: https://github.com/react-navigation/react-navigation-tabs
```

An important bit of context that's missing from this warning is that
`react-navigation-tabs`'s function `createBottomTabNavigator`,
specifically, is what replaces what was previously done with
`TabBarBottom`.

`react-navigation` also exports another function
`createBottomTabNavigator`. So, is it so important to use the export
from `react-navigation-tabs`?

Turns out it's not; that function in `react-navigation` is an
extremely thin wrapper on the one in `react-navigation-tabs`:

```
get createBottomTabNavigator() {
  return require('react-navigation-tabs').createBottomTabNavigator;
},
```

So, `react-navigation` depends on `react-navigation-tabs`, so we can
remove the latter as a direct dependency in our project.

The `react-navigation` v2 doc [1] also doesn't say anything about
needing to depend directly on `react-navigation-tabs`. See also
further discussion [2].

It would also have been possible to remove the direct dependency on
`react-native-screens`, but we're planning to use that later; that's
issue zulip#4111.

[1]: https://reactnavigation.org/docs/2.x/tab-based-navigation/
[2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877185
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 15, 2020
@chrisbobbe
deps: Remove react-navigation-tabs.
d71abdd 
See the previous commit for the reason for the $FlowFixMe.

In 0d2066f, we followed a warning to use the `react-native-tabs`
package:

```
console.warn node_modules/react-navigation/src/react-navigation.js:180
  TabBarBottom is deprecated. Please use the react-navigation-tabs package
  instead: https://github.com/react-navigation/react-navigation-tabs
```

An important bit of context that's missing from this warning is that
`react-navigation-tabs`'s function `createBottomTabNavigator`,
specifically, is what replaces what was previously done with
`TabBarBottom`.

`react-navigation` also exports another function
`createBottomTabNavigator`. So, is it so important to use the export
from `react-navigation-tabs`?

Turns out it's not; that function in `react-navigation` is an
extremely thin wrapper on the one in `react-navigation-tabs`:

In node_modules/react-navigation/src/react-navigation.js:69:
```
get createBottomTabNavigator() {
  return require('react-navigation-tabs').createBottomTabNavigator;
},
```

So, `react-navigation` depends on `react-navigation-tabs`, so we can
remove the latter as a direct dependency in our project.

The `react-navigation` v2 doc [1] also doesn't say anything about
needing to depend directly on `react-navigation-tabs`. See also
further discussion [2].

It would also have been possible to remove the direct dependency on
`react-native-screens`, but we're planning to use that later; that's
issue zulip#4111.

[1]: https://reactnavigation.org/docs/2.x/tab-based-navigation/
[2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877185
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 18, 2020
@chrisbobbe
ios deps [nfc]: Mark two deps that we're explicitly not linking.
616b743 
In 33f4b41 (the CocoaPods commit), we sectioned off a "Pods we
need that depend on React Native" list. First, we accumulated these
dependencies one by one as we observed build failures caused by
their absence. (It's self-evident that we did this, at least, but
I'm fairly certain we also went down a list in the Xcode UI of
libraries that were linked.) Then, we checked this list in the
Podfile against a search of libraries in node_modules with a
"podspec" file that declared a dependency on React Native.

From that commit:

"""
We also added a number of dependencies that depend on React Native
(react-native-image-picker, etc.).
`grep -Rlw 'React' --include=\*.podspec node_modules/` verified
that they do indeed depend on the 'React' pod, which is React
Native.
"""

Now, while working on the RN v0.60.0 upgrade, I decided to re-check
that list with a more precise search:

```
grep -Rl dependency.*React --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

-- we were looking for non-RN libraries that have a podspec with a
line like `s.dependency 'React'`.

The results of this search are much cleaner. It

    (a) excludes podspec files in `react-native`, and

    (b) excludes offhand, code-comment mentions of React.

(b) didn't change the result set at all, but (a) reduced a lot of
noise. Following cb87f90, there's a result from `@unimodules`
which we can ignore; that's handled by `use_unimodules!`.

I re-checked that everything in that list in our Podfile matched one
of these search results; so far, so good. But with this cleaner
result set, I had the idea to check the reverse: that each item in
the result set (except the irrelevant `@unimodules` one) was
represented in that list in the Podfile.

Two results were not: `react-native-document-picker` and
`react-native-screens`.

I verified that, in fact, we weren't linking either of these before
the CocoaPods commit. But it's best to mark this explicitly, with my
(new, alas) knowledge that we're excluding them intentionally.

Some background on why we didn't link them --

On iOS, there are currently no uses of
`react-native-document-picker`; it's only imported in
`src/compose/ComposeMenu.js`, and there, only to enable file uploads
on Android. See d9bbb5e, and the discussion at zulip#3490, where it
looks like we explicitly decided *not* to link it on iOS -- some
stuff in `project.pbxproj` was auto-generated, as we'd expect before
CocoaPods, and Greg removed it as unnecessary.

Piecing together the `react-native-screens` story was more
interesting, and it resulted in filing #M4111. See also discussion
in chat [1]. We haven't really known what this library can do for
us, but now we sort of do. Anyway, we explicitly decided not to link
it during work on 0d2066f; see discussion [2]. That will change
during work on zulip#4111.

[1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877096.
[2]: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Windows.20build/near/794533;
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 18, 2020
@chrisbobbe
ios deps [nfc]: Mark two deps that we're explicitly not linking.
20b92aa 
In 33f4b41 (the CocoaPods commit), we sectioned off a "Pods we
need that depend on React Native" list. First, we accumulated these
dependencies one by one as we observed build failures caused by
their absence. (It's self-evident that we did this, at least, but
I'm fairly certain we also went down a list in the Xcode UI of
libraries that were linked.) Then, we checked this list in the
Podfile against a search of libraries in node_modules with a
"podspec" file that declared a dependency on React Native.

From that commit:

"""
We also added a number of dependencies that depend on React Native
(react-native-image-picker, etc.).
`grep -Rlw 'React' --include=\*.podspec node_modules/` verified
that they do indeed depend on the 'React' pod, which is React
Native.
"""

Now, while working on the RN v0.60.0 upgrade, I decided to re-check
that list with a more precise search:

```
grep -Rl dependency.*React --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

-- we were looking for non-RN libraries that have a podspec with a
line like `s.dependency 'React'`.

The results of this search are much cleaner. It

    (a) excludes podspec files in `react-native`, and

    (b) excludes offhand, code-comment mentions of React.

(b) didn't change the result set at all, but (a) reduced a lot of
noise. Following cb87f90, there's a result from `@unimodules`
which we can ignore; that's handled by `use_unimodules!`.

I re-checked that everything in that list in our Podfile matched one
of these search results; so far, so good. But with this cleaner
result set, I had the idea to check the reverse: that each item in
the result set (except the irrelevant `@unimodules` one) was
represented in that list in the Podfile.

Two results were not: `react-native-document-picker` and
`react-native-screens`.

I verified that, in fact, we weren't linking either of these before
the CocoaPods commit. But it's best to mark this explicitly, with my
(new, alas) knowledge that we're excluding them intentionally.

Some background on why we didn't link them --

On iOS, there are currently no uses of
`react-native-document-picker`; it's only imported in
`src/compose/ComposeMenu.js`, and there, only to enable file uploads
on Android. See d9bbb5e, and the discussion at zulip#3490, where it
looks like we explicitly decided *not* to link it on iOS -- some
stuff in `project.pbxproj` was auto-generated, as we'd expect before
CocoaPods, and Greg removed it as unnecessary.

Piecing together the `react-native-screens` story was more
interesting, and it resulted in filing #M4111. See also discussion
in chat [1]. We haven't really known what this library can do for
us, but now we sort of do. Anyway, we explicitly decided not to link
it during work on 0d2066f; see discussion [2]. That will change
during work on zulip#4111.

[1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877096
[2]: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Windows.20build/near/794533
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue May 20, 2020
@chrisbobbe
deps: Remove react-navigation-tabs.
c0c50d5 
See the previous commit for the reason for the $FlowFixMe.

In 0d2066f, we followed a warning to use the `react-native-tabs`
package:

```
console.warn node_modules/react-navigation/src/react-navigation.js:180
  TabBarBottom is deprecated. Please use the react-navigation-tabs package
  instead: https://github.com/react-navigation/react-navigation-tabs
```

An important bit of context that's missing from this warning is that
`react-navigation-tabs`'s function `createBottomTabNavigator`,
specifically, is what replaces what was previously done with
`TabBarBottom`.

`react-navigation` also exports another function
`createBottomTabNavigator`. So, is it so important to use the export
from `react-navigation-tabs`?

Turns out it's not; that function in `react-navigation` is an
extremely thin wrapper on the one in `react-navigation-tabs`:

In node_modules/react-navigation/src/react-navigation.js:69:
```
get createBottomTabNavigator() {
  return require('react-navigation-tabs').createBottomTabNavigator;
},
```

So, `react-navigation` depends on `react-navigation-tabs`, so we can
remove the latter as a direct dependency in our project.

The `react-navigation` v2 doc [1] also doesn't say anything about
needing to depend directly on `react-navigation-tabs`. See also
further discussion [2].

It would also have been possible to remove the direct dependency on
`react-native-screens`, but we're planning to use that later; that's
issue zulip#4111.

The same story applies to `createMaterialTopTabNavigator`.

[1]: https://reactnavigation.org/docs/2.x/tab-based-navigation/
[2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877185
gnprice pushed a commit to chrisbobbe/zulip-mobile that referenced this issue May 26, 2020
@chrisbobbe @gnprice
ios deps [nfc]: Mark two deps that we're explicitly not linking.
250cde5 
In 33f4b41 (the CocoaPods commit), we sectioned off a "Pods we
need that depend on React Native" list. First, we accumulated these
dependencies one by one as we observed build failures caused by
their absence. (It's self-evident that we did this, at least, but
I'm fairly certain we also went down a list in the Xcode UI of
libraries that were linked.) Then, we checked this list in the
Podfile against a search of libraries in node_modules with a
"podspec" file that declared a dependency on React Native.

From that commit:

"""
We also added a number of dependencies that depend on React Native
(react-native-image-picker, etc.).
`grep -Rlw 'React' --include=\*.podspec node_modules/` verified
that they do indeed depend on the 'React' pod, which is React
Native.
"""

Now, while working on the RN v0.60.0 upgrade, I decided to re-check
that list with a more precise search:

```
grep -Rl dependency.*React --include=\*.podspec \
  --exclude="node_modules/react-native/*" node_modules
```

-- we were looking for non-RN libraries that have a podspec with a
line like `s.dependency 'React'`.

The results of this search are much cleaner. It

    (a) excludes podspec files in `react-native`, and

    (b) excludes offhand, code-comment mentions of React.

(b) didn't change the result set at all, but (a) reduced a lot of
noise. Following cb87f90, there's a result from `@unimodules`
which we can ignore; that's handled by `use_unimodules!`.

I re-checked that everything in that list in our Podfile matched one
of these search results; so far, so good. But with this cleaner
result set, I had the idea to check the reverse: that each item in
the result set (except the irrelevant `@unimodules` one) was
represented in that list in the Podfile.

Two results were not: `react-native-document-picker` and
`react-native-screens`.

I verified that, in fact, we weren't linking either of these before
the CocoaPods commit. But it's best to mark this explicitly, with my
(new, alas) knowledge that we're excluding them intentionally.

Some background on why we didn't link them --

On iOS, there are currently no uses of
`react-native-document-picker`; it's only imported in
`src/compose/ComposeMenu.js`, and there, only to enable file uploads
on Android. See d9bbb5e, and the discussion at zulip#3490, where it
looks like we explicitly decided *not* to link it on iOS -- some
stuff in `project.pbxproj` was auto-generated, as we'd expect before
CocoaPods, and Greg removed it as unnecessary.

Piecing together the `react-native-screens` story was more
interesting, and it resulted in filing #M4111. See also discussion
in chat [1]. We haven't really known what this library can do for
us, but now we sort of do. Anyway, we explicitly decided not to link
it during work on 0d2066f; see discussion [2]. That will change
during work on zulip#4111.

[1]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877096
[2]: https://chat.zulip.org/#narrow/stream/48-mobile/topic/Windows.20build/near/794533
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 4, 2020
@chrisbobbe
deps: Remove react-navigation-tabs.
40ffc19 
See the previous commit for the reason for the $FlowFixMe.

In 0d2066f, we followed a warning to use the `react-native-tabs`
package:

```
console.warn node_modules/react-navigation/src/react-navigation.js:180
  TabBarBottom is deprecated. Please use the react-navigation-tabs package
  instead: https://github.com/react-navigation/react-navigation-tabs
```

An important bit of context that's missing from this warning is that
`react-navigation-tabs`'s function `createBottomTabNavigator`,
specifically, is what replaces what was previously done with
`TabBarBottom`.

`react-navigation` also exports another function
`createBottomTabNavigator`. So, is it so important to use the export
from `react-navigation-tabs`?

Turns out it's not; that function in `react-navigation` is an
extremely thin wrapper on the one in `react-navigation-tabs`:

In node_modules/react-navigation/src/react-navigation.js:69:
```
get createBottomTabNavigator() {
  return require('react-navigation-tabs').createBottomTabNavigator;
},
```

So, `react-navigation` depends on `react-navigation-tabs`, so we can
remove the latter as a direct dependency in our project.

The `react-navigation` v2 doc [1] also doesn't say anything about
needing to depend directly on `react-navigation-tabs`. See also
further discussion [2].

It would also have been possible to remove the direct dependency on
`react-native-screens`, but we're planning to use that later; that's
issue zulip#4111.

The same story applies to `createMaterialTopTabNavigator`.

[1]: https://reactnavigation.org/docs/2.x/tab-based-navigation/
[2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877185
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 4, 2020
@chrisbobbe
deps: Remove react-navigation-tabs.
8b9f5ab 
See the previous commit for the reason for the $FlowFixMes.

In 0d2066f, we followed a warning to use the `react-native-tabs`
package:

```
console.warn node_modules/react-navigation/src/react-navigation.js:180
  TabBarBottom is deprecated. Please use the react-navigation-tabs package
  instead: https://github.com/react-navigation/react-navigation-tabs
```

An important bit of context that's missing from this warning is that
`react-navigation-tabs`'s function `createBottomTabNavigator`,
specifically, is what replaces what was previously done with
`TabBarBottom`.

`react-navigation` also exports another function
`createBottomTabNavigator`. So, is it so important to use the export
from `react-navigation-tabs`?

Turns out it's not; that function in `react-navigation` is an
extremely thin wrapper on the one in `react-navigation-tabs`:

In node_modules/react-navigation/src/react-navigation.js:69:
```
get createBottomTabNavigator() {
  return require('react-navigation-tabs').createBottomTabNavigator;
},
```

So, `react-navigation` depends on `react-navigation-tabs`, so we can
remove the latter as a direct dependency in our project.

The `react-navigation` v2 doc [1] also doesn't say anything about
needing to depend directly on `react-navigation-tabs`. See also
further discussion [2].

It would also have been possible to remove the direct dependency on
`react-native-screens`, but we're planning to use that later; that's
issue zulip#4111.

The same story applies to `createMaterialTopTabNavigator`.

[1]: https://reactnavigation.org/docs/2.x/tab-based-navigation/

[2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877185
@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jun 12, 2020 •
edited
Loading

Here's a dramatic before-and-after view, on iOS. At 19:30 of that talk, it shows you a very small button in the Xcode UI, where you can see an anatomy of all rendered UI components. In each of these, I clicked links so that there would be four MessageList routes pushed to the stack (I started from this message and clicked the recursive link there three times).

Without useScreens():

without useScreens

With useScreens():

with useScreens

With useScreens(), logging shows that the MessageList components are not indeed being unmounted, as I'd expected. This means that I can press a back button (the one implemented in #3954) and the scroll position is maintained.

I still have some trouble understanding exactly what's happening as described in that talk, but it seems like the non-visible screens are being frozen and removed (though still able to be re-introduced into things) on a level that React isn't aware of and doesn't have control over. Tasks like drawing rectangles can be skipped...maybe (from that talk) things related to the camera and maps/geolocation get to be dropped, freeing up resources?

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jun 12, 2020 •
edited
Loading

This problem does not seem to be substantially fixed. Although, more recently: maybe the wait times are more like 10-20 seconds instead of over a minute? I'm not getting a huge number of trials because I don't want to waste the time doing so. (EDIT: After debugging, I opened #4160 for this.)

Now that, experimentally, I've seen that not even the scroll position is messed up, maybe it's time to try this in an alpha or beta release? I haven't tested at all on Android, but we could always enable this for one platform at a time.

@chrisbobbe chrisbobbe removed the blocked on other work To come back to after another related PR, or some other task. label Jun 12, 2020
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 15, 2020
@chrisbobbe
navigation: Use react-native-screens.
22979fe 
Follow a doc [1] for optimizing memory usage and performance by
making `react-navigation` work more closely with native navigation
controls. That doc links to some performance comparisons [2], and
you can also see a talk [3] by the library's author (notable
timestamps are 18:43 and 27:04).

It means removing `react-native-screens` from a list of libraries to
exclude in autolinking.

We would also have liked to upgrade this to the latest version (or
even a non-alpha 1.0.0), but it won't be possible until we're on a
later version of React Navigation because of peer dependency
constraints in `react-native-tabs`. The react-navigation/tabs repo
has been archived and made read-only (there seems to be a different
structure in place with later `react-navigation` versions) and the
archived state of that repo has a `react-native-screens` peer
dependency of "^1.0.0 || ^1.0.0-alpha". A non-alpha 1.0.0 version
doesn't exist, and 23 is the latest. So, we'll stay there.

[1]: https://reactnavigation.org/docs/2.x/react-native-screens/
[2]: https://twitter.com/janicduplessis/status/1039979591815897088?s=21
[3]: https://www.youtube.com/watch?v=Z0Jl1KCWiag

Fixes: zulip#4111
@chrisbobbe chrisbobbe mentioned this issue Jun 15, 2020
Closed
@chrisbobbe
Copy link
Contributor Author

I've just opened #4161 with this enabled, so I or others can come back to it and do some performance testing, as desired. Or, with the knowledge that it's theoretically a performance improvement, we could check for bugs and just release it at some point and see how it goes. The toll on performance in being able to push routes to the stack indefinitely is a known pain point.

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jun 25, 2020
@chrisbobbe
deps: Remove react-navigation-tabs.
8c39c5f 
See the previous commit for the reason for the $FlowFixMes.

In 0d2066f, we followed a warning to use the `react-native-tabs`
package:

```
console.warn node_modules/react-navigation/src/react-navigation.js:180
  TabBarBottom is deprecated. Please use the react-navigation-tabs package
  instead: https://github.com/react-navigation/react-navigation-tabs
```

An important bit of context that's missing from this warning is that
`react-navigation-tabs`'s function `createBottomTabNavigator`,
specifically, is what replaces what was previously done with
`TabBarBottom`.

`react-navigation` also exports another function
`createBottomTabNavigator`. So, is it so important to use the export
from `react-navigation-tabs`?

Turns out it's not; that function in `react-navigation` is an
extremely thin wrapper on the one in `react-navigation-tabs`:

In node_modules/react-navigation/src/react-navigation.js:69:
```
get createBottomTabNavigator() {
  return require('react-navigation-tabs').createBottomTabNavigator;
},
```

So, `react-navigation` depends on `react-navigation-tabs`, so we can
remove the latter as a direct dependency in our project.

The `react-navigation` v2 doc [1] also doesn't say anything about
needing to depend directly on `react-navigation-tabs`. See also
further discussion [2].

It would also have been possible to remove the direct dependency on
`react-native-screens`, but we're planning to use that later; that's
issue zulip#4111.

The same story applies to `createMaterialTopTabNavigator`.

[1]: https://reactnavigation.org/docs/2.x/tab-based-navigation/

[2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877185
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Jul 23, 2020
@chrisbobbe
navigation: Use react-native-screens.
ee7c8c3 
Follow a doc [1] for optimizing memory usage and performance by
making `react-navigation` work more closely with native navigation
controls. That doc links to some performance comparisons [2], and
you can also see a talk [3] by the library's author (notable
timestamps are 18:43 and 27:04).

It means removing `react-native-screens` from a list of libraries to
exclude in autolinking.

We would also have liked to upgrade this to the latest version (or
even a non-alpha 1.0.0), but it won't be possible until we're on a
later version of React Navigation because of peer dependency
constraints in `react-native-tabs`. The react-navigation/tabs repo
has been archived and made read-only (there seems to be a different
structure in place with later `react-navigation` versions) and the
archived state of that repo has a `react-native-screens` peer
dependency of "^1.0.0 || ^1.0.0-alpha". A non-alpha 1.0.0 version
doesn't exist, and 23 is the latest. So, we'll stay there.

[1]: https://reactnavigation.org/docs/2.x/react-native-screens/
[2]: https://twitter.com/janicduplessis/status/1039979591815897088?s=21
[3]: https://www.youtube.com/watch?v=Z0Jl1KCWiag

Fixes: zulip#4111
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Aug 25, 2020
@chrisbobbe
deps: Remove react-navigation-tabs.
31b59f0 
See the previous commit for the reason for the $FlowFixMes.

In 0d2066f, we followed a warning to use the `react-native-tabs`
package:

```
console.warn node_modules/react-navigation/src/react-navigation.js:180
  TabBarBottom is deprecated. Please use the react-navigation-tabs package
  instead: https://github.com/react-navigation/react-navigation-tabs
```

An important bit of context that's missing from this warning is that
`react-navigation-tabs`'s function `createBottomTabNavigator`,
specifically, is what replaces what was previously done with
`TabBarBottom`.

`react-navigation` also exports another function
`createBottomTabNavigator`. So, is it so important to use the export
from `react-navigation-tabs`?

Turns out it's not; that function in `react-navigation` is an
extremely thin wrapper on the one in `react-navigation-tabs`:

In node_modules/react-navigation/src/react-navigation.js:69:
```
get createBottomTabNavigator() {
  return require('react-navigation-tabs').createBottomTabNavigator;
},
```

So, `react-navigation` depends on `react-navigation-tabs`, so we can
remove the latter as a direct dependency in our project.

The `react-navigation` v2 doc [1] also doesn't say anything about
needing to depend directly on `react-navigation-tabs`. See also
further discussion [2].

It would also have been possible to remove the direct dependency on
`react-native-screens`, but we're planning to use that later; that's
issue zulip#4111.

The same story applies to `createMaterialTopTabNavigator`.

[1]: https://reactnavigation.org/docs/2.x/tab-based-navigation/

[2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.3A.20r-n-screens.20.2F.20r-n-document-picker/near/877185
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jun 22, 2022
@gnprice
nav: Enable react-native-screens
92f03cb 
This greatly improves performance on Android with many screens on
the nav stack!  On iOS, the effect is less dramatic, but still
significant.

Chris investigated this back in 2020 (zulip#4111), and sent a draft PR
to enable it (zulip#4161), but we never followed up, oops.  Better late
than never.

Experiment to repro the perf difference:

  1. push like 30 ChatScreen screens to the stack, by following a
     loop of internal links;

  2. go to one with some backlog (by following the "up" icon to the
     stream message-list);

  3. try scrolling around.

Before, on Android: extremely laggy.  Also pretty laggy even just
following those links, after the first few.

After: pretty much as smooth with 30 screens on the stack as with the
first one.

On iOS, things seem to already be about as smooth with 30 screens as
with one, even before this change; in particular, scrolling through
the message list is very smooth.  (That's even though the app in my
iOS testing is fairly sluggish on the first few navigations.  I was
using the simulator on an underpowered Mac Mini, vs. the Android
emulator on my fast Linux desktop.)

But Xcode does report (under the "Debug View Hierarchy" button) that
before the change, tons and tons of views remain mounted when many
screens are on the stack; after the change, they don't.  I reproduce
the same observations Chris reported a couple of years ago here:
  zulip#4111 (comment)

More concretely, the memory usage (which Xcode reports at the same
time) gets cut way down: around 700 MB before, and around 300 MB
after.  If nothing else, that should mean that the app is less
likely to get killed by the OS if the user switches away -- so is
more likely to pick up immediately where they left off, rather than
have to reload from scratch, if the user switches back.

Fixes: zulip#4111
@gnprice gnprice mentioned this issue Jun 22, 2022
Merged
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jul 18, 2022
@gnprice
nav: Enable react-native-screens
d04f11e 
This greatly improves performance on Android with many screens on
the nav stack!  On iOS, the effect is less dramatic, but still
significant.

Chris investigated this back in 2020 (zulip#4111), and sent a draft PR
to enable it (zulip#4161), but we never followed up, oops.  Better late
than never.

Experiment to repro the perf difference:

  1. push like 30 ChatScreen screens to the stack, by following a
     loop of internal links;

  2. go to one with some backlog (by following the "up" icon to the
     stream message-list);

  3. try scrolling around.

Before, on Android: extremely laggy.  Also pretty laggy even just
following those links, after the first few.

After: pretty much as smooth with 30 screens on the stack as with the
first one.

On iOS, things seem to already be about as smooth with 30 screens as
with one, even before this change; in particular, scrolling through
the message list is very smooth.  (That's even though the app in my
iOS testing is fairly sluggish on the first few navigations.  I was
using the simulator on an underpowered Mac Mini, vs. the Android
emulator on my fast Linux desktop.)

But Xcode does report (under the "Debug View Hierarchy" button) that
before the change, tons and tons of views remain mounted when many
screens are on the stack; after the change, they don't.  I reproduce
the same observations Chris reported a couple of years ago here:
  zulip#4111 (comment)

More concretely, the memory usage (which Xcode reports at the same
time) gets cut way down: around 700 MB before, and around 300 MB
after.  If nothing else, that should mean that the app is less
likely to get killed by the OS if the user switches away -- so is
more likely to pick up immediately where they left off, rather than
have to reload from scratch, if the user switches back.

Fixes: zulip#4111
gnprice added a commit to gnprice/zulip-mobile that referenced this issue Jul 18, 2022
@gnprice
nav: Enable react-native-screens
3b0221b 
This greatly improves performance on Android with many screens on
the nav stack!  On iOS, the effect is less dramatic, but still
significant.

Chris investigated this back in 2020 (zulip#4111), and sent a draft PR
to enable it (zulip#4161), but we never followed up, oops.  Better late
than never.

Experiment to repro the perf difference:

  1. push like 30 ChatScreen screens to the stack, by following a
     loop of internal links;

  2. go to one with some backlog (by following the "up" icon to the
     stream message-list);

  3. try scrolling around.

Before, on Android: extremely laggy.  Also pretty laggy even just
following those links, after the first few.

After: pretty much as smooth with 30 screens on the stack as with the
first one.

On iOS, things seem to already be about as smooth with 30 screens as
with one, even before this change; in particular, scrolling through
the message list is very smooth.  (That's even though the app in my
iOS testing is fairly sluggish on the first few navigations.  I was
using the simulator on an underpowered Mac Mini, vs. the Android
emulator on my fast Linux desktop.)

But Xcode does report (under the "Debug View Hierarchy" button) that
before the change, tons and tons of views remain mounted when many
screens are on the stack; after the change, they don't.  I reproduce
the same observations Chris reported a couple of years ago here:
  zulip#4111 (comment)

More concretely, the memory usage (which Xcode reports at the same
time) gets cut way down: around 700 MB before, and around 300 MB
after.  If nothing else, that should mean that the app is less
likely to get killed by the OS if the user switches away -- so is
more likely to pick up immediately where they left off, rather than
have to reload from scratch, if the user switches back.

Fixes: zulip#4111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gnprice gnprice

Labels
a-navigation
Projects
None yet
Milestone
No milestone
2 participants
@gnprice @chrisbobbe