Refactor imperative multiple dispatch usage of navigate-to

[goranjovic]

Description

Type: Refactor

When we want app to navigate to a different screen we dispatch a re-frame event navigate-to with the screen identifier as the parameter, e.g.

(re-frame/dispatch [:navigate-to :discover-search-results])

So far, so good. Now, if the said screen depends on some parameter that we need to pass from the current context to its rendering function, we can't do that explicitly. Instead, we have to first change the state of app-db explicitly, then dispatch the same code as above, e.g.

(do (re-frame/dispatch [:set :discover-search-tags [tag]])
    (re-frame/dispatch [:navigate-to :discover-search-results]))

And then screen renderer needs to use the value set by the previous dispatch.

This is inconvenient due to a few reasons:

• Two dispatches instead of one, I'm assuming just one would be a tad faster
• Correct behavior depends on dispatches being done in this order (probably true anyway, but still)
• We lose a lot of the edge gained by functional/reactive style when we revert to imperative style with mutable global state and all.

[jeluard]

It makes sense to consider #1592 when implementing this.

[janherich]

I propose intermediate solution, not bringing another library in, or depending on https://reactnavigation.org/ (even if that is what we probably should do in the long run), but something which could be done in minimal effort and still solve the imperative approach which we are unfortunately currently fielding all over the app.

The case with 2-event dispatch described by @goranjovic can be currently "solved" by overloading status-im.ui.screens.navigation/preload-data! multimethod for particular view id, so in the case above :discover-search-tags. The [:set :discover-search-tags [tag]] event can be replaced by doing the equivalent db operation in the multimethod, and as the multimethod is called in the :navigate-to event handler as an normal function (or rather, interceptor in the interceptor chain, but it's still synchronous call), it would actually satisfy the 3 points mentioned. But, I think it's still not the correct approach because of the following reasons:

• enrich middleware (that's how preload-data!) is expected to be pure function taking :db and returning new :db, what if we need to return some other coeffects/effects as well, as it's the case of preload-data! implementation for the :chat view ? Then we are forced to leave functional paradigm do again do imperative stuff, calling dispatch, saving/retrieving data from realm, etc.
• although defining this preloading through multimethods may look convenient, I think it's actually obfuscating things and instead of simple following the event chain when investigating what's happening, you have to hunt for preload-data! implementation, whenever you want to understand what's happening when [:navigate-to some-view] is executed. Multimethods are great for many things, mostly when you want to provide open dispatch and ability to extend functionality of your library to end-users who do not control the source code, but that's really not the case here.

So what I think we should do instead is to simply encapsulate navigation logic in couple of public (pure) functions in the status-im.ui.screens.navigation namespace, after all, it's just about changing the navigation-stack/modal/tabs data correctly in app-db. We will also leave default event-handlers there, so if as result of some action, user needs to be just navigated to particular view without any other data change or effect, event [:navigate-to some-view] could be fired just as before. But, when additional action is needed, like in the example of navigating to :discover-search-tags, we will just define new event for that:

(handlers/register-handler-db
  :navigate-to-discover-search-tags
  (fb [db [_ tag]]
     (-> db 
         (navigation/navigate-to :discover-search-tags)
         (assoc :discover-search-tags [tag]))))

Of course, instead of register-handler-db, we could use register-handler-fx if other coeffects/effects other then changing the db are needed.

[goranjovic]

In any case, let's hold on this one. We can probably live with it for several more weeks at least.