status-im / status-mobile

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

Chore: share `navigate-back` function between namespaces #19971

Open seanstrom opened 4 months ago

seanstrom commented 4 months ago

Problem

Summary

Anonymous Functions

Usages #(rf/dispatch [:navigate-back]) without a use-callback or use-memo can lead to more re-renders because of the anonymous function being passed as a prop which invalidates the React shouldComponentUpdate (aka React.memo) logic used by Reagent.

It's also worth mentioning that using use-callback or use-memo would be unnecessary since the expression #(rf/dispatch [:navigate-back]) does not use any variables or scope from a component. Which means it can be safely refactored into a static function inside the component namespace.

This means usages of #(rf/dispatch [:navigate-back]) would become (defn navigate-back [] ...) in practice.

Static functions

Using static functions is preferred because they create stable function references for components to pass around as props. Which leads to potentially less unnecessary re-renders because we're always passing the same function reference around.

However, defining many (defn navigate-back [] ...) functions in many different namespaces can have other drawbacks. For example, if we want refactor the app's naming of :navigate-back to something like :navigation/back or ::navigation/back, we'll need to go to each local definition of (defn navigate-back [] ...) and update the keywords.

Ideally, we would have a way to share the definition of (defn navigate-back [] ...) so that we can improve maintenance of the code and make refactors simpler. For example, if we share a static function from a namespace, we can easily find all the usages of the function with familiar IDE tooling. We can also easily rename the function and the event keyword in one/two places, which is much easier than needing to find all usages of the :navigate-back keyword being passed to a dispatch function.

Implementation

One potential solution is to revise the code to share a navigate-back function from a navigation or utility namespace. This was already being done from an events namespace here: https://github.com/status-im/status-mobile/blob/4f0a49f7bff32ed43ae16562da95f402c59e64f2/src/status_im/navigation/events.cljs#L47-L50

But I'm unsure whether we want more namespaces depending on the status-im.navigation.events namespace. If possible, we would decide whether to use this namespace or define a separate namespace for common dispatch functions. Though the downside to creating a separate namespace is that renaming the event name requires us to change it in two places.

Parveshdhull commented 4 months ago

If possible, we would decide whether to use this namespace or define a separate namespace for common dispatch functions

Thank you @seanstrom for creating this issue. As navigation.events is namespace for navigation related events, I think having a separate namespace will be better. In that way we also prevent cyclic dependency issue that might occur for events namespace.

Parveshdhull commented 3 months ago

Update:

https://github.com/status-im/status-mobile/pull/20668#discussion_r1666974223

https://github.com/status-im/status-mobile/pull/20671/files#diff-e2501e3768e34da38eb7b9a2d5da1d3d3df4cf3b9b6b5f86e140b76ec91ba00c

image