search

zulip / zulip-mobile

Zulip mobile apps for Android and iOS.
https://zulip.com/apps/
Apache License 2.0
1.29k stars 654 forks source link

Enable `react-native-screens`. #4111

Closed chrisbobbe closed 2 years ago

chrisbobbe commented 4 years ago

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 0d2066f41.

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 commented 4 years ago

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 https://github.com/zulip/zulip-mobile/pull/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 commented 4 years ago

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 https://github.com/zulip/zulip-mobile/pull/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 commented 4 years ago

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 https://github.com/zulip/zulip-mobile/pull/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 commented 4 years ago

I've just opened https://github.com/zulip/zulip-mobile/pull/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.