status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.91k stars 984 forks source link

Spike: POC and report on new Navigation library #20471

Open J-Son89 opened 5 months ago

J-Son89 commented 5 months ago

The current navigation library poses many issues.

We want to do a small time boxed investigation on using the library: https://reactnavigation.org/

This should investigate how:

The output should come in the form of a workable POC for some parts of the application and ideally can have a workable proposal for how an iterative migration to this library could happen. (e.g migrate certain parts of the application).

ulisesmac commented 4 months ago

@Parveshdhull @flexsurfer

This PoC won't be implemented now since we have other priorities, but it'd be great to also have your POV on the trade-offs of this change.

flexsurfer commented 4 months ago

https://github.com/status-im/status-mobile/pull/12141 This might be helpful, i think for v2 reactnavigation.org/ should work better

flexsurfer commented 4 months ago

actually we don't need POC , we already know it will work and POC will be just implementation, it's not that difficult, this also might be useful https://www.notion.so/Navigation-597d2359318d40b89a5d90d57cf08910

J-Son89 commented 4 months ago

actually we don't need POC , we already know it will work and POC will be just implementation, it's not that difficult, this also might be useful https://www.notion.so/Navigation-597d2359318d40b89a5d90d57cf08910

In a vacuum this statement is true, but right now we have several critical releases ahead of us. For these releases we want to keep the app in a releasable state at all times. As such changing the navigation library is high risk to do that as there is just many blind spots we cannot know of until we use it. It will likely be a huge and disruptive change to the codebase and we want to explore if we can do it iteratively to mitigate some of that risk. Further, changing the navigation library is low priority compared with many other items on the current roadmap. A POC will help us understand how much effort is involved with the migration, hopefully detect a few unknowns, and add weight to what actual benefits we get from this migration apart from a nicer api etc. it is not wasted work as it can serve as a reference and can speed things up a lot when/if we eventually make the move much later/ perhaps even next year.

@ulisesmac & I have found an opportune time to do this POC just after 2.30 release. 👍

Parveshdhull commented 4 months ago

hi @flexsurfer as per the notion document

RN has only one root, its RN root component, and all navigation will be always in the RN tree, just views will be not active

I am curious about how RN will address the performance problem of having a big RN tree. By views not being active do you mean subscriptions will not be triggered there? We cached subs and stopped triggering subscriptions also in the case of jump-to, still performance was affected. So I am a little worried that migration to RN might bring those problems back, wdyt?

flexsurfer commented 4 months ago

actually we don't need POC , we already know it will work and POC will be just implementation, it's not that difficult, this also might be useful https://www.notion.so/Navigation-597d2359318d40b89a5d90d57cf08910

In a vacuum this statement is true, but right now we have several critical releases ahead of us. For these releases we want to keep the app in a releasable state at all times. As such changing the navigation library is high risk to do that as there is just many blind spots we cannot know of until we use it. It will likely be a huge and disruptive change to the codebase and we want to explore if we can do it iteratively to mitigate some of that risk. Further, changing the navigation library is low priority compared with many other items on the current roadmap. A POC will help us understand how much effort is involved with the migration, hopefully detect a few unknowns, and add weight to what actual benefits we get from this migration apart from a nicer api etc. it is not wasted work as it can serve as a reference and can speed things up a lot when/if we eventually make the move much later/ perhaps even next year.

@ulisesmac & I have found an opportune time to do this POC just after 2.30 release. 👍

thanks @J-Son89, we don't have anything special in v2, so we already know what we need to do, and if it will work or not, we just need to "revert" https://github.com/status-im/status-mobile/pull/12141, and because I was implementing it, I know how long it will take, no need in POC, and it's not possible to make anything working just partially you will need to fully implement all functionality for POC

hi @flexsurfer as per the notion document

RN has only one root, its RN root component, and all navigation will be always in the RN tree, just views will be not active

I am curious about how RN will address the performance problem of having a big RN tree. By views not being active do you mean subscriptions will not be triggered there? We cached subs and stopped triggering subscriptions also in the case of jump-to, still performance was affected. So I am a little worried that migration to RN might bring those problems back, wdyt?

i think it should be better in the latest version, react-native-screens uses similar concept as react-native-navigation

I would say we should change our current screen hierarchy but it's not related to the navigation library

flexsurfer commented 4 months ago

btw moving back to reactnavigation should be simpler because we just need to mostly remove things because reactnavigation does all the navigation work, we just need to specify routing stacks https://github.com/status-im/status-mobile/pull/12141/files#diff-28737ceff0a6112787d8c0c0ff028e21a58c71b333898f1b5eb19283d4665669

flexsurfer commented 4 months ago

i'll try to answer POC questions

This should investigate how:

Home tabs will work, they will work the same or even better because in reactnavigation tabBar is just rn view navigation performance, should be the same or even better because reactnavigation uses new architecture and fabric transitions, should be better more flexible in reactnavigation handling complex navigation paths, better because reactnavigation does all the job for complex navigation Managing the status bar - better, under full control, reactnavigation doesn't interact with status bar

The output should come in the form of a workable POC for some parts of the application and ideally can have a workable proposal for how an iterative migration to this library could happen. (e.g migrate certain parts of the application). it's not possible to migrate certain parts, or make iterative changes

ulisesmac commented 5 days ago

@flexsurfer I remember you already did an exploration of React Navigation. I can continue with this one (it's blocked, but I think it can be taken now), but let me know if you already did a PoC/exploration so you can take this one or close it.

flexsurfer commented 4 days ago

hey @ulisesmac i only researched that's possible to migrate all functionality we have to reactnavigation, but there is no any code, i didn't start the work, we discussed this with @ilmotta that we'll start working on this after 2.32

ulisesmac commented 4 days ago

@flexsurfer Great! thank you.