status-mobile/doc/new-guidelines.md

921 lines
26 KiB
Markdown
Raw Normal View History

# Code Style Guidelines
2022-11-14 10:15:49 +00:00
> [!IMPORTANT]
> The goal of this document is to help all contributors (core and external) to
> write code in _unison_ and help establish good practices that serve the Status
> Mobile contributors well.
2022-11-14 10:15:49 +00:00
We don't want to turn this document into an exhaustive list of rules to be
followed that nobody will read. As much as possible, we'll try to document only
what we consider important for Status Mobile. In other words, we don't want to
maintain a general Clojure convention/style guide, nor do we want to turn this
document into a long tutorial.
> [!WARNING]
> This is a **work in progress**, and not all conventions are properly
> implemented in the codebase yet. The project structure is also undergoing
> major changes, and it will take a considerable amount of time until we migrate
> the existing code to the new structure.
2022-11-14 10:15:49 +00:00
If you find out anything is outdated or missing, please, share with us or even
better, create a pull-request! 🤸
## Style guide
We follow the [Clojure Style
Guide](https://github.com/bbatsov/clojure-style-guide) and we use
[zprint](https://github.com/kkinnear/zprint) to format Clojure code. Running
`make lint-fix` should fix most formatting issues, but not all of them.
2022-11-14 10:15:49 +00:00
## Dos and don'ts
2023-04-24 12:38:11 +00:00
### Hiccup
Never use anonymous inline function in hiccup, this will lead to
reinitialization of component on each render of parent component.
2023-04-24 12:38:11 +00:00
```clojure
;; bad
(defn checkbox-view
[{:keys [size]}]
[rn/view
[(fn [] [rn/view])]])
;; good
(defn comp []
[rn/view])
(defn checkbox-view
[{:keys [size]}]
[rn/view
[comp]])
```
This mistake mostly happens with functional components.
2023-04-24 12:38:11 +00:00
```clojure
;; bad
(fn []
(let [atom (rf/sub [:sub])]
(fn []
[:f>
(fn []
[rn/text atom]
;; good
(defn f-comp [atom]
[rn/text atom])
2023-04-24 12:38:11 +00:00
(fn []
(let [atom (rf/sub [:sub])]
(fn []
[:f> f-comp atom])))
2023-04-24 12:38:11 +00:00
```
It's important to name functional components with `f-` prefix.
2023-04-24 12:38:11 +00:00
### Component props and API scheme to match Figma as closely as possible
Ideally, the prop names for components (particularly in quo Design System)
should match the Figma properties as best as possible. This makes it easier for
the developer using that component to configure it correctly for the screen it
is being used on and avoids unnecessary overwrites and adjustments being made.
#### Avoid unnecessarily grouping categories to reduce the number of props
For example in Figma if there is a component and it has the following variants:
|theme: "light" blur: "False"|theme: "dark" blur: "False"|theme: "light" blur: "True"|theme: "dark" blur: "True"|
|----------------------------|---------------------------|---------------------------|---------------------------|
| type :neutral label "ABC" | type :neutral label "ABC" | | |
| type :active label "ABC" | type :active label "ABC" | | |
| type :danger label "ABC" | type :danger label "ABC" | type :danger label "ABC" | type :danger label "ABC" |
```clojure
;; bad
"theme - :light or :dark
type - can be :neutral :active :danger :danger-blur"
(defn my-component [{:keys [theme type]} label])
;; good
"theme - :light or :dark
type - can be :neutral :active :danger
blur? - boolean
"
(defn my-component [{:keys [theme blur? type]} label])
```
Please note this is only for the external API of the component and there should
be no restriction of how the component manages its internal API as that will not
affect the developer using the component with the issues described above.
In some cases this is not always possible or does not make sense. However the
thought process should be how easy will it be for another developer to use this
component with the correct configuration given the screen designs for Figma.
#### Avoid unnecessarily renaming props
In general it can be helpful to avoid renaming props from their counterpart in
Figma.
For example if Figma has sizes `:small`, `:medium` and `:large`
```clojure
;; bad
":size - :little, :default or :big"
(defn my-component [{:keys [size]}])
;; good
":size - :small, :medium or :large"
(defn my-component [{:keys [size]}])
```
2022-11-14 10:15:49 +00:00
### Component styles
Prefer to define styles in a separate file named `style.cljs`, colocated with
the source file. For a real example, see
[src/quo/components/record_audio/record_audio/style.cljs](../src/quo/components/record_audio/record_audio/style.cljs).
2022-11-14 10:15:49 +00:00
```clojure
;; bad
(defn checkbox-view
[{:keys [size]}]
[rn/view
2022-11-14 10:15:49 +00:00
{:style {:width size
:height size
:border-radius 4
:justify-content :center
:align-items :center}}
[rn/view (do-something)]])
2022-11-14 10:15:49 +00:00
;; good
(defn checkbox-view
[{:keys [size]}]
[rn/view {:style (style/checkbox size)}
[rn/view (do-something)]])
2022-11-14 10:15:49 +00:00
```
### Always add styles inside the `:style` key
Although when compiling ReactNative for mobile some components are able work with
their styles in the top-level of the properties map, prefer to add them inside the
`:style` key in order to separate styles from properties:
```clojure
;; bad
[rn/button {:flex 1
:padding-vertical 10
:padding-horizontal 20
:on-press #(js/alert "Hi!")
:title "Button"}]
;; good
[rn/button {:style {:flex 1
:padding-vertical 10
:padding-horizontal 20}
:on-press #(js/alert "Hi!")
:title "Button"}]
;; better
;; (define them in a style ns & place them inside `:style` key)
[rn/button {:style (style/button)
:on-press #(js/alert "Hi!")
:title "Button"}]
```
2023-04-24 12:38:11 +00:00
Also its fine to keep one liner styles in view
```clojure
;; ok
[rn/view {:style {:flex 1 :padding-top 5}}]
```
### Don't define properties in styles ns
Properties must be set on view level
2023-04-24 12:38:11 +00:00
```clojure
;; bad
{:style {:position :absolute
:left 0
:right 0
:bottom 0}
:blur-amount 30
:blur-radius 25
:blur-type :transparent
:overlay-color :transparent}
;; good
{:position :absolute
:left 0
:right 0
:bottom 0}
```
### Animated styles in the style file
```clojure
;; bad
(defn circle
[]
(let [opacity (reanimated/use-shared-value 1)]
[reanimated/view {:style [{:opacity opacity}
style/circle-container]}]))
;; good
(defn circle
[]
(let [opacity (reanimated/use-shared-value 1)]
[reanimated/view {:style (style/circle-container opacity)}]))
```
### Pass a vector of styles to Reanimated view
Prefer to pass a vector of styles to `react-native.reanimated/view` `:style`
prop instead of using `apply-animations-to-style` directly. For more details, check out Reanimated docs about [inline
styles](https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/your-first-animation/#defining-a-shared-value)
and [useAnimatedStyle](https://docs.swmansion.com/react-native-reanimated/docs/fundamentals/animating-styles-and-props/#animating-styles).
```clojure
(defn f-view []
(let [scroll-x (reanimated/use-shared-value 0)
opacity (reanimated/interpolate scroll-x [0 45 50] [1 1 0])]
[reanimated/view
;; bad
{:style (reanimated/apply-animations-to-style
{:opacity opacity
:transform [{:translate-x scroll-x}]}
{:flex-direction :row})}
;; good
{:style [{:opacity opacity
:transform [{:translate-x scroll-x}]}
{:flex-direction :row}]}
;; other valid and good variants
{:style [{:opacity opacity}
{:transform [{:translate-x scroll-x}]}
{:flex-direction :row}]}
{:style {:opacity opacity
:transform [{:translate-x scroll-x}]
:flex-direction :row}}]))
```
2023-01-31 12:06:29 +00:00
### Don't use percents to define width/height
In ReactNative, all layouts use the [flexbox
model](https://reactnative.dev/docs/flexbox), so percentages are unnecessary the
vast majority of the time, don't use them. Check out this great [interactive
flexbox guide](https://www.joshwcomeau.com/css/interactive-guide-to-flexbox/) by
Joshua Comeau.
2023-01-31 12:06:29 +00:00
```clojure
;; bad
[rn/view {:style {:width "80%"}}]
2023-01-31 12:06:29 +00:00
;; good
[rn/view {:style {:padding-horizontal 20}}]
```
2023-01-31 12:06:29 +00:00
### Use a question mark to convey the value is a boolean
The Clojure Style Guide suggests using a question mark only in [predicate
functions](https://guide.clojure.style/#naming-predicates), but nothing is
mentioned about other symbols and keywords. We prefer to extend the convention
to all boolean references.
```clojure
;; bad
(let [is-open? true] ...)
(def flag-is-enabled false)
;; good
(let [open? true] ...)
(def flag-enabled? false)
```
And for keywords too:
```clojure
;; bad
[some-component {:logged-in true}]
;; good
[some-component {:logged-in? true}]
```
### Styles def vs defn
2022-11-28 12:58:36 +00:00
Always use `def` over `defn` if there are no dynamic values. This helps cut the
cost of function calls.
2022-11-28 12:58:36 +00:00
```clojure
;; bad
(defn title-column []
{:height 56})
;; good
(def title-column
{:height 56})
```
```clojure
;; bad
(def community-card
{:background-color (colors/theme-colors colors/white colors/neutral-90)})
;; good
(defn community-card []
{:background-color (colors/theme-colors colors/white colors/neutral-90)})
```
### Custom Colors
The Status designs have a lot of customization of user and group colors. For
consistency it is best to use `customization-color` as the prop key on pages and
components. This will help easily identify what pages and components in the
application are using customized colors.
```clojure
;; bad
(defn community-card [{keys [custom-color]}]
...)
;; good
(defn community-card [{keys [customization-color]}]
...)
```
2022-11-14 10:15:49 +00:00
### Using TODOs comments
_TODO_ comments are used extensively in the codebase, but prefer to use them
only when strictly necessary and when an issue is not enough to track the work
left to be done.
These are all good examples:
```clojure
;; TODO(@username): <message>
;; TODO(@username): <message>, <issue URL>
;; TODO(YYYY-MM-DD): <message>
;; TODO(@username,YYYY-MM-DD): <message>
```
### Subscription names and event names
Always register events and subscriptions using a meaningful namespace, but don't
namespace them with `::`. We understand it's a controversial decision because
there are both pros and cons to such practice.
Whenever appropriate, it's also recommended to use _fake_ namespaces to convey
more knowledge in the keyword about which bounded context (domain) it refers to.
You may also use dots to convey hierarchical structures.
```clojure
;; bad
;; Don't use real namespaced keywords.
(re-frame/reg-sub
::profile-pictures-visibility
:<- [:multiaccount]
(fn [multiaccount]
(:profile-pictures-visibility multiaccount)))
;; good
;; Uses a fake namespaced keyword.
(re-frame/reg-sub
:profile/pictures-visibility
:<- [:multiaccount]
(fn [multiaccount]
(:profile-pictures-visibility multiaccount)))
;; better
;; Uses a fake namespaced keyword with a parent namespace (multiaccount).
(re-frame/reg-sub
:multiaccount.profile/pictures-visibility
:<- [:multiaccount]
(fn [multiaccount]
(:profile-pictures-visibility multiaccount)))
```
### Declaring view components
Use the simple `defn` to declare components. Don't use `utils.views/defview` and
`utils.views/letsubs`.
```clojure
;; bad
(utils.views/defview browser []
(utils.views/letsubs [window-width [:dimensions/window-width]]
(do-something window-width)))
;; good
(defn browser []
(let [window-width (rf/sub [:dimensions/window-width])]
(do-something window-width)))
```
### Use `[]` instead of `()` in Reagent components
- The `()` version [does NOT work with Form-2 and
Form-3](https://github.com/reagent-project/reagent/blob/master/doc/UsingSquareBracketsInsteadOfParens.md#a-further-significant-why)
components.
- Components defined with `[]` will be [more efficient at re-render
time](https://github.com/reagent-project/reagent/blob/master/doc/UsingSquareBracketsInsteadOfParens.md#which-and-why)
because they're interpreted by Reagent and transformed into distinct React
components, with their own lifecycle.
```clojure
;; bad
[rn/view
(message-card message)]
;; good
[rn/view
[message-card message]]
```
2022-11-14 10:15:49 +00:00
### Using re-frame subscriptions and dispatching events
Use the `utils.re-frame` namespace instead of `re-frame.core` to subscribe and
dispatch.
```clojure
;; bad
(ns my-namespace
(:require [re-frame.core :as rf]))
(let [username @(rf/subscribe [:username])]
[pressable/pressable {:on-press #(rf/dispatch [:do-something])}
[rn/view
(str "Hello " username)]])
;; good
(ns my-namespace
(:require [utils.re-frame :as rf]))
(let [username (rf/sub [:username])]
[pressable/pressable {:on-press #(rf/dispatch [:do-something])}
[rn/view
(str "Hello " username)]])
```
Make json-rpc/call effect more in line with re-frame standards (#15936) Changes effect :json/rpc-call to accept on-success and on-error parameters either as functions OR as re-frame event vectors. The changes are 100% backwards compatible. ## Why? Re-frame is very explicit in its documentation and its architecture, saying that event handlers should be pure. Calling re-frame.core/dispatch in event handlers makes them sort of stateful. > So, you can "get away with it". But it ain't pure. > > -- https://day8.github.io/re-frame/EffectfulHandlers/#90-solution In status-mobile, arguably one of our most important effects (not to be confused with event handlers) is :json-rpc/call, but at the moment, the on-success and on-error values are expected to be stateful functions (e.g. usually used for logging and dispatching subsequent events). This creates two important problems: 1. The value returned by event handlers is more opaque and cannot be easily inspected (for example using tap>, log/debug or just println). If we try to inspect or log them, on-success and on-error will be printed as #object[Function]. 2. Testing event handlers using :json-rpc/call becomes an exercise of frustration, because we can't simply compare the results of the event handler with a good expected value, which is one of the big selling points of testing pure functions. ### The testability of event handlers > For any re-frame app, there's three things to test: > > - Event Handlers - most of your testing focus will be here because this is > where most of the logic lives > - Subscription Handlers - often not a lot to test here. Only Layer 3 > subscriptions need testing. > - View functions - I don't tend to write tests for views. > > -- https://day8.github.io/re-frame/Testing/#what-to-test So re-frame is saying event handlers should be pure, and that event handlers should be tested. In order to achieve the divine simplicity of testing event handlers as pure functions, we need to make :json-rpc/call also accept on-success and on-error as event vectors. Good news is that there is a known pattern to solve this problem, e.g. used by the library https://github.com/Day8/re-frame-http-fx. The pattern is simple once we see it: behind the scenes, :json-rpc/call conj'es the results of the RPC call into the event vectors on-success and on-error, and :json-rpc/call dispatches the events itself. This eliminates the need for the stateful dispatch call in event handlers.
2023-05-18 18:56:10 +00:00
### Registering effects
When registering re-frame effects (`reg-fx`), prefer to expose a data-only
interface because that will allow event handlers to stay pure.
For instance, if an effect needs a `on-success` callback, allow it to receive a
*re-frame event vector*. This approach is used by us in the [json-rpc/call
effect](src/status_im2/common/json_rpc/events.cljs), but also by third-party
effects, such as https://github.com/Day8/re-frame-http-fx. For the complete
rationale, see [PR #15936](https://github.com/status-im/status-mobile/pull/15936).
### Using the effect `:json-rpc/call`
Prefer the pure version of `:json-rpc/call` (no callbacks).
```clojure
;; not as good
(rf/defn accept-contact-request
{:events [:activity-center.contact-requests/accept]}
[_ contact-id]
{:json-rpc/call
[{:method "wakuext_acceptContactRequest"
:params [{:id contact-id}]
:on-success #(rf/dispatch [:sanitize-messages-and-process-response %])
:on-error #(rf/dispatch [:activity-center.contact-requests/accept-error contact-id %])}]})
;; better
(rf/defn accept-contact-request
{:events [:activity-center.contact-requests/accept]}
[_ contact-id]
{:json-rpc/call
[{:method "wakuext_acceptContactRequest"
:params [{:id contact-id}]
:on-success [:sanitize-messages-and-process-response]
:on-error [:activity-center.contact-requests/accept-error contact-id]}]})
```
2022-11-14 10:15:49 +00:00
### Registering event handlers
Register events with `utils.re-frame/reg-event-fx` and follow [re-frame's best
Migrate away from rf/defn and rf/merge (first step) (#17451) This commit shows how to move away from rf/defn https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.clj#L1-L90 & rf/merge https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.cljs#L39-L85 and why we should do it. ## Problems Before jumping to solutions, let's understand the problems first, in no order of importance. ### Problem 1: Cyclic dependencies If you ever tried to move event handlers or the functions used inside them to different files in status-mobile, you probably stumbled in cyclic dependency errors. When an event is registered in re-frame, it is globally available for any other place to dispatch. The dispatch mechanism relies on globally unique keywords, the so called event IDs, e.g. :chat/mute-successfully. This means that event namespaces don't need to require other event namespaces, just like you don't need to require subscription namespaces in views. rf/merge increases the likelihood of cyclic dependencies because they force event namespaces to require each other. Although not as common, this happened a few times with devs in the team and it can be a big hassle to fix if you are unlucky. It is a problem we should not have in the first place (at least not as easily). ### Problem 2: We are not linting thousands of lines of code The linter (clj-kondo) is incapable of understanding the rf/defn macro. In theory, we could teach clj-kondo what the macro produces. I tried this, but gave up after a few tries. This is a big deal, clj-kondo can catch many issues and will continue to catch more as it continue to evolve. It's hard to precisely count how many lines are affected, but `find src/ -type f -name 'events.cljs' -exec wc -l {} +` gives us more than 4k LOC. ### Problem 3: Blocking RN's UI thread for too long Re-frame has a routing mechanism to manage events. When an event is dispatched, it is enqueued and scheduled to run some time later (very soon). This process is asynchronous and is optimized in such a way as to balance responsiveness vs the time to empty the queue. >[...] when processing events, one after the other, do ALL the currently queued >events. Don't stop. Don't yield to the browser. Hog that CPU. > >[...] but if any new events are dispatched during this cycle of processing, >don't do them immediately. Leave them queued. > >-- https://github.com/day8/re-frame/blob/master/src/re_frame/router.cljc#L8-L60 Decisions were made (way back in 2017) to reduce the number of registered re-frame events and, more importantly, to coalesce events into bigger ones with the rf/merge pattern. I tried to find evidence of real problems that were trying to be solved, but my understanding is that decisions were largely based on personal architectural preferences. Fast-forward to 2023, and we are in a situation where we have many heavy events that process a LOT of stuff in one go using rf/merge, thus blocking the UI thread longer than we should. See, for example, [status-im2.contexts.profile.login.events/login-existing-profile](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L69), [status-im2.contexts.profile.login.events/get-chats-callback](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L98), and many others. The following excerpt was generally used to justify the idea that coalescing events would make the app perform better. > We will reduce the the amount of subscription re-computations, as for each > distinct action, :db effect will be produced and swapped into app-db only once > > -- https://github.com/status-im/swarms/issues/31#issuecomment-346345981 This is in fact incorrect. Re-frame, ever since 2015 (so before the original discussions in 2017) uses a concept of batching to process events, which means subscriptions won't re-run after every dispatched event, and thus components won't re-render either. Re-frame is smarter than that. > groups of events queued up will be handled in a batch, one after the other, > without yielding to the browser (previously re-frame yielded to the browser > before every single event). > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2015.md#050--2015-11-5 Here's a practical example you can try in a shadow-cljs :mobile REPL to see the batching behavior in practice. ```clojure ;; A dummy event that toggles between DEBUG and INFO levels. (re-frame/reg-event-fx :dummy-event (fn [{:keys [db]}] {:db (update-in db [:profile/profile :log-level] (fn [level] (if (= "DEBUG" level) "INFO" "DEBUG")))})) (def timer (js/setInterval #(re-frame/dispatch [:dummy-event]) 50)) ;; 1. In component status-im.ui.screens.advanced-settings.views/advanced-settings, ;; add a print call to see when it's re-rendered by Reagent because the ;; subscription :log-level/current-log-level will be affected by our dummy event. ;; ;; 2. Also add a print call to the subscription :log-level/current-log-level to ;; see that the subscription does NOT re-run on every dispatch. ;; Remember to eval this expression to cancel the timer. (js/clearInterval timer) ``` If you run the above timer with 50ms interval, you'll see a fair amount of batching happening. You can infer that's the case because you'll see way less than 20 print statements per second (so way less than 20 recomputations of the subscription, which is the max theoretical limit). When the interval is reduced even more, to say 10ms (to simulate lots of dispatches in a row), sometimes you don't see a single recomputation in a 5s window because re-frame is too busy processing events. This shows just how critical it is to have event handlers finishing as soon as possible to relinquish control back to the UI thread, otherwise responsiveness is affected. It also shows that too many dispatches in a row can be bad, just as big event handlers would block the batch for too long. You see here that dispatching events in succession does NOT cause needless re-computations. Of course there's an overhead of using re-frame.core/dispatch instead of calling a Clojure function, but the trade-off is clearly documented: the more we process in a single event, the less responsive the app may be because re-frame won't be able to relinquish control back to the UI thread. The total time to process the batch increases, but re-frame can't stop in the middle compared to when different dispatches are used. Thus, I believe this rf/merge pattern is harmful as a default practice in an environment such as ours, where it's desirable end-users feel a snappy RN app. I actually firmly believe we can improve the app's responsiveness by not coalescing events by default. We're also preventing status-mobile from taking the most advantage from future improvements in re-frame's scheduler. I can totally see us experimenting with other algorithms in the scheduler to best fit our needs. We should not blindly reduce the number of events as stated here https://github.com/status-im/status-mobile/pull/2634#discussion_r155243127. Solution: only coalesce events into one pile when it's strictly desirable to atomically update the app db to avoid inconsistencies, otherwise, let the re-frame scheduler do its job by using fx, not rf/merge. When needed, embrace *eventual app db consistency* as a way to achieve lower UI latency, i.e. write fast and short events, intentionally use :dispatch-later or other timing effects to bend the re-frame's scheduler to your will. There's another argument in favor of using something like rf/merge which I would like to deconstruct. rf/merge gives us a way to reuse computations from different events, which is nice. The thing here is that we don't need rf/merge or re-frame to reuse functions across namespaces. rf/merge complects re-frame with the need to reuse transformations. Instead, the solution is as trivial as it gets, reuse app db "transformers" across events by extracting the logic to data store namespaces (src/status_im/data_store). This solution has the added benefit of not causing cyclic dependency errors. ### Problem 4: Clojure's language server doesn't understand code under rf/defn Nowadays, many (if not most) Clojure devs rely on the Clojure Language Server https://github.com/clojure-lsp/clojure-lsp to be more effective. It is an invaluable tool, but it doesn't work well with the macro rf/defn, and it's a constant source of frustration when working in event namespaces. Renaming symbols inside the macro don't work, finding references, jumping to local bindings, etc. Solution: don't use rf/defn, instead use re-frame's reg-event-fx function and clojure-lsp will understand all the code inside event handlers. ### Problem 5: Unit tests for events need to "test the world" Re-frame's author strongly recommends testing events that contain non-trivial data transformations, and we do have many in status-mobile (note: let's not confuse with integration tests in status_im/integration_test.cljs). That, and non-trivial layer-3 subscriptions should be covered too. The reasoning is that if we have a well developed and tested state layer, many UI bugs can be prevented as the software evolves, since the UI is partially or greatly derived from the global state. See re-frame: What to Test? https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Testing.md#what-to-test. See PR Introduce subscription tests https://github.com/status-im/status-mobile/pull/14472, where I share more details about re-frame's testing practices. When we use rf/merge, we make unit testing events a perennial source of frustration because too many responsibilities are aggregated in a single event. Unfortunately, we don't have many devs in the team that attempted to write unit tests for events to confirm my claim, but no worries, let's dive into a real example. In a unit test for an event, we want to test that, given a cofx and args, the event handler returns the expected map of effects with the correct values (usually db transformations). Let's assume we need to test the following event. The code is still using the combo rf/defn & rf/merge. ```clojure (rf/defn accept-notification-success {:events [:activity-center.notifications/accept-success]} [{:keys [db] :as cofx} notification-id {:keys [chats]}] (when-let [notification (get-notification db notification-id)] (rf/merge cofx (chat.events/ensure-chats (map data-store.chats/<-rpc chats)) (notifications-reconcile [(assoc notification :read true :accepted true)])))) ``` As you can see, we're "rf/merging" two other functions, namely ensure-chats and notifications-reconcile. In fact, ensure-chats is not registered in re-frame, but it's 99% defined as if it's one because it needs to be "mergeable" according to the rules of rf/merge. Both of these "events" are quite complicated under the hood and should be unit tested on their own. Now here goes the unit test. Don't worry about the details, except for the expected output. ```clojure (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} expected {:db {:activity-center {:filter {:type 0 :status :all} :notifications [notif-2-accepted notif-1]} :chats {} :chats-home-list nil} ;; *** HERE *** :dispatch-n [[:activity-center.notifications/fetch-unread-count] [:activity-center.notifications/fetch-pending-contact-requests]]} actual (events/accept-notification-success cofx (:id notif-2) nil)] (is (= expected actual))))) ``` Notice the map has a :dispatch-n effect and other stuff inside of it that are not the responsibility of the event under test to care about. This happens because rf/merge forces the event handler to compute/call everything in one go. And things get MUCH worse when you want to test an event A that uses rf/merge, but A calls other events B and C that also use rf/merge (e.g. event :profile.login/get-chats-callback). At that point you flip the table in horror 😱, but testing events and maintaining them should be trivial. Solution: Use re-frame's `fx` effect. Here's the improved implementation and its accompanying test. ```clojure (defn accept-notification-success [{:keys [db]} [notification-id {:keys [chats]}]] (when-let [notification (get-notification db notification-id)] (let [new-notifications [(assoc notification :read true :accepted true)]] {:fx [[:dispatch [:chat/ensure-chats (map data-store.chats/<-rpc chats)]] [:dispatch [:activity-center.notifications/reconcile new-notifications]]]}))) (re-frame/reg-event-fx :activity-center.notifications/accept-success accept-notification-success) (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} ;; *** HERE *** expected {:fx [[:dispatch [:chat/ensure-chats []]] [:dispatch [:activity-center.notifications/reconcile [notif-2-accepted]]]]} actual (events/accept-notification-success cofx [(:id notif-2) nil])] (is (= expected actual))))) ``` Notice how the test expectation is NOT verifying what other events do (it's actually "impossible" now). Using fx completely decouples events and makes testing them a joy again. ### Problem 6: Unordered effects status-mobile still uses the legacy way to describe the effects map, which has the problem that their order is unpredictable. > Prior to v1.1.0, the answer is: no guarantees were provided about ordering. > Actual order is an implementation detail upon which you should not rely. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects > In fact, with v1.1.0 best practice changed to event handlers should only > return two effects :db and :fx, in which case :db was always done first and > then :fx, and within :fx the ordering is sequential. This new approach is more > about making it easier to compose event handlers from many smaller functions, > but more specificity around ordering was a consequence. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects ### Problem 7: Usage of deprecated effect dispatch-n We have 35 usages, the majority in new code using dispatch-n, which has been officially deprecated in favor of multiple dispatch tuples in fx. See https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/api-builtin-effects.md#L114 ### Problem 8: Complexity 🧙‍♂️ Have you ever tried to understand and/or explain how rf/merge and rf/defn work? They have their fare share of complexity and have tripped up many contributors. This is not ideal if we want to create a project where contributors can learn re-frame as quickly as possible. Re-frame is already complicated enough to grasp for many, the added abstractions should be valuable enough to justify. Interestingly, rf/merge is a stateful function, and although this is not a problem in practice, it is partially violating re-frame's spirit of only using pure functions inside event handlers. ### Problem 9: Using a wrapping macro rf/defn instead of global interceptors When rf/defn was created inside status-mobile, re-frame didn't have global interceptors yet (which were introduced 3+ years ago). We no longer have this limitation after we upgraded our old re-frame version in PR https://github.com/status-im/status-mobile/pull/15997. Global interceptors are a simple and functional abstraction to specify functions that should run on every event, for example, for debugging during development, logging, etc. This PR already shows this is possible by removing the wrapping function utils.re-frame/register-handler-fx without causing any breakage. ## Conclusion By embracing re-frame's best practices for describing effects https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/FAQs/BestPractice.md#use-the-fx-effect, we can solve long standing issues that affect every contributor at different levels and bring the following benefits: - Simplify the codebase. - Bring back the DX we all deserve, i.e. Clojure Language Server and clj-kondo fully working in event namespaces. - Greatly facilitate the testability of events. - Give devs more flexibility to make the app more responsive, because the new default would not coalesce events, which in turn, would block the UI thread for shorter periods of time. At least that's the theory, but exceptions will be found. The actions to achieve those benefits are: - Don't use the macro approach, replace rf/defn with re-frame.core/reg-event-fx. - Don't use rf/merge, simply use re-frame's built-in effect :fx. - Don't call event handlers as normal functions, just as we don't directly call subscription handlers. Use re-frame's built-in effect :fx. ## How do we refactor the remainder of the code? Some numbers first: - There are 228 events defined with rf/defn in src/status-im2/. - There are 34 usages of rf/merge in src/status_im2/. ## Resources - Release notes where fx was introduced in re-frame: https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2020.md#110-2020-08-24
2023-10-05 19:11:45 +00:00
practice](https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/FAQs/BestPractice.md#use-the-fx-effect)
so use only `:db` and `:fx` effects. `utils.re-frame/merge` and `utils.re-frame/defn` are deprecated and should not be
Migrate away from rf/defn and rf/merge (first step) (#17451) This commit shows how to move away from rf/defn https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.clj#L1-L90 & rf/merge https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.cljs#L39-L85 and why we should do it. ## Problems Before jumping to solutions, let's understand the problems first, in no order of importance. ### Problem 1: Cyclic dependencies If you ever tried to move event handlers or the functions used inside them to different files in status-mobile, you probably stumbled in cyclic dependency errors. When an event is registered in re-frame, it is globally available for any other place to dispatch. The dispatch mechanism relies on globally unique keywords, the so called event IDs, e.g. :chat/mute-successfully. This means that event namespaces don't need to require other event namespaces, just like you don't need to require subscription namespaces in views. rf/merge increases the likelihood of cyclic dependencies because they force event namespaces to require each other. Although not as common, this happened a few times with devs in the team and it can be a big hassle to fix if you are unlucky. It is a problem we should not have in the first place (at least not as easily). ### Problem 2: We are not linting thousands of lines of code The linter (clj-kondo) is incapable of understanding the rf/defn macro. In theory, we could teach clj-kondo what the macro produces. I tried this, but gave up after a few tries. This is a big deal, clj-kondo can catch many issues and will continue to catch more as it continue to evolve. It's hard to precisely count how many lines are affected, but `find src/ -type f -name 'events.cljs' -exec wc -l {} +` gives us more than 4k LOC. ### Problem 3: Blocking RN's UI thread for too long Re-frame has a routing mechanism to manage events. When an event is dispatched, it is enqueued and scheduled to run some time later (very soon). This process is asynchronous and is optimized in such a way as to balance responsiveness vs the time to empty the queue. >[...] when processing events, one after the other, do ALL the currently queued >events. Don't stop. Don't yield to the browser. Hog that CPU. > >[...] but if any new events are dispatched during this cycle of processing, >don't do them immediately. Leave them queued. > >-- https://github.com/day8/re-frame/blob/master/src/re_frame/router.cljc#L8-L60 Decisions were made (way back in 2017) to reduce the number of registered re-frame events and, more importantly, to coalesce events into bigger ones with the rf/merge pattern. I tried to find evidence of real problems that were trying to be solved, but my understanding is that decisions were largely based on personal architectural preferences. Fast-forward to 2023, and we are in a situation where we have many heavy events that process a LOT of stuff in one go using rf/merge, thus blocking the UI thread longer than we should. See, for example, [status-im2.contexts.profile.login.events/login-existing-profile](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L69), [status-im2.contexts.profile.login.events/get-chats-callback](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L98), and many others. The following excerpt was generally used to justify the idea that coalescing events would make the app perform better. > We will reduce the the amount of subscription re-computations, as for each > distinct action, :db effect will be produced and swapped into app-db only once > > -- https://github.com/status-im/swarms/issues/31#issuecomment-346345981 This is in fact incorrect. Re-frame, ever since 2015 (so before the original discussions in 2017) uses a concept of batching to process events, which means subscriptions won't re-run after every dispatched event, and thus components won't re-render either. Re-frame is smarter than that. > groups of events queued up will be handled in a batch, one after the other, > without yielding to the browser (previously re-frame yielded to the browser > before every single event). > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2015.md#050--2015-11-5 Here's a practical example you can try in a shadow-cljs :mobile REPL to see the batching behavior in practice. ```clojure ;; A dummy event that toggles between DEBUG and INFO levels. (re-frame/reg-event-fx :dummy-event (fn [{:keys [db]}] {:db (update-in db [:profile/profile :log-level] (fn [level] (if (= "DEBUG" level) "INFO" "DEBUG")))})) (def timer (js/setInterval #(re-frame/dispatch [:dummy-event]) 50)) ;; 1. In component status-im.ui.screens.advanced-settings.views/advanced-settings, ;; add a print call to see when it's re-rendered by Reagent because the ;; subscription :log-level/current-log-level will be affected by our dummy event. ;; ;; 2. Also add a print call to the subscription :log-level/current-log-level to ;; see that the subscription does NOT re-run on every dispatch. ;; Remember to eval this expression to cancel the timer. (js/clearInterval timer) ``` If you run the above timer with 50ms interval, you'll see a fair amount of batching happening. You can infer that's the case because you'll see way less than 20 print statements per second (so way less than 20 recomputations of the subscription, which is the max theoretical limit). When the interval is reduced even more, to say 10ms (to simulate lots of dispatches in a row), sometimes you don't see a single recomputation in a 5s window because re-frame is too busy processing events. This shows just how critical it is to have event handlers finishing as soon as possible to relinquish control back to the UI thread, otherwise responsiveness is affected. It also shows that too many dispatches in a row can be bad, just as big event handlers would block the batch for too long. You see here that dispatching events in succession does NOT cause needless re-computations. Of course there's an overhead of using re-frame.core/dispatch instead of calling a Clojure function, but the trade-off is clearly documented: the more we process in a single event, the less responsive the app may be because re-frame won't be able to relinquish control back to the UI thread. The total time to process the batch increases, but re-frame can't stop in the middle compared to when different dispatches are used. Thus, I believe this rf/merge pattern is harmful as a default practice in an environment such as ours, where it's desirable end-users feel a snappy RN app. I actually firmly believe we can improve the app's responsiveness by not coalescing events by default. We're also preventing status-mobile from taking the most advantage from future improvements in re-frame's scheduler. I can totally see us experimenting with other algorithms in the scheduler to best fit our needs. We should not blindly reduce the number of events as stated here https://github.com/status-im/status-mobile/pull/2634#discussion_r155243127. Solution: only coalesce events into one pile when it's strictly desirable to atomically update the app db to avoid inconsistencies, otherwise, let the re-frame scheduler do its job by using fx, not rf/merge. When needed, embrace *eventual app db consistency* as a way to achieve lower UI latency, i.e. write fast and short events, intentionally use :dispatch-later or other timing effects to bend the re-frame's scheduler to your will. There's another argument in favor of using something like rf/merge which I would like to deconstruct. rf/merge gives us a way to reuse computations from different events, which is nice. The thing here is that we don't need rf/merge or re-frame to reuse functions across namespaces. rf/merge complects re-frame with the need to reuse transformations. Instead, the solution is as trivial as it gets, reuse app db "transformers" across events by extracting the logic to data store namespaces (src/status_im/data_store). This solution has the added benefit of not causing cyclic dependency errors. ### Problem 4: Clojure's language server doesn't understand code under rf/defn Nowadays, many (if not most) Clojure devs rely on the Clojure Language Server https://github.com/clojure-lsp/clojure-lsp to be more effective. It is an invaluable tool, but it doesn't work well with the macro rf/defn, and it's a constant source of frustration when working in event namespaces. Renaming symbols inside the macro don't work, finding references, jumping to local bindings, etc. Solution: don't use rf/defn, instead use re-frame's reg-event-fx function and clojure-lsp will understand all the code inside event handlers. ### Problem 5: Unit tests for events need to "test the world" Re-frame's author strongly recommends testing events that contain non-trivial data transformations, and we do have many in status-mobile (note: let's not confuse with integration tests in status_im/integration_test.cljs). That, and non-trivial layer-3 subscriptions should be covered too. The reasoning is that if we have a well developed and tested state layer, many UI bugs can be prevented as the software evolves, since the UI is partially or greatly derived from the global state. See re-frame: What to Test? https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Testing.md#what-to-test. See PR Introduce subscription tests https://github.com/status-im/status-mobile/pull/14472, where I share more details about re-frame's testing practices. When we use rf/merge, we make unit testing events a perennial source of frustration because too many responsibilities are aggregated in a single event. Unfortunately, we don't have many devs in the team that attempted to write unit tests for events to confirm my claim, but no worries, let's dive into a real example. In a unit test for an event, we want to test that, given a cofx and args, the event handler returns the expected map of effects with the correct values (usually db transformations). Let's assume we need to test the following event. The code is still using the combo rf/defn & rf/merge. ```clojure (rf/defn accept-notification-success {:events [:activity-center.notifications/accept-success]} [{:keys [db] :as cofx} notification-id {:keys [chats]}] (when-let [notification (get-notification db notification-id)] (rf/merge cofx (chat.events/ensure-chats (map data-store.chats/<-rpc chats)) (notifications-reconcile [(assoc notification :read true :accepted true)])))) ``` As you can see, we're "rf/merging" two other functions, namely ensure-chats and notifications-reconcile. In fact, ensure-chats is not registered in re-frame, but it's 99% defined as if it's one because it needs to be "mergeable" according to the rules of rf/merge. Both of these "events" are quite complicated under the hood and should be unit tested on their own. Now here goes the unit test. Don't worry about the details, except for the expected output. ```clojure (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} expected {:db {:activity-center {:filter {:type 0 :status :all} :notifications [notif-2-accepted notif-1]} :chats {} :chats-home-list nil} ;; *** HERE *** :dispatch-n [[:activity-center.notifications/fetch-unread-count] [:activity-center.notifications/fetch-pending-contact-requests]]} actual (events/accept-notification-success cofx (:id notif-2) nil)] (is (= expected actual))))) ``` Notice the map has a :dispatch-n effect and other stuff inside of it that are not the responsibility of the event under test to care about. This happens because rf/merge forces the event handler to compute/call everything in one go. And things get MUCH worse when you want to test an event A that uses rf/merge, but A calls other events B and C that also use rf/merge (e.g. event :profile.login/get-chats-callback). At that point you flip the table in horror 😱, but testing events and maintaining them should be trivial. Solution: Use re-frame's `fx` effect. Here's the improved implementation and its accompanying test. ```clojure (defn accept-notification-success [{:keys [db]} [notification-id {:keys [chats]}]] (when-let [notification (get-notification db notification-id)] (let [new-notifications [(assoc notification :read true :accepted true)]] {:fx [[:dispatch [:chat/ensure-chats (map data-store.chats/<-rpc chats)]] [:dispatch [:activity-center.notifications/reconcile new-notifications]]]}))) (re-frame/reg-event-fx :activity-center.notifications/accept-success accept-notification-success) (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} ;; *** HERE *** expected {:fx [[:dispatch [:chat/ensure-chats []]] [:dispatch [:activity-center.notifications/reconcile [notif-2-accepted]]]]} actual (events/accept-notification-success cofx [(:id notif-2) nil])] (is (= expected actual))))) ``` Notice how the test expectation is NOT verifying what other events do (it's actually "impossible" now). Using fx completely decouples events and makes testing them a joy again. ### Problem 6: Unordered effects status-mobile still uses the legacy way to describe the effects map, which has the problem that their order is unpredictable. > Prior to v1.1.0, the answer is: no guarantees were provided about ordering. > Actual order is an implementation detail upon which you should not rely. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects > In fact, with v1.1.0 best practice changed to event handlers should only > return two effects :db and :fx, in which case :db was always done first and > then :fx, and within :fx the ordering is sequential. This new approach is more > about making it easier to compose event handlers from many smaller functions, > but more specificity around ordering was a consequence. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects ### Problem 7: Usage of deprecated effect dispatch-n We have 35 usages, the majority in new code using dispatch-n, which has been officially deprecated in favor of multiple dispatch tuples in fx. See https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/api-builtin-effects.md#L114 ### Problem 8: Complexity 🧙‍♂️ Have you ever tried to understand and/or explain how rf/merge and rf/defn work? They have their fare share of complexity and have tripped up many contributors. This is not ideal if we want to create a project where contributors can learn re-frame as quickly as possible. Re-frame is already complicated enough to grasp for many, the added abstractions should be valuable enough to justify. Interestingly, rf/merge is a stateful function, and although this is not a problem in practice, it is partially violating re-frame's spirit of only using pure functions inside event handlers. ### Problem 9: Using a wrapping macro rf/defn instead of global interceptors When rf/defn was created inside status-mobile, re-frame didn't have global interceptors yet (which were introduced 3+ years ago). We no longer have this limitation after we upgraded our old re-frame version in PR https://github.com/status-im/status-mobile/pull/15997. Global interceptors are a simple and functional abstraction to specify functions that should run on every event, for example, for debugging during development, logging, etc. This PR already shows this is possible by removing the wrapping function utils.re-frame/register-handler-fx without causing any breakage. ## Conclusion By embracing re-frame's best practices for describing effects https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/FAQs/BestPractice.md#use-the-fx-effect, we can solve long standing issues that affect every contributor at different levels and bring the following benefits: - Simplify the codebase. - Bring back the DX we all deserve, i.e. Clojure Language Server and clj-kondo fully working in event namespaces. - Greatly facilitate the testability of events. - Give devs more flexibility to make the app more responsive, because the new default would not coalesce events, which in turn, would block the UI thread for shorter periods of time. At least that's the theory, but exceptions will be found. The actions to achieve those benefits are: - Don't use the macro approach, replace rf/defn with re-frame.core/reg-event-fx. - Don't use rf/merge, simply use re-frame's built-in effect :fx. - Don't call event handlers as normal functions, just as we don't directly call subscription handlers. Use re-frame's built-in effect :fx. ## How do we refactor the remainder of the code? Some numbers first: - There are 228 events defined with rf/defn in src/status-im2/. - There are 34 usages of rf/merge in src/status_im2/. ## Resources - Release notes where fx was introduced in re-frame: https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2020.md#110-2020-08-24
2023-10-05 19:11:45 +00:00
used in the new code in `src/status_im2/`. Don't use
2022-11-14 10:15:49 +00:00
`re-frame.core/reg-event-db`.
```clojure
;; bad
Migrate away from rf/defn and rf/merge (first step) (#17451) This commit shows how to move away from rf/defn https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.clj#L1-L90 & rf/merge https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.cljs#L39-L85 and why we should do it. ## Problems Before jumping to solutions, let's understand the problems first, in no order of importance. ### Problem 1: Cyclic dependencies If you ever tried to move event handlers or the functions used inside them to different files in status-mobile, you probably stumbled in cyclic dependency errors. When an event is registered in re-frame, it is globally available for any other place to dispatch. The dispatch mechanism relies on globally unique keywords, the so called event IDs, e.g. :chat/mute-successfully. This means that event namespaces don't need to require other event namespaces, just like you don't need to require subscription namespaces in views. rf/merge increases the likelihood of cyclic dependencies because they force event namespaces to require each other. Although not as common, this happened a few times with devs in the team and it can be a big hassle to fix if you are unlucky. It is a problem we should not have in the first place (at least not as easily). ### Problem 2: We are not linting thousands of lines of code The linter (clj-kondo) is incapable of understanding the rf/defn macro. In theory, we could teach clj-kondo what the macro produces. I tried this, but gave up after a few tries. This is a big deal, clj-kondo can catch many issues and will continue to catch more as it continue to evolve. It's hard to precisely count how many lines are affected, but `find src/ -type f -name 'events.cljs' -exec wc -l {} +` gives us more than 4k LOC. ### Problem 3: Blocking RN's UI thread for too long Re-frame has a routing mechanism to manage events. When an event is dispatched, it is enqueued and scheduled to run some time later (very soon). This process is asynchronous and is optimized in such a way as to balance responsiveness vs the time to empty the queue. >[...] when processing events, one after the other, do ALL the currently queued >events. Don't stop. Don't yield to the browser. Hog that CPU. > >[...] but if any new events are dispatched during this cycle of processing, >don't do them immediately. Leave them queued. > >-- https://github.com/day8/re-frame/blob/master/src/re_frame/router.cljc#L8-L60 Decisions were made (way back in 2017) to reduce the number of registered re-frame events and, more importantly, to coalesce events into bigger ones with the rf/merge pattern. I tried to find evidence of real problems that were trying to be solved, but my understanding is that decisions were largely based on personal architectural preferences. Fast-forward to 2023, and we are in a situation where we have many heavy events that process a LOT of stuff in one go using rf/merge, thus blocking the UI thread longer than we should. See, for example, [status-im2.contexts.profile.login.events/login-existing-profile](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L69), [status-im2.contexts.profile.login.events/get-chats-callback](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L98), and many others. The following excerpt was generally used to justify the idea that coalescing events would make the app perform better. > We will reduce the the amount of subscription re-computations, as for each > distinct action, :db effect will be produced and swapped into app-db only once > > -- https://github.com/status-im/swarms/issues/31#issuecomment-346345981 This is in fact incorrect. Re-frame, ever since 2015 (so before the original discussions in 2017) uses a concept of batching to process events, which means subscriptions won't re-run after every dispatched event, and thus components won't re-render either. Re-frame is smarter than that. > groups of events queued up will be handled in a batch, one after the other, > without yielding to the browser (previously re-frame yielded to the browser > before every single event). > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2015.md#050--2015-11-5 Here's a practical example you can try in a shadow-cljs :mobile REPL to see the batching behavior in practice. ```clojure ;; A dummy event that toggles between DEBUG and INFO levels. (re-frame/reg-event-fx :dummy-event (fn [{:keys [db]}] {:db (update-in db [:profile/profile :log-level] (fn [level] (if (= "DEBUG" level) "INFO" "DEBUG")))})) (def timer (js/setInterval #(re-frame/dispatch [:dummy-event]) 50)) ;; 1. In component status-im.ui.screens.advanced-settings.views/advanced-settings, ;; add a print call to see when it's re-rendered by Reagent because the ;; subscription :log-level/current-log-level will be affected by our dummy event. ;; ;; 2. Also add a print call to the subscription :log-level/current-log-level to ;; see that the subscription does NOT re-run on every dispatch. ;; Remember to eval this expression to cancel the timer. (js/clearInterval timer) ``` If you run the above timer with 50ms interval, you'll see a fair amount of batching happening. You can infer that's the case because you'll see way less than 20 print statements per second (so way less than 20 recomputations of the subscription, which is the max theoretical limit). When the interval is reduced even more, to say 10ms (to simulate lots of dispatches in a row), sometimes you don't see a single recomputation in a 5s window because re-frame is too busy processing events. This shows just how critical it is to have event handlers finishing as soon as possible to relinquish control back to the UI thread, otherwise responsiveness is affected. It also shows that too many dispatches in a row can be bad, just as big event handlers would block the batch for too long. You see here that dispatching events in succession does NOT cause needless re-computations. Of course there's an overhead of using re-frame.core/dispatch instead of calling a Clojure function, but the trade-off is clearly documented: the more we process in a single event, the less responsive the app may be because re-frame won't be able to relinquish control back to the UI thread. The total time to process the batch increases, but re-frame can't stop in the middle compared to when different dispatches are used. Thus, I believe this rf/merge pattern is harmful as a default practice in an environment such as ours, where it's desirable end-users feel a snappy RN app. I actually firmly believe we can improve the app's responsiveness by not coalescing events by default. We're also preventing status-mobile from taking the most advantage from future improvements in re-frame's scheduler. I can totally see us experimenting with other algorithms in the scheduler to best fit our needs. We should not blindly reduce the number of events as stated here https://github.com/status-im/status-mobile/pull/2634#discussion_r155243127. Solution: only coalesce events into one pile when it's strictly desirable to atomically update the app db to avoid inconsistencies, otherwise, let the re-frame scheduler do its job by using fx, not rf/merge. When needed, embrace *eventual app db consistency* as a way to achieve lower UI latency, i.e. write fast and short events, intentionally use :dispatch-later or other timing effects to bend the re-frame's scheduler to your will. There's another argument in favor of using something like rf/merge which I would like to deconstruct. rf/merge gives us a way to reuse computations from different events, which is nice. The thing here is that we don't need rf/merge or re-frame to reuse functions across namespaces. rf/merge complects re-frame with the need to reuse transformations. Instead, the solution is as trivial as it gets, reuse app db "transformers" across events by extracting the logic to data store namespaces (src/status_im/data_store). This solution has the added benefit of not causing cyclic dependency errors. ### Problem 4: Clojure's language server doesn't understand code under rf/defn Nowadays, many (if not most) Clojure devs rely on the Clojure Language Server https://github.com/clojure-lsp/clojure-lsp to be more effective. It is an invaluable tool, but it doesn't work well with the macro rf/defn, and it's a constant source of frustration when working in event namespaces. Renaming symbols inside the macro don't work, finding references, jumping to local bindings, etc. Solution: don't use rf/defn, instead use re-frame's reg-event-fx function and clojure-lsp will understand all the code inside event handlers. ### Problem 5: Unit tests for events need to "test the world" Re-frame's author strongly recommends testing events that contain non-trivial data transformations, and we do have many in status-mobile (note: let's not confuse with integration tests in status_im/integration_test.cljs). That, and non-trivial layer-3 subscriptions should be covered too. The reasoning is that if we have a well developed and tested state layer, many UI bugs can be prevented as the software evolves, since the UI is partially or greatly derived from the global state. See re-frame: What to Test? https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Testing.md#what-to-test. See PR Introduce subscription tests https://github.com/status-im/status-mobile/pull/14472, where I share more details about re-frame's testing practices. When we use rf/merge, we make unit testing events a perennial source of frustration because too many responsibilities are aggregated in a single event. Unfortunately, we don't have many devs in the team that attempted to write unit tests for events to confirm my claim, but no worries, let's dive into a real example. In a unit test for an event, we want to test that, given a cofx and args, the event handler returns the expected map of effects with the correct values (usually db transformations). Let's assume we need to test the following event. The code is still using the combo rf/defn & rf/merge. ```clojure (rf/defn accept-notification-success {:events [:activity-center.notifications/accept-success]} [{:keys [db] :as cofx} notification-id {:keys [chats]}] (when-let [notification (get-notification db notification-id)] (rf/merge cofx (chat.events/ensure-chats (map data-store.chats/<-rpc chats)) (notifications-reconcile [(assoc notification :read true :accepted true)])))) ``` As you can see, we're "rf/merging" two other functions, namely ensure-chats and notifications-reconcile. In fact, ensure-chats is not registered in re-frame, but it's 99% defined as if it's one because it needs to be "mergeable" according to the rules of rf/merge. Both of these "events" are quite complicated under the hood and should be unit tested on their own. Now here goes the unit test. Don't worry about the details, except for the expected output. ```clojure (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} expected {:db {:activity-center {:filter {:type 0 :status :all} :notifications [notif-2-accepted notif-1]} :chats {} :chats-home-list nil} ;; *** HERE *** :dispatch-n [[:activity-center.notifications/fetch-unread-count] [:activity-center.notifications/fetch-pending-contact-requests]]} actual (events/accept-notification-success cofx (:id notif-2) nil)] (is (= expected actual))))) ``` Notice the map has a :dispatch-n effect and other stuff inside of it that are not the responsibility of the event under test to care about. This happens because rf/merge forces the event handler to compute/call everything in one go. And things get MUCH worse when you want to test an event A that uses rf/merge, but A calls other events B and C that also use rf/merge (e.g. event :profile.login/get-chats-callback). At that point you flip the table in horror 😱, but testing events and maintaining them should be trivial. Solution: Use re-frame's `fx` effect. Here's the improved implementation and its accompanying test. ```clojure (defn accept-notification-success [{:keys [db]} [notification-id {:keys [chats]}]] (when-let [notification (get-notification db notification-id)] (let [new-notifications [(assoc notification :read true :accepted true)]] {:fx [[:dispatch [:chat/ensure-chats (map data-store.chats/<-rpc chats)]] [:dispatch [:activity-center.notifications/reconcile new-notifications]]]}))) (re-frame/reg-event-fx :activity-center.notifications/accept-success accept-notification-success) (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} ;; *** HERE *** expected {:fx [[:dispatch [:chat/ensure-chats []]] [:dispatch [:activity-center.notifications/reconcile [notif-2-accepted]]]]} actual (events/accept-notification-success cofx [(:id notif-2) nil])] (is (= expected actual))))) ``` Notice how the test expectation is NOT verifying what other events do (it's actually "impossible" now). Using fx completely decouples events and makes testing them a joy again. ### Problem 6: Unordered effects status-mobile still uses the legacy way to describe the effects map, which has the problem that their order is unpredictable. > Prior to v1.1.0, the answer is: no guarantees were provided about ordering. > Actual order is an implementation detail upon which you should not rely. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects > In fact, with v1.1.0 best practice changed to event handlers should only > return two effects :db and :fx, in which case :db was always done first and > then :fx, and within :fx the ordering is sequential. This new approach is more > about making it easier to compose event handlers from many smaller functions, > but more specificity around ordering was a consequence. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects ### Problem 7: Usage of deprecated effect dispatch-n We have 35 usages, the majority in new code using dispatch-n, which has been officially deprecated in favor of multiple dispatch tuples in fx. See https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/api-builtin-effects.md#L114 ### Problem 8: Complexity 🧙‍♂️ Have you ever tried to understand and/or explain how rf/merge and rf/defn work? They have their fare share of complexity and have tripped up many contributors. This is not ideal if we want to create a project where contributors can learn re-frame as quickly as possible. Re-frame is already complicated enough to grasp for many, the added abstractions should be valuable enough to justify. Interestingly, rf/merge is a stateful function, and although this is not a problem in practice, it is partially violating re-frame's spirit of only using pure functions inside event handlers. ### Problem 9: Using a wrapping macro rf/defn instead of global interceptors When rf/defn was created inside status-mobile, re-frame didn't have global interceptors yet (which were introduced 3+ years ago). We no longer have this limitation after we upgraded our old re-frame version in PR https://github.com/status-im/status-mobile/pull/15997. Global interceptors are a simple and functional abstraction to specify functions that should run on every event, for example, for debugging during development, logging, etc. This PR already shows this is possible by removing the wrapping function utils.re-frame/register-handler-fx without causing any breakage. ## Conclusion By embracing re-frame's best practices for describing effects https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/FAQs/BestPractice.md#use-the-fx-effect, we can solve long standing issues that affect every contributor at different levels and bring the following benefits: - Simplify the codebase. - Bring back the DX we all deserve, i.e. Clojure Language Server and clj-kondo fully working in event namespaces. - Greatly facilitate the testability of events. - Give devs more flexibility to make the app more responsive, because the new default would not coalesce events, which in turn, would block the UI thread for shorter periods of time. At least that's the theory, but exceptions will be found. The actions to achieve those benefits are: - Don't use the macro approach, replace rf/defn with re-frame.core/reg-event-fx. - Don't use rf/merge, simply use re-frame's built-in effect :fx. - Don't call event handlers as normal functions, just as we don't directly call subscription handlers. Use re-frame's built-in effect :fx. ## How do we refactor the remainder of the code? Some numbers first: - There are 228 events defined with rf/defn in src/status-im2/. - There are 34 usages of rf/merge in src/status_im2/. ## Resources - Release notes where fx was introduced in re-frame: https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2020.md#110-2020-08-24
2023-10-05 19:11:45 +00:00
(rf/defn invite-people-pressed
{:events [:communities/invite-people-pressed]}
[cofx id]
(rf/merge cofx
(reset-community-id-input id)
(bottom-sheet/hide-bottom-sheet)
(navigation/open-modal :invite-people-community {:invite? true})))
2022-11-14 10:15:49 +00:00
;; good
Migrate away from rf/defn and rf/merge (first step) (#17451) This commit shows how to move away from rf/defn https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.clj#L1-L90 & rf/merge https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.cljs#L39-L85 and why we should do it. ## Problems Before jumping to solutions, let's understand the problems first, in no order of importance. ### Problem 1: Cyclic dependencies If you ever tried to move event handlers or the functions used inside them to different files in status-mobile, you probably stumbled in cyclic dependency errors. When an event is registered in re-frame, it is globally available for any other place to dispatch. The dispatch mechanism relies on globally unique keywords, the so called event IDs, e.g. :chat/mute-successfully. This means that event namespaces don't need to require other event namespaces, just like you don't need to require subscription namespaces in views. rf/merge increases the likelihood of cyclic dependencies because they force event namespaces to require each other. Although not as common, this happened a few times with devs in the team and it can be a big hassle to fix if you are unlucky. It is a problem we should not have in the first place (at least not as easily). ### Problem 2: We are not linting thousands of lines of code The linter (clj-kondo) is incapable of understanding the rf/defn macro. In theory, we could teach clj-kondo what the macro produces. I tried this, but gave up after a few tries. This is a big deal, clj-kondo can catch many issues and will continue to catch more as it continue to evolve. It's hard to precisely count how many lines are affected, but `find src/ -type f -name 'events.cljs' -exec wc -l {} +` gives us more than 4k LOC. ### Problem 3: Blocking RN's UI thread for too long Re-frame has a routing mechanism to manage events. When an event is dispatched, it is enqueued and scheduled to run some time later (very soon). This process is asynchronous and is optimized in such a way as to balance responsiveness vs the time to empty the queue. >[...] when processing events, one after the other, do ALL the currently queued >events. Don't stop. Don't yield to the browser. Hog that CPU. > >[...] but if any new events are dispatched during this cycle of processing, >don't do them immediately. Leave them queued. > >-- https://github.com/day8/re-frame/blob/master/src/re_frame/router.cljc#L8-L60 Decisions were made (way back in 2017) to reduce the number of registered re-frame events and, more importantly, to coalesce events into bigger ones with the rf/merge pattern. I tried to find evidence of real problems that were trying to be solved, but my understanding is that decisions were largely based on personal architectural preferences. Fast-forward to 2023, and we are in a situation where we have many heavy events that process a LOT of stuff in one go using rf/merge, thus blocking the UI thread longer than we should. See, for example, [status-im2.contexts.profile.login.events/login-existing-profile](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L69), [status-im2.contexts.profile.login.events/get-chats-callback](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L98), and many others. The following excerpt was generally used to justify the idea that coalescing events would make the app perform better. > We will reduce the the amount of subscription re-computations, as for each > distinct action, :db effect will be produced and swapped into app-db only once > > -- https://github.com/status-im/swarms/issues/31#issuecomment-346345981 This is in fact incorrect. Re-frame, ever since 2015 (so before the original discussions in 2017) uses a concept of batching to process events, which means subscriptions won't re-run after every dispatched event, and thus components won't re-render either. Re-frame is smarter than that. > groups of events queued up will be handled in a batch, one after the other, > without yielding to the browser (previously re-frame yielded to the browser > before every single event). > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2015.md#050--2015-11-5 Here's a practical example you can try in a shadow-cljs :mobile REPL to see the batching behavior in practice. ```clojure ;; A dummy event that toggles between DEBUG and INFO levels. (re-frame/reg-event-fx :dummy-event (fn [{:keys [db]}] {:db (update-in db [:profile/profile :log-level] (fn [level] (if (= "DEBUG" level) "INFO" "DEBUG")))})) (def timer (js/setInterval #(re-frame/dispatch [:dummy-event]) 50)) ;; 1. In component status-im.ui.screens.advanced-settings.views/advanced-settings, ;; add a print call to see when it's re-rendered by Reagent because the ;; subscription :log-level/current-log-level will be affected by our dummy event. ;; ;; 2. Also add a print call to the subscription :log-level/current-log-level to ;; see that the subscription does NOT re-run on every dispatch. ;; Remember to eval this expression to cancel the timer. (js/clearInterval timer) ``` If you run the above timer with 50ms interval, you'll see a fair amount of batching happening. You can infer that's the case because you'll see way less than 20 print statements per second (so way less than 20 recomputations of the subscription, which is the max theoretical limit). When the interval is reduced even more, to say 10ms (to simulate lots of dispatches in a row), sometimes you don't see a single recomputation in a 5s window because re-frame is too busy processing events. This shows just how critical it is to have event handlers finishing as soon as possible to relinquish control back to the UI thread, otherwise responsiveness is affected. It also shows that too many dispatches in a row can be bad, just as big event handlers would block the batch for too long. You see here that dispatching events in succession does NOT cause needless re-computations. Of course there's an overhead of using re-frame.core/dispatch instead of calling a Clojure function, but the trade-off is clearly documented: the more we process in a single event, the less responsive the app may be because re-frame won't be able to relinquish control back to the UI thread. The total time to process the batch increases, but re-frame can't stop in the middle compared to when different dispatches are used. Thus, I believe this rf/merge pattern is harmful as a default practice in an environment such as ours, where it's desirable end-users feel a snappy RN app. I actually firmly believe we can improve the app's responsiveness by not coalescing events by default. We're also preventing status-mobile from taking the most advantage from future improvements in re-frame's scheduler. I can totally see us experimenting with other algorithms in the scheduler to best fit our needs. We should not blindly reduce the number of events as stated here https://github.com/status-im/status-mobile/pull/2634#discussion_r155243127. Solution: only coalesce events into one pile when it's strictly desirable to atomically update the app db to avoid inconsistencies, otherwise, let the re-frame scheduler do its job by using fx, not rf/merge. When needed, embrace *eventual app db consistency* as a way to achieve lower UI latency, i.e. write fast and short events, intentionally use :dispatch-later or other timing effects to bend the re-frame's scheduler to your will. There's another argument in favor of using something like rf/merge which I would like to deconstruct. rf/merge gives us a way to reuse computations from different events, which is nice. The thing here is that we don't need rf/merge or re-frame to reuse functions across namespaces. rf/merge complects re-frame with the need to reuse transformations. Instead, the solution is as trivial as it gets, reuse app db "transformers" across events by extracting the logic to data store namespaces (src/status_im/data_store). This solution has the added benefit of not causing cyclic dependency errors. ### Problem 4: Clojure's language server doesn't understand code under rf/defn Nowadays, many (if not most) Clojure devs rely on the Clojure Language Server https://github.com/clojure-lsp/clojure-lsp to be more effective. It is an invaluable tool, but it doesn't work well with the macro rf/defn, and it's a constant source of frustration when working in event namespaces. Renaming symbols inside the macro don't work, finding references, jumping to local bindings, etc. Solution: don't use rf/defn, instead use re-frame's reg-event-fx function and clojure-lsp will understand all the code inside event handlers. ### Problem 5: Unit tests for events need to "test the world" Re-frame's author strongly recommends testing events that contain non-trivial data transformations, and we do have many in status-mobile (note: let's not confuse with integration tests in status_im/integration_test.cljs). That, and non-trivial layer-3 subscriptions should be covered too. The reasoning is that if we have a well developed and tested state layer, many UI bugs can be prevented as the software evolves, since the UI is partially or greatly derived from the global state. See re-frame: What to Test? https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Testing.md#what-to-test. See PR Introduce subscription tests https://github.com/status-im/status-mobile/pull/14472, where I share more details about re-frame's testing practices. When we use rf/merge, we make unit testing events a perennial source of frustration because too many responsibilities are aggregated in a single event. Unfortunately, we don't have many devs in the team that attempted to write unit tests for events to confirm my claim, but no worries, let's dive into a real example. In a unit test for an event, we want to test that, given a cofx and args, the event handler returns the expected map of effects with the correct values (usually db transformations). Let's assume we need to test the following event. The code is still using the combo rf/defn & rf/merge. ```clojure (rf/defn accept-notification-success {:events [:activity-center.notifications/accept-success]} [{:keys [db] :as cofx} notification-id {:keys [chats]}] (when-let [notification (get-notification db notification-id)] (rf/merge cofx (chat.events/ensure-chats (map data-store.chats/<-rpc chats)) (notifications-reconcile [(assoc notification :read true :accepted true)])))) ``` As you can see, we're "rf/merging" two other functions, namely ensure-chats and notifications-reconcile. In fact, ensure-chats is not registered in re-frame, but it's 99% defined as if it's one because it needs to be "mergeable" according to the rules of rf/merge. Both of these "events" are quite complicated under the hood and should be unit tested on their own. Now here goes the unit test. Don't worry about the details, except for the expected output. ```clojure (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} expected {:db {:activity-center {:filter {:type 0 :status :all} :notifications [notif-2-accepted notif-1]} :chats {} :chats-home-list nil} ;; *** HERE *** :dispatch-n [[:activity-center.notifications/fetch-unread-count] [:activity-center.notifications/fetch-pending-contact-requests]]} actual (events/accept-notification-success cofx (:id notif-2) nil)] (is (= expected actual))))) ``` Notice the map has a :dispatch-n effect and other stuff inside of it that are not the responsibility of the event under test to care about. This happens because rf/merge forces the event handler to compute/call everything in one go. And things get MUCH worse when you want to test an event A that uses rf/merge, but A calls other events B and C that also use rf/merge (e.g. event :profile.login/get-chats-callback). At that point you flip the table in horror 😱, but testing events and maintaining them should be trivial. Solution: Use re-frame's `fx` effect. Here's the improved implementation and its accompanying test. ```clojure (defn accept-notification-success [{:keys [db]} [notification-id {:keys [chats]}]] (when-let [notification (get-notification db notification-id)] (let [new-notifications [(assoc notification :read true :accepted true)]] {:fx [[:dispatch [:chat/ensure-chats (map data-store.chats/<-rpc chats)]] [:dispatch [:activity-center.notifications/reconcile new-notifications]]]}))) (re-frame/reg-event-fx :activity-center.notifications/accept-success accept-notification-success) (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} ;; *** HERE *** expected {:fx [[:dispatch [:chat/ensure-chats []]] [:dispatch [:activity-center.notifications/reconcile [notif-2-accepted]]]]} actual (events/accept-notification-success cofx [(:id notif-2) nil])] (is (= expected actual))))) ``` Notice how the test expectation is NOT verifying what other events do (it's actually "impossible" now). Using fx completely decouples events and makes testing them a joy again. ### Problem 6: Unordered effects status-mobile still uses the legacy way to describe the effects map, which has the problem that their order is unpredictable. > Prior to v1.1.0, the answer is: no guarantees were provided about ordering. > Actual order is an implementation detail upon which you should not rely. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects > In fact, with v1.1.0 best practice changed to event handlers should only > return two effects :db and :fx, in which case :db was always done first and > then :fx, and within :fx the ordering is sequential. This new approach is more > about making it easier to compose event handlers from many smaller functions, > but more specificity around ordering was a consequence. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects ### Problem 7: Usage of deprecated effect dispatch-n We have 35 usages, the majority in new code using dispatch-n, which has been officially deprecated in favor of multiple dispatch tuples in fx. See https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/api-builtin-effects.md#L114 ### Problem 8: Complexity 🧙‍♂️ Have you ever tried to understand and/or explain how rf/merge and rf/defn work? They have their fare share of complexity and have tripped up many contributors. This is not ideal if we want to create a project where contributors can learn re-frame as quickly as possible. Re-frame is already complicated enough to grasp for many, the added abstractions should be valuable enough to justify. Interestingly, rf/merge is a stateful function, and although this is not a problem in practice, it is partially violating re-frame's spirit of only using pure functions inside event handlers. ### Problem 9: Using a wrapping macro rf/defn instead of global interceptors When rf/defn was created inside status-mobile, re-frame didn't have global interceptors yet (which were introduced 3+ years ago). We no longer have this limitation after we upgraded our old re-frame version in PR https://github.com/status-im/status-mobile/pull/15997. Global interceptors are a simple and functional abstraction to specify functions that should run on every event, for example, for debugging during development, logging, etc. This PR already shows this is possible by removing the wrapping function utils.re-frame/register-handler-fx without causing any breakage. ## Conclusion By embracing re-frame's best practices for describing effects https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/FAQs/BestPractice.md#use-the-fx-effect, we can solve long standing issues that affect every contributor at different levels and bring the following benefits: - Simplify the codebase. - Bring back the DX we all deserve, i.e. Clojure Language Server and clj-kondo fully working in event namespaces. - Greatly facilitate the testability of events. - Give devs more flexibility to make the app more responsive, because the new default would not coalesce events, which in turn, would block the UI thread for shorter periods of time. At least that's the theory, but exceptions will be found. The actions to achieve those benefits are: - Don't use the macro approach, replace rf/defn with re-frame.core/reg-event-fx. - Don't use rf/merge, simply use re-frame's built-in effect :fx. - Don't call event handlers as normal functions, just as we don't directly call subscription handlers. Use re-frame's built-in effect :fx. ## How do we refactor the remainder of the code? Some numbers first: - There are 228 events defined with rf/defn in src/status-im2/. - There are 34 usages of rf/merge in src/status_im2/. ## Resources - Release notes where fx was introduced in re-frame: https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2020.md#110-2020-08-24
2023-10-05 19:11:45 +00:00
(re-frame/reg-event-fx :communities/invite-people-pressed
(fn [{:keys [db]} [id]]
{:db (assoc db :communities/community-id-input id)
:fx [[:dispatch [:hide-bottom-sheet]]
[:dispatch [:open-modal :invite-people-community {:invite? true}]]]}))
2022-11-14 10:15:49 +00:00
```
### Registering top-level re-frame subscriptions
Use `subs.root/reg-root-key-sub` to register top-level (root) subscriptions.
Additionally, register root subscriptions in the `subs.root` namespace.
```clojure
;; bad
(re-frame/reg-sub
:view-id
(fn [db]
(:view-id db)))
;; good
(reg-root-key-sub :view-id :view-id)
```
### Registering layer-3 subscriptions
The majority of the subscriptions should be defined as [layer-3
subscriptions](https://day8.github.io/re-frame/subscriptions/#the-four-layer)
due to performance constraints.
```clojure
;; bad
(re-frame/reg-sub
:ens/preferred-name
(fn [db]
(get-in db [:multiaccount :preferred-name])))
;; good
(re-frame/reg-sub
:ens/preferred-name
:<- [:multiaccount]
(fn [multiaccount]
(:preferred-name multiaccount)))
```
### Requiring quo components
2022-11-14 10:15:49 +00:00
Consume `quo` components from `quo.core`, unless the namespace is also inside
the `quo/` directory.
2022-11-14 10:15:49 +00:00
```clojure
;; bad
(ns my-namespace
(:require [quo.components.icon :as icon]))
2022-11-14 10:15:49 +00:00
(icon/icon :i/verified)
;; good
(ns my-namespace
(:require [quo.core :as quo]))
2022-11-14 10:15:49 +00:00
(quo/icon :i/verified)
2022-11-14 10:15:49 +00:00
;; also good because both namespaces are inside quo/
(ns quo.components.tabs.account-selector
(:require [quo.components.markdown.text :as text]))
2022-11-14 10:15:49 +00:00
```
### Require/import
Prefer `:as` instead of `:refer`. There are exceptions to this rule, e.g. the
test macros `deftest` and `is`, which are ubiquitous in the Clojure community.
```clojure
;; bad
(ns status-im.utils.datetime
(:require [cljs-time.coerce :refer [from-long]]))
;; good
(ns status-im.utils.datetime
(:require [cljs-time.coerce :as time.coerce]))
```
2022-11-28 12:58:36 +00:00
### Javascript interop
Use [binaryage/oops](https://github.com/binaryage/cljs-oops) macros instead of
core interop macros.
```clojure
;; bad
(fn [^js event]
(.-width (.-nativeEvent event)))
;; good
(require '[oops.core :as oops])
(fn [event]
(oops/oget event "nativeEvent.width"))
```
2022-11-14 10:15:49 +00:00
### Accessibility labels
Accessibility labels are currently used only for end-to-end tests. Use keywords
instead of strings (remember keywords are cached).
2022-11-14 10:15:49 +00:00
```clojure
;; bad
[text/text {:accessibility-label "profile-nickname"}
"Markov"]
;; good
[text/text {:accessibility-label :profile-nickname}
"Markov"]
```
Avoid dynamic labels, for example to specify an element's index because
[Appium](https://appium.io/) already supports element selection based on
indices.
```clojure
;; bad
[button {:accessibility-label (str "do-something" index)}]
;; good
[button {:accessibility-label :do-something}]
```
2022-11-14 10:15:49 +00:00
### Icons
Use the appropriate keyword qualification/namespace.
2022-11-14 10:15:49 +00:00
```clojure
;; bad
(require '[quo.components.icon :as icons])
2022-11-14 10:15:49 +00:00
(icons/icon :main-icons2/verified)
;; good
(require '[quo.core :as quo])
(quo/icon :i/verified)
2022-11-14 10:15:49 +00:00
```
### Translations
Prefer to use translation placeholders instead of creating multiple translation
keywords and concatenating them into a single string.
```clojure
;; bad
;; Assume the translation key is:
;; "biometric-auth-error": "Unable perform biometric authentication"
(str (i18n/label :t/biometric-auth-error) "(" error-code ")")
;; good
;; Assume the translation key is:
;; "biometric-auth-error": "Unable perform biometric authentication ({{code}})"
(i18n/label :t/biometric-auth-error {:code error-code})
```
2022-12-06 16:36:05 +00:00
### Tests
#### Prefer `match?` over `=` when comparing data structures
Prefer the `match?` directive over `=` when comparing data structures, otherwise
when the check fails the output can be too difficult to read. `match?` is
defined by library https://github.com/nubank/matcher-combinators.
```clojure
;; bad
(deftest some-test
(let [expected {...}
actual {...}]
(is (= expected actual))))
;; good
(deftest some-test
(let [expected {...}
actual {...}]
(is (match? expected actual))))
```
2022-12-06 16:36:05 +00:00
#### Subscription tests
Test [layer-3 subscriptions](https://day8.github.io/re-frame/subscriptions/) by
actually subscribing to them, so reframe's signal graph gets validated too.
```clojure
;; bad
(defn user-recipes
[[current-user all-recipes location]]
...)
(re-frame/reg-sub
:user/recipes
:<- [:current-user]
:<- [:all-recipes]
:<- [:location]
user-recipes)
(deftest user-recipes-test
(testing "builds list of recipes"
(let [current-user {...}
all-recipes {...}
location [...]]
(is (= expected (recipes [current-user all-recipes location]))))))
;; good
(require '[test-helpers.unit :as h])
2022-12-06 16:36:05 +00:00
(re-frame/reg-sub
:user/recipes
:<- [:current-user]
:<- [:all-recipes]
:<- [:location]
(fn [[current-user all-recipes location]]
...))
(h/deftest-sub :user/recipes
[sub-name]
(testing "builds list of recipes"
(swap! rf-db/app-db assoc
:current-user {...}
:all-recipes {...}
:location [...])
(is (= expected (rf/sub [sub-name])))))
```
2022-11-14 10:15:49 +00:00
## Project Structure
First, the bird's-eye view with some example ClojureScript files:
```
src
├── js/
├── mocks/
├── quo
2022-11-14 10:15:49 +00:00
│ ├── components/
│ ├── foundations/
│ └── theme.cljs
├── react_native
│ ├── gesture.cljs
│ └── platform.cljs
├── status_im/
├── status_im2
2022-11-14 10:15:49 +00:00
│ ├── common
│ │ └── components
│ │ └── bottom_sheet.cljs
│ ├── contexts/
│ ├── setup/
│ └── subs/
├── test_helpers/
2022-11-14 10:15:49 +00:00
└── utils.cljs
```
- `src/js`: Raw Javascript files, e.g. React Native Reanimated worklets.
- `src/mocks`: Plumbing configuration to be able to run tests.
- `src/quo/`: The component library for Status Mobile. [Read more...](../src/quo/README.md)
2022-11-14 10:15:49 +00:00
- `src/react_native/`: Contains only low-level constructs to help React Native
work in tandem with Clojure(Script).
- `src/status_im2/`: Directory where we try to be as strict as possible about
our guidelines and where we prefer to write code for the new, redesigned
mobile app.
- `src/status_im/`: Directory containing what we call "old code", not yet
migrated to new guidelines for the new mobile app.
- `src/status_im2/common/`: Directories named `common` can appear at any level
of the directory tree. Just like directories named `utils`, their directory
2022-11-14 10:15:49 +00:00
nesting level communicates their applicable limits.
- `src/status_im2/common/components/`: Contains reusable components that are not
part of the design system (quo).
- `src/status_im2/contexts/`: Contains [bounded contexts](#glossary), like
2022-11-14 10:15:49 +00:00
`browser/`, `messaging/`, etc. As much as possible, _bounded contexts_ should
not directly require each other's namespaces.
- `src/status_im2/setup/`: Contains namespaces that are mostly used to
initialize the application, configure test runners, etc. In general, such
namespaces should not be required from the outside.
- `src/test_helpers/`: Reusable utilities for writing all kinds of tests.
2022-11-14 10:15:49 +00:00
- `src/status_im/subs/`: All subscriptions should live inside it.
Directories named `utils/` can appear at any level of the directory tree. The
directory nesting level precisely indicates its boundaries. For example, a
`contexts/user_settings/utils/datetime.cljs` file communicates that it should
only be used in the `user_settings` context.
### src/quo
2022-11-14 10:15:49 +00:00
The `src/quo/` directory holds all components for the new design system. As
2022-11-14 10:15:49 +00:00
much as possible, its sub-directories and component names should reflect the
same language used by designers.
Even though the directory lives alongside the rest of the codebase, we should
think of it as an external entity that abstracts away particular Status domain
knowledge.
Components inside `src/quo/` should not rely on re-frame, i.e. they should not
2022-11-14 10:15:49 +00:00
dispatch events or use subscriptions.
Example structure:
```
src
└── quo
2022-11-14 10:15:49 +00:00
├── components
│ └── dropdown
│ ├── style.cljs
│ ├── test.cljs
│ └── view.cljs
└── screens
└── dropdown
└── view.cljs
```
### Re-frame events
Event handlers should be defined in files named `events.cljs`, and they should
be _close_ to other _things_, like view files, components, etc.
For example:
```
src
└── contexts
└── browser
├── bookmarks/
├── options/
├── permissions/
├── events.cljs
├── events_test.cljs
├── style.cljs
└── view.cljs
```
## Deprecation process
To deprecate a var, add the `:deprecated` metadata and, if necessary, suggest an
alternative.
```clojure
;; Good if there's no better alternative yet, but we want to deprecate it anyway.
(defn ^:deprecated foo
[]
(bar))
;; Good
(defn foo
{:deprecated "Use some.namespace/var-name instead."}
[]
(bar))
```
Please check the [Clojure Style](https://guide.clojure.style/#deprecated) documentation
To reduce visual clutter from deprecated methods in your text editor, consult this [example](https://rider-support.jetbrains.com/hc/en-us/community/posts/4419728641810-How-to-disable-the-the-strike-thru-for-deprecated-methods-in-Javascript-#:~:text=Try%20disabling%20%22Preferences%20%7C%20Editor%20%7C,It%20works%20for%20me). The approach can be adapted for settings in VSCode, Emacs, VIM, and others.
Migrate away from rf/defn and rf/merge (first step) (#17451) This commit shows how to move away from rf/defn https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.clj#L1-L90 & rf/merge https://github.com/status-im/status-mobile/blob/f12c7401d167d7adb81f97bdf9c0902b39ee37bc/src/utils/re_frame.cljs#L39-L85 and why we should do it. ## Problems Before jumping to solutions, let's understand the problems first, in no order of importance. ### Problem 1: Cyclic dependencies If you ever tried to move event handlers or the functions used inside them to different files in status-mobile, you probably stumbled in cyclic dependency errors. When an event is registered in re-frame, it is globally available for any other place to dispatch. The dispatch mechanism relies on globally unique keywords, the so called event IDs, e.g. :chat/mute-successfully. This means that event namespaces don't need to require other event namespaces, just like you don't need to require subscription namespaces in views. rf/merge increases the likelihood of cyclic dependencies because they force event namespaces to require each other. Although not as common, this happened a few times with devs in the team and it can be a big hassle to fix if you are unlucky. It is a problem we should not have in the first place (at least not as easily). ### Problem 2: We are not linting thousands of lines of code The linter (clj-kondo) is incapable of understanding the rf/defn macro. In theory, we could teach clj-kondo what the macro produces. I tried this, but gave up after a few tries. This is a big deal, clj-kondo can catch many issues and will continue to catch more as it continue to evolve. It's hard to precisely count how many lines are affected, but `find src/ -type f -name 'events.cljs' -exec wc -l {} +` gives us more than 4k LOC. ### Problem 3: Blocking RN's UI thread for too long Re-frame has a routing mechanism to manage events. When an event is dispatched, it is enqueued and scheduled to run some time later (very soon). This process is asynchronous and is optimized in such a way as to balance responsiveness vs the time to empty the queue. >[...] when processing events, one after the other, do ALL the currently queued >events. Don't stop. Don't yield to the browser. Hog that CPU. > >[...] but if any new events are dispatched during this cycle of processing, >don't do them immediately. Leave them queued. > >-- https://github.com/day8/re-frame/blob/master/src/re_frame/router.cljc#L8-L60 Decisions were made (way back in 2017) to reduce the number of registered re-frame events and, more importantly, to coalesce events into bigger ones with the rf/merge pattern. I tried to find evidence of real problems that were trying to be solved, but my understanding is that decisions were largely based on personal architectural preferences. Fast-forward to 2023, and we are in a situation where we have many heavy events that process a LOT of stuff in one go using rf/merge, thus blocking the UI thread longer than we should. See, for example, [status-im2.contexts.profile.login.events/login-existing-profile](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L69), [status-im2.contexts.profile.login.events/get-chats-callback](https://github.com/status-im/status-mobile/blob/3082605d1e9897da1b1784588aa22fdc65c84823/src/status_im2/contexts/profile/login/events.cljs#L98), and many others. The following excerpt was generally used to justify the idea that coalescing events would make the app perform better. > We will reduce the the amount of subscription re-computations, as for each > distinct action, :db effect will be produced and swapped into app-db only once > > -- https://github.com/status-im/swarms/issues/31#issuecomment-346345981 This is in fact incorrect. Re-frame, ever since 2015 (so before the original discussions in 2017) uses a concept of batching to process events, which means subscriptions won't re-run after every dispatched event, and thus components won't re-render either. Re-frame is smarter than that. > groups of events queued up will be handled in a batch, one after the other, > without yielding to the browser (previously re-frame yielded to the browser > before every single event). > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2015.md#050--2015-11-5 Here's a practical example you can try in a shadow-cljs :mobile REPL to see the batching behavior in practice. ```clojure ;; A dummy event that toggles between DEBUG and INFO levels. (re-frame/reg-event-fx :dummy-event (fn [{:keys [db]}] {:db (update-in db [:profile/profile :log-level] (fn [level] (if (= "DEBUG" level) "INFO" "DEBUG")))})) (def timer (js/setInterval #(re-frame/dispatch [:dummy-event]) 50)) ;; 1. In component status-im.ui.screens.advanced-settings.views/advanced-settings, ;; add a print call to see when it's re-rendered by Reagent because the ;; subscription :log-level/current-log-level will be affected by our dummy event. ;; ;; 2. Also add a print call to the subscription :log-level/current-log-level to ;; see that the subscription does NOT re-run on every dispatch. ;; Remember to eval this expression to cancel the timer. (js/clearInterval timer) ``` If you run the above timer with 50ms interval, you'll see a fair amount of batching happening. You can infer that's the case because you'll see way less than 20 print statements per second (so way less than 20 recomputations of the subscription, which is the max theoretical limit). When the interval is reduced even more, to say 10ms (to simulate lots of dispatches in a row), sometimes you don't see a single recomputation in a 5s window because re-frame is too busy processing events. This shows just how critical it is to have event handlers finishing as soon as possible to relinquish control back to the UI thread, otherwise responsiveness is affected. It also shows that too many dispatches in a row can be bad, just as big event handlers would block the batch for too long. You see here that dispatching events in succession does NOT cause needless re-computations. Of course there's an overhead of using re-frame.core/dispatch instead of calling a Clojure function, but the trade-off is clearly documented: the more we process in a single event, the less responsive the app may be because re-frame won't be able to relinquish control back to the UI thread. The total time to process the batch increases, but re-frame can't stop in the middle compared to when different dispatches are used. Thus, I believe this rf/merge pattern is harmful as a default practice in an environment such as ours, where it's desirable end-users feel a snappy RN app. I actually firmly believe we can improve the app's responsiveness by not coalescing events by default. We're also preventing status-mobile from taking the most advantage from future improvements in re-frame's scheduler. I can totally see us experimenting with other algorithms in the scheduler to best fit our needs. We should not blindly reduce the number of events as stated here https://github.com/status-im/status-mobile/pull/2634#discussion_r155243127. Solution: only coalesce events into one pile when it's strictly desirable to atomically update the app db to avoid inconsistencies, otherwise, let the re-frame scheduler do its job by using fx, not rf/merge. When needed, embrace *eventual app db consistency* as a way to achieve lower UI latency, i.e. write fast and short events, intentionally use :dispatch-later or other timing effects to bend the re-frame's scheduler to your will. There's another argument in favor of using something like rf/merge which I would like to deconstruct. rf/merge gives us a way to reuse computations from different events, which is nice. The thing here is that we don't need rf/merge or re-frame to reuse functions across namespaces. rf/merge complects re-frame with the need to reuse transformations. Instead, the solution is as trivial as it gets, reuse app db "transformers" across events by extracting the logic to data store namespaces (src/status_im/data_store). This solution has the added benefit of not causing cyclic dependency errors. ### Problem 4: Clojure's language server doesn't understand code under rf/defn Nowadays, many (if not most) Clojure devs rely on the Clojure Language Server https://github.com/clojure-lsp/clojure-lsp to be more effective. It is an invaluable tool, but it doesn't work well with the macro rf/defn, and it's a constant source of frustration when working in event namespaces. Renaming symbols inside the macro don't work, finding references, jumping to local bindings, etc. Solution: don't use rf/defn, instead use re-frame's reg-event-fx function and clojure-lsp will understand all the code inside event handlers. ### Problem 5: Unit tests for events need to "test the world" Re-frame's author strongly recommends testing events that contain non-trivial data transformations, and we do have many in status-mobile (note: let's not confuse with integration tests in status_im/integration_test.cljs). That, and non-trivial layer-3 subscriptions should be covered too. The reasoning is that if we have a well developed and tested state layer, many UI bugs can be prevented as the software evolves, since the UI is partially or greatly derived from the global state. See re-frame: What to Test? https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Testing.md#what-to-test. See PR Introduce subscription tests https://github.com/status-im/status-mobile/pull/14472, where I share more details about re-frame's testing practices. When we use rf/merge, we make unit testing events a perennial source of frustration because too many responsibilities are aggregated in a single event. Unfortunately, we don't have many devs in the team that attempted to write unit tests for events to confirm my claim, but no worries, let's dive into a real example. In a unit test for an event, we want to test that, given a cofx and args, the event handler returns the expected map of effects with the correct values (usually db transformations). Let's assume we need to test the following event. The code is still using the combo rf/defn & rf/merge. ```clojure (rf/defn accept-notification-success {:events [:activity-center.notifications/accept-success]} [{:keys [db] :as cofx} notification-id {:keys [chats]}] (when-let [notification (get-notification db notification-id)] (rf/merge cofx (chat.events/ensure-chats (map data-store.chats/<-rpc chats)) (notifications-reconcile [(assoc notification :read true :accepted true)])))) ``` As you can see, we're "rf/merging" two other functions, namely ensure-chats and notifications-reconcile. In fact, ensure-chats is not registered in re-frame, but it's 99% defined as if it's one because it needs to be "mergeable" according to the rules of rf/merge. Both of these "events" are quite complicated under the hood and should be unit tested on their own. Now here goes the unit test. Don't worry about the details, except for the expected output. ```clojure (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} expected {:db {:activity-center {:filter {:type 0 :status :all} :notifications [notif-2-accepted notif-1]} :chats {} :chats-home-list nil} ;; *** HERE *** :dispatch-n [[:activity-center.notifications/fetch-unread-count] [:activity-center.notifications/fetch-pending-contact-requests]]} actual (events/accept-notification-success cofx (:id notif-2) nil)] (is (= expected actual))))) ``` Notice the map has a :dispatch-n effect and other stuff inside of it that are not the responsibility of the event under test to care about. This happens because rf/merge forces the event handler to compute/call everything in one go. And things get MUCH worse when you want to test an event A that uses rf/merge, but A calls other events B and C that also use rf/merge (e.g. event :profile.login/get-chats-callback). At that point you flip the table in horror 😱, but testing events and maintaining them should be trivial. Solution: Use re-frame's `fx` effect. Here's the improved implementation and its accompanying test. ```clojure (defn accept-notification-success [{:keys [db]} [notification-id {:keys [chats]}]] (when-let [notification (get-notification db notification-id)] (let [new-notifications [(assoc notification :read true :accepted true)]] {:fx [[:dispatch [:chat/ensure-chats (map data-store.chats/<-rpc chats)]] [:dispatch [:activity-center.notifications/reconcile new-notifications]]]}))) (re-frame/reg-event-fx :activity-center.notifications/accept-success accept-notification-success) (deftest accept-notification-success-test (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} notif-2 {:id "0x2" :type types/private-group-chat} notif-2-accepted (assoc notif-2 :accepted true :read true) cofx {:db {:activity-center {:filter {:type types/no-type :status :all} :notifications [notif-2 notif-1]}}} ;; *** HERE *** expected {:fx [[:dispatch [:chat/ensure-chats []]] [:dispatch [:activity-center.notifications/reconcile [notif-2-accepted]]]]} actual (events/accept-notification-success cofx [(:id notif-2) nil])] (is (= expected actual))))) ``` Notice how the test expectation is NOT verifying what other events do (it's actually "impossible" now). Using fx completely decouples events and makes testing them a joy again. ### Problem 6: Unordered effects status-mobile still uses the legacy way to describe the effects map, which has the problem that their order is unpredictable. > Prior to v1.1.0, the answer is: no guarantees were provided about ordering. > Actual order is an implementation detail upon which you should not rely. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects > In fact, with v1.1.0 best practice changed to event handlers should only > return two effects :db and :fx, in which case :db was always done first and > then :fx, and within :fx the ordering is sequential. This new approach is more > about making it easier to compose event handlers from many smaller functions, > but more specificity around ordering was a consequence. > > -- https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/Effects.md#order-of-effects ### Problem 7: Usage of deprecated effect dispatch-n We have 35 usages, the majority in new code using dispatch-n, which has been officially deprecated in favor of multiple dispatch tuples in fx. See https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/api-builtin-effects.md#L114 ### Problem 8: Complexity 🧙‍♂️ Have you ever tried to understand and/or explain how rf/merge and rf/defn work? They have their fare share of complexity and have tripped up many contributors. This is not ideal if we want to create a project where contributors can learn re-frame as quickly as possible. Re-frame is already complicated enough to grasp for many, the added abstractions should be valuable enough to justify. Interestingly, rf/merge is a stateful function, and although this is not a problem in practice, it is partially violating re-frame's spirit of only using pure functions inside event handlers. ### Problem 9: Using a wrapping macro rf/defn instead of global interceptors When rf/defn was created inside status-mobile, re-frame didn't have global interceptors yet (which were introduced 3+ years ago). We no longer have this limitation after we upgraded our old re-frame version in PR https://github.com/status-im/status-mobile/pull/15997. Global interceptors are a simple and functional abstraction to specify functions that should run on every event, for example, for debugging during development, logging, etc. This PR already shows this is possible by removing the wrapping function utils.re-frame/register-handler-fx without causing any breakage. ## Conclusion By embracing re-frame's best practices for describing effects https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/FAQs/BestPractice.md#use-the-fx-effect, we can solve long standing issues that affect every contributor at different levels and bring the following benefits: - Simplify the codebase. - Bring back the DX we all deserve, i.e. Clojure Language Server and clj-kondo fully working in event namespaces. - Greatly facilitate the testability of events. - Give devs more flexibility to make the app more responsive, because the new default would not coalesce events, which in turn, would block the UI thread for shorter periods of time. At least that's the theory, but exceptions will be found. The actions to achieve those benefits are: - Don't use the macro approach, replace rf/defn with re-frame.core/reg-event-fx. - Don't use rf/merge, simply use re-frame's built-in effect :fx. - Don't call event handlers as normal functions, just as we don't directly call subscription handlers. Use re-frame's built-in effect :fx. ## How do we refactor the remainder of the code? Some numbers first: - There are 228 events defined with rf/defn in src/status-im2/. - There are 34 usages of rf/merge in src/status_im2/. ## Resources - Release notes where fx was introduced in re-frame: https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/releases/2020.md#110-2020-08-24
2023-10-05 19:11:45 +00:00
2022-11-14 10:15:49 +00:00
### Test structure
[Unit tests](#glossary) should be created alongside their respective source
implementation. We prefer them colocated with the source and not like most
Clojure (JVM) codebases which mirror the sources in a top-level test directory.
```
├── models
│ ├── message.cljs
│ └── message_test.cljs
├── models.cljs
└── models_test.cljs
```
Upgrade shadow-cljs and ClojureScript (#15417) This commit upgrades Shadow CLJS from 2.11.16 (released on Feb/21) to latest 2.25.0 (Jul/23), so ~1.5 years worth of upgrades. By upgrading shadow we can finally use the latest major Clojure version 1.11.x. Why upgrade shadow? - Shadow CLJS controls the ClojureScript version we can use. In order to use the latest major Clojure version we must upgrade Shadow CLJS. - Shadow CLJS releases new versions very frequently, and if you take a look at its changelog https://github.com/thheller/shadow-cljs/blob/master/CHANGELOG.md, you'll see it had tons and tons of bug fixes over the years. I hope some of them help improve the DX for certain contributors who recently reported issues with it. - Clojure 1.11 brings new features, bug fixes and even performance improvements (although I think the performance mostly impacts Clojure on the JVM). See the changelog https://github.com/clojure/clojure/blob/master/changes.md#changes-to-clojure-in-version-1110 Things that can be beneficial to us, or are interesting nonetheless: - New :as-alias to be used in require, which is like :as but does not require the namespace to load. This means namespaced keywords using :as-alias can't cause circular dependency errors. This feature would very useful if we used namespaced keywords, but we don't, so... https://github.com/clojure/clojure/blob/master/changes.md#22-as-alias-in-require - New macros run-test and run-test-var to run single test with fixtures and report. - New iteration function, useful for processing paginated data. https://www.abhinavomprakash.com/posts/clojure-iteration/ - New update-keys function: applies a function to every key in a map. - New update-vals function: applies a function to every value in a map. Examples for update-vals and update-keys. They should perform better than the common reduce-kv approach since they use a transient data structure. (let [m {:a 1 :b 2}] (update-vals m inc)) ; => {:a 2, :b 3} (let [m {:a 1 :b 2}] (update-keys m name)) ; => {"a" 1, "b" 2} Why change namespaces within __tests__ directories? Any namespace with the word --tests-- throws an error, like the one below. I didn't bother investigating why, so I changed the guidelines to reflect the new convention. It's probably related to the double dashes in the name. Namespace quo2.components.dividers.--tests--.divider-label-component-spec has a segment starting with an invalid JavaScript identifier at line 1
2023-07-28 16:40:54 +00:00
Component tests should be created in the same directory as the source component,
and named as `component_spec.cljs`.
```
└── filter
├── component_spec.cljs
├── style.cljs
└── view.cljs
```
2022-11-14 10:15:49 +00:00
There's no hard rule on how integration test namespaces should be split, but
we're at least striving to define them under appropriate bounded contexts that
mirror the source code.
```
test
├── appium/
└── integration
├── browser/
├── communities/
├── messaging/
├── user_settings/
└── wallet
└── payment_test.cljs
```
## Glossary
**Unit test**: The smallest atomic unit that's meaningful to test. For example,
tests for utility functions and event handlers are considered unit tests in the
mobile codebase. They should be completely deterministic, _fast_, and they
should work flawlessly in the REPL.
**Bounded context**: A logical separation between different domains. It's an
important concept in the [Domain-Driven
Design](https://en.wikipedia.org/wiki/Domain-driven_design) literature. See
[Bounded Context, by Martin
Fowler](https://martinfowler.com/bliki/BoundedContext.html) for an introduction
to the topic.