From 6e897f0ea6113f8242d4180f52eae6711a9c9938 Mon Sep 17 00:00:00 2001 From: Icaro Motta Date: Thu, 5 Oct 2023 19:11:45 +0000 Subject: [PATCH] Migrate away from rf/defn and rf/merge (first step) (#17451) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .zprintrc | 1 + doc/new-guidelines.md | 27 ++++++---- src/status_im/communities/core.cljs | 54 ++++--------------- src/status_im/events.cljs | 12 ++--- src/status_im/stickers/core.cljs | 17 ++---- src/status_im/test_runner.cljs | 2 + src/status_im/transport/message/core.cljs | 3 +- src/status_im/wallet/core.cljs | 23 ++++---- src/status_im/wallet/swap/core.cljs | 14 ++--- src/status_im2/contexts/chat/events.cljs | 30 +++++------ .../shell/activity_center/events.cljs | 46 ++++++++-------- .../shell/activity_center/events_test.cljs | 36 ++++++------- src/status_im2/core.cljs | 11 ++-- src/status_im2/navigation/events.cljs | 15 +++--- src/status_im2/setup/dev.cljs | 1 + src/status_im2/setup/interceptors.cljs | 29 ++++++++++ src/utils/re_frame.clj | 4 +- src/utils/re_frame.cljs | 9 ---- 18 files changed, 157 insertions(+), 177 deletions(-) create mode 100644 src/status_im2/setup/interceptors.cljs diff --git a/.zprintrc b/.zprintrc index dcf7bbd8f3..5be832ad4b 100644 --- a/.zprintrc +++ b/.zprintrc @@ -31,6 +31,7 @@ "defview" :arg1-body "letsubs" :binding "with-let" "let" + "reg-event-fx" :arg1-pair "testing" :arg1-body "deftest-sub" :arg1-body "wait-for" :arg1-body diff --git a/doc/new-guidelines.md b/doc/new-guidelines.md index 5a11fc6191..2aae5f80af 100644 --- a/doc/new-guidelines.md +++ b/doc/new-guidelines.md @@ -467,21 +467,28 @@ Prefer the pure version of `:json-rpc/call` (no callbacks). ### Registering event handlers -Events must always be declared with the `utils.fx/defn` macro. Also, don't use +Register events with `re-frame.core/reg-event-fx` and follow [re-frame's best +practice](https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/FAQs/BestPractice.md#use-the-fx-effect) +so use only `:db` and `:fx` effects. `rf/merge` is deprecated and should not be +used in the new code in `src/status_im2/`. Don't use `re-frame.core/reg-event-db`. ```clojure ;; bad -(re-frame/reg-event-fx - :wakuv2.ui/save-all-confirmed - (fn [{:keys [db] :as cofx}] - ...)) +(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}))) ;; good -(fx/defn save-all - {:events [:wakuv2.ui/save-all-confirmed]} - [{:keys [db] :as cofx}] - ...) +(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}]]]})) ``` ### Registering top-level re-frame subscriptions @@ -805,7 +812,7 @@ alternative. 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. - + ### Test structure [Unit tests](#glossary) should be created alongside their respective source diff --git a/src/status_im/communities/core.cljs b/src/status_im/communities/core.cljs index 23a0784307..09627e3a55 100644 --- a/src/status_im/communities/core.cljs +++ b/src/status_im/communities/core.cljs @@ -6,7 +6,6 @@ [quo2.foundations.colors :as quo2.colors] [re-frame.core :as re-frame] [status-im.utils.deprecated-types :as types] - [status-im.ui.components.emoji-thumbnail.styles :as emoji-thumbnail-styles] [status-im.utils.universal-links.core :as universal-links] [status-im.bottom-sheet.events :as bottom-sheet] [status-im2.common.toasts.events :as toasts] @@ -457,50 +456,17 @@ [{:keys [db]}] {:db (assoc db :communities/create-channel {})}) -(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-old) - (navigation/open-modal :invite-people-community {:invite? true}))) +(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}]]]})) -(rf/defn share-community-pressed - {:events [:communities/share-community-pressed]} - [cofx id] - (rf/merge cofx - (reset-community-id-input id) - (bottom-sheet/hide-bottom-sheet-old) - (navigation/open-modal :invite-people-community {}))) - -(rf/defn create-channel-pressed - {:events [::create-channel-pressed]} - [{:keys [db] :as cofx} id] - (rf/merge cofx - (reset-community-id-input id) - (reset-channel-info) - (rf/dispatch [::create-channel-fields - (rand-nth emoji-thumbnail-styles/emoji-picker-default-thumbnails)]) - (navigation/open-modal :create-community-channel {:community-id id}))) - -(rf/defn edit-channel-pressed - {:events [::edit-channel-pressed]} - [{:keys [db] :as cofx} community-id chat-name description color emoji chat-id category-id position] - (let [{:keys [color emoji]} (if (string/blank? emoji) - (rand-nth emoji-thumbnail-styles/emoji-picker-default-thumbnails) - {:color color :emoji emoji})] - (rf/merge cofx - {:db (assoc db - :communities/create-channel - {:name chat-name - :description description - :color color - :community-id community-id - :emoji emoji - :edit-channel-id chat-id - :category-id category-id - :position position})} - (navigation/open-modal :edit-community-channel nil)))) +(re-frame/reg-event-fx :communities/share-community-pressed + (fn [{:keys [db]} [id]] + {:db (assoc db :communities/community-id-input id) + :fx [[:dispatch [:hide-bottom-sheet]] + [:dispatch [:open-modal :invite-people-community {}]]]})) (rf/defn community-created {:events [::community-created]} diff --git a/src/status_im/events.cljs b/src/status_im/events.cljs index bfaa44f354..71c5d42e29 100644 --- a/src/status_im/events.cljs +++ b/src/status_im/events.cljs @@ -54,7 +54,6 @@ status-im2.contexts.shell.jump-to.events status-im2.contexts.onboarding.events status-im.chat.models.gaps - [status-im2.navigation.events :as navigation] [status-im2.common.theme.core :as theme] [react-native.core :as rn] [react-native.platform :as platform] @@ -250,10 +249,7 @@ :on-success (fn [on-ramps] (re-frame/dispatch [::crypto-loaded on-ramps]))}]}) -(rf/defn open-buy-crypto-screen - {:events [:buy-crypto.ui/open-screen]} - [cofx] - (rf/merge - cofx - (navigation/open-modal :buy-crypto nil) - (wallet/keep-watching-history))) +(re-frame/reg-event-fx :buy-crypto.ui/open-screen + (fn [] + {:fx [[:dispatch [:wallet/keep-watching]] + [:dispatch [:open-modal :buy-crypto nil]]]})) diff --git a/src/status_im/stickers/core.cljs b/src/status_im/stickers/core.cljs index 1403f13ffb..bbe9aa5ce8 100644 --- a/src/status_im/stickers/core.cljs +++ b/src/status_im/stickers/core.cljs @@ -1,13 +1,10 @@ (ns status-im.stickers.core - (:require [clojure.string :as string] - [re-frame.core :as re-frame] - [status-im2.constants :as constants] - [status-im2.config :as config] - [utils.re-frame :as rf] + (:require [re-frame.core :as re-frame] [status-im.utils.utils :as utils] - [status-im2.navigation.events :as navigation] + [status-im.wallet.utils :as wallet.utils] + [status-im2.constants :as constants] [utils.ethereum.chain :as chain] - [status-im.wallet.utils :as wallet.utils])) + [utils.re-frame :as rf])) (re-frame/reg-fx :stickers/set-pending-timeout-fx @@ -115,12 +112,6 @@ [{:keys [db]} packs] {:db (assoc db :stickers/recent-stickers packs)}) -(rf/defn open-sticker-pack - {:events [:stickers/open-sticker-pack]} - [{{:networks/keys [current-network]} :db :as cofx} id] - (when (and id (or config/stickers-test-enabled? (string/starts-with? current-network "mainnet"))) - (navigation/open-modal cofx :stickers-pack {:id id}))) - (rf/defn select-pack {:events [:stickers/select-pack]} [{:keys [db]} id] diff --git a/src/status_im/test_runner.cljs b/src/status_im/test_runner.cljs index 311ea52c1d..29c7d94c65 100644 --- a/src/status_im/test_runner.cljs +++ b/src/status_im/test_runner.cljs @@ -5,6 +5,7 @@ [shadow.test :as st] [shadow.test.env :as env] [utils.re-frame :as rf] + [status-im2.setup.interceptors :as interceptors] status-im2.setup.i18n-resources)) (defonce repl? (atom false)) @@ -113,6 +114,7 @@ (defn ^:export main [& args] (reset-test-data!) + (interceptors/register-global-interceptors) (rf/set-mergeable-keys #{:filters/load-filters :pairing/set-installation-metadata :dispatch-n diff --git a/src/status_im/transport/message/core.cljs b/src/status_im/transport/message/core.cljs index 696f82e3bc..5d0dbd09ce 100644 --- a/src/status_im/transport/message/core.cljs +++ b/src/status_im/transport/message/core.cljs @@ -23,6 +23,7 @@ [status-im.wallet.core :as wallet])) (rf/defn process-next + {:events [:transport/process-next]} [cofx ^js response-js sync-handler] (if sync-handler (sync-handler cofx response-js true) @@ -60,7 +61,7 @@ (js-delete response-js "chats") (rf/merge cofx (process-next response-js sync-handler) - (chat.events/ensure-chats (map data-store.chats/<-rpc (types/js->clj chats))))) + #(chat.events/ensure-chats % [(map data-store.chats/<-rpc (types/js->clj chats))]))) (seq messages) (models.message/receive-many cofx response-js) diff --git a/src/status_im/wallet/core.cljs b/src/status_im/wallet/core.cljs index 32557b7b5b..0f9673e770 100644 --- a/src/status_im/wallet/core.cljs +++ b/src/status_im/wallet/core.cljs @@ -197,12 +197,10 @@ :on-success #(re-frame/dispatch [::collectible-assets-fetch-success address collectible-slug %])}]})) -(rf/defn show-nft-details - {:events [::show-nft-details]} - [cofx asset] - (rf/merge cofx - {:db (assoc (:db cofx) :wallet/selected-collectible asset)} - (navigation/open-modal :nft-details {}))) +(re-frame/reg-event-fx ::show-nft-details + (fn [{:keys [db]} [asset]] + {:db (assoc db :wallet/selected-collectible asset) + :fx [[:dispatch [:open-modal :nft-details {}]]]})) (defn rpc->token [tokens] @@ -610,14 +608,11 @@ {:db (assoc-in db [:wallet/prepare-transaction field] value)} (bottom-sheet/hide-bottom-sheet-old))) -(rf/defn navigate-to-recipient-code - {:events [:wallet.send/navigate-to-recipient-code]} - [{:keys [db] :as cofx}] - (rf/merge cofx - {:db (-> db - (assoc :wallet/recipient {}))} - (bottom-sheet/hide-bottom-sheet-old) - (navigation/open-modal :recipient nil))) +(re-frame/reg-event-fx :wallet.send/navigate-to-recipient-code + (fn [{:keys [db]}] + {:db (-> db (assoc :wallet/recipient {})) + :fx [[:dispatch [:bottom-sheet/hide-old]] + [:dispatch [:open-modal :recipient nil]]]})) (rf/defn show-delete-account-confirmation {:events [:wallet.settings/show-delete-account-confirmation]} diff --git a/src/status_im/wallet/swap/core.cljs b/src/status_im/wallet/swap/core.cljs index eb8de5a82c..f7c0578db5 100644 --- a/src/status_im/wallet/swap/core.cljs +++ b/src/status_im/wallet/swap/core.cljs @@ -1,14 +1,15 @@ (ns status-im.wallet.swap.core (:require [re-frame.db :as re-frame.db] + [re-frame.core :as re-frame] [utils.re-frame :as rf] [status-im2.navigation.events :as navigation])) -(rf/defn open-asset-selector-modal - "source? true signinfies we are selecting the source asset. false implies selection of sink asset" - {:events [::open-asset-selector-modal]} - [{:keys [db]} source?] - (rf/merge {:db (assoc db :wallet/modal-selecting-source-token? source?)} - (navigation/open-modal :swap-asset-selector {}))) +;; "source? true" means we are selecting the source asset. false implies +;; selection of sink asset." +(re-frame/reg-event-fx ::open-asset-selector-modal + (fn [{:keys [db]} [source?]] + {:db (assoc db :wallet/modal-selecting-source-token? source?) + :fx [[:dispatch [:open-modal :swap-asset-selector {}]]]})) (rf/defn set-from-token {:events [::set-from-token]} @@ -48,4 +49,3 @@ :wallet/all-tokens vals (map #(str (:name %) "-" (:symbol %))))) - diff --git a/src/status_im2/contexts/chat/events.cljs b/src/status_im2/contexts/chat/events.cljs index fae4162249..0cf31d7c46 100644 --- a/src/status_im2/contexts/chat/events.cljs +++ b/src/status_im2/contexts/chat/events.cljs @@ -80,15 +80,15 @@ (:invitation-admin chat))))) (rf/defn leave-removed-chat + {:events [:chat/leave-removed-chat]} [{{:keys [view-id current-chat-id chats]} :db :as cofx}] (when (and (= view-id :chat) (not (contains? chats current-chat-id))) (navigation/navigate-back cofx))) -(rf/defn ensure-chats - "Add chats to db and update" - [{:keys [db] :as cofx} chats] +(defn ensure-chats + [{:keys [db] :as cofx} [chats]] (let [{:keys [all-chats chats-home-list removed-chats]} (reduce (fn [acc {:keys [chat-id profile-public-key timeline? community-id active muted] :as chat}] @@ -103,18 +103,18 @@ :chats-home-list #{} :removed-chats #{}} (map (map-chats cofx) chats))] - (rf/merge - cofx - (merge {:db (-> db - (update :chats merge all-chats) - (update :chats-home-list set/union chats-home-list) - (update :chats #(apply dissoc % removed-chats)) - (update :chats-home-list set/difference removed-chats))} - (when (not-empty removed-chats) - {:clear-message-notifications - [removed-chats - (get-in db [:profile/profile :remote-push-notifications-enabled?])]})) - leave-removed-chat))) + {:db (-> db + (update :chats merge all-chats) + (update :chats-home-list set/union chats-home-list) + (update :chats #(apply dissoc % removed-chats)) + (update :chats-home-list set/difference removed-chats)) + :fx [(when (not-empty removed-chats) + [:clear-message-notifications + [removed-chats + (get-in db [:profile/profile :remote-push-notifications-enabled?])]]) + [:dispatch [:chat/leave-removed-chat]]]})) + +(re-frame/reg-event-fx :chat/ensure-chats ensure-chats) (rf/defn clear-history "Clears history of the particular chat" diff --git a/src/status_im2/contexts/shell/activity_center/events.cljs b/src/status_im2/contexts/shell/activity_center/events.cljs index 940ee0ed46..6835499752 100644 --- a/src/status_im2/contexts/shell/activity_center/events.cljs +++ b/src/status_im2/contexts/shell/activity_center/events.cljs @@ -1,10 +1,10 @@ (ns status-im2.contexts.shell.activity-center.events (:require [quo2.foundations.colors :as colors] + [re-frame.core :as re-frame] [status-im.data-store.activities :as activities] [status-im.data-store.chats :as data-store.chats] [status-im2.common.toasts.events :as toasts] [status-im2.constants :as constants] - [status-im2.contexts.chat.events :as chat.events] [status-im2.contexts.shell.activity-center.notification-types :as types] [taoensso.timbre :as log] [utils.collection :as collection] @@ -21,20 +21,22 @@ ;;;; Navigation -(rf/defn open-activity-center - {:events [:activity-center/open]} - [{:keys [db]} {:keys [filter-type filter-status]}] - {:db (cond-> db - filter-status - (assoc-in [:activity-center :filter :status] filter-status) +(defn open-activity-center + [{:keys [db]} [{:keys [filter-type filter-status]}]] + {:db (cond-> db + filter-status + (assoc-in [:activity-center :filter :status] filter-status) - filter-type - (assoc-in [:activity-center :filter :type] filter-type)) - :dispatch [:open-modal :activity-center {}] - ;; We delay marking as seen so that the user doesn't see the unread bell icon - ;; change while the Activity Center modal is opening. - :dispatch-later [{:ms 1000 - :dispatch [:activity-center/mark-as-seen]}]}) + filter-type + (assoc-in [:activity-center :filter :type] filter-type)) + :fx [[:dispatch [:open-modal :activity-center {}]] + ;; We delay marking as seen so that the user doesn't see the unread bell icon + ;; change while the Activity Center modal is opening. + [:dispatch-later + [{:ms 1000 + :dispatch [:activity-center/mark-as-seen]}]]]}) + +(re-frame/reg-event-fx :activity-center/open open-activity-center) ;;;; Misc @@ -208,13 +210,14 @@ :on-error [:activity-center/process-notification-failure notification-id :notification/accept]}]}) -(rf/defn accept-notification-success - {:events [:activity-center.notifications/accept-success]} - [{:keys [db] :as cofx} notification-id {:keys [chats]}] +(defn accept-notification-success + [{:keys [db]} [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)])))) + (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) (rf/defn dismiss-notification {:events [:activity-center.notifications/dismiss]} @@ -527,7 +530,8 @@ (not dismissed)) (toasts/upsert cofx {:user user-avatar - :user-public-key chat-id ;; user public key who accepted the request + ;; user public key who accepted the request + :user-public-key chat-id :icon-color colors/success-50-opa-40 :title (i18n/label :t/contact-request-accepted-toast {:name (or name (:alias message))})}) diff --git a/src/status_im2/contexts/shell/activity_center/events_test.cljs b/src/status_im2/contexts/shell/activity_center/events_test.cljs index 2cb2f728d7..d6f123c687 100644 --- a/src/status_im2/contexts/shell/activity_center/events_test.cljs +++ b/src/status_im2/contexts/shell/activity_center/events_test.cljs @@ -13,18 +13,18 @@ (deftest open-activity-center-test (testing "opens the activity center with default filters" - (is (= {:db {} - :dispatch [:open-modal :activity-center {}] - :dispatch-later [{:ms 1000 :dispatch [:activity-center/mark-as-seen]}]} - (events/open-activity-center {:db {}} nil)))) + (is (= {:db {} + :fx [[:dispatch [:open-modal :activity-center {}]] + [:dispatch-later [{:ms 1000 :dispatch [:activity-center/mark-as-seen]}]]]} + (events/open-activity-center {:db {}} [nil])))) (testing "opens the activity center with filters enabled" - (is (= {:db {:activity-center {:filter {:status :unread :type types/contact-request}}} - :dispatch [:open-modal :activity-center {}] - :dispatch-later [{:ms 1000 :dispatch [:activity-center/mark-as-seen]}]} + (is (= {:db {:activity-center {:filter {:status :unread :type types/contact-request}}} + :fx [[:dispatch [:open-modal :activity-center {}]] + [:dispatch-later [{:ms 1000 :dispatch [:activity-center/mark-as-seen]}]]]} (events/open-activity-center {:db {}} - {:filter-type types/contact-request - :filter-status :unread}))))) + [{:filter-type types/contact-request + :filter-status :unread}]))))) (deftest process-notification-failure-test (testing "logs and returns nil" @@ -120,7 +120,7 @@ {:notifications [{:id "0x1" :read false :type types/one-to-one-chat}]}}}] - (is (nil? (events/accept-notification-success cofx "0x99" nil))))) + (is (nil? (events/accept-notification-success cofx ["0x99" nil]))))) (testing "marks notification as accepted and read, then reconciles" (let [notif-1 {:id "0x1" :type types/private-group-chat} @@ -128,13 +128,9 @@ 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]}}}] - (is (= {:db {:activity-center {:filter {:type 0 :status :all} - :notifications [notif-2-accepted notif-1]} - :chats {} - :chats-home-list nil} - :dispatch-n [[:activity-center.notifications/fetch-unread-count] - [:activity-center.notifications/fetch-pending-contact-requests]]} - (events/accept-notification-success cofx (:id notif-2) nil)))))) + (is (= {:fx [[:dispatch [:chat/ensure-chats []]] + [:dispatch [:activity-center.notifications/reconcile [notif-2-accepted]]]]} + (events/accept-notification-success cofx [(:id notif-2) nil])))))) (deftest dismiss-notification-test (is (= {:json-rpc/call @@ -375,11 +371,9 @@ notif-3 {:id "0x3" :read false :type types/private-group-chat :author author} cofx {:db {:activity-center {:notifications - [notif-3 ; will be ignored because it's not a contact - ; request + [notif-3 ; will be ignored because it's not a contact request notif-2 ; will be removed - notif-1 ; will be ignored because it's not from the - ; same author + notif-1 ; will be ignored because it's not from the same author ]}}}] (is (= {:db {:activity-center {:notifications [notif-3 notif-1]}}} (events/notifications-remove-pending-contact-request cofx author)))))) diff --git a/src/status_im2/core.cljs b/src/status_im2/core.cljs index 7990fb0be4..f6188fc2bd 100644 --- a/src/status_im2/core.cljs +++ b/src/status_im2/core.cljs @@ -15,6 +15,7 @@ [status-im2.setup.dev :as dev] [status-im2.setup.global-error :as global-error] [status-im2.common.log :as log] + [status-im2.setup.interceptors :as interceptors] [react-native.async-storage :as async-storage] [native-module.core :as native-module] [status-im.notifications.local :as notifications] @@ -43,16 +44,16 @@ (i18n-resources/load-language "en") (react-native-shake/add-shake-listener #(re-frame/dispatch [:shake-event])) (utils.universal-links/initialize) + (interceptors/register-global-interceptors) ;; Shell (async-storage/get-item :selected-stack-id #(shell.utils/change-selected-stack-id % nil nil)) (async-storage/get-item :screen-height #(reset! shell.state/screen-height %)) - ;; Note - We have to enable layout animations manually at app startup, - ;; otherwise, they will be enabled at runtime when they are used and will cause few bugs. - ;; https://github.com/status-im/status-mobile/issues/16693 - ;; We can remove this call, once reanimated library is upgraded to v3. - ;; Also, we can't move this call to reanimated.cljs file, + ;; Note - We have to enable layout animations manually at app startup, otherwise, they will be + ;; enabled at runtime when they are used and will cause few bugs. + ;; https://github.com/status-im/status-mobile/issues/16693. We can remove this call, once + ;; reanimated library is upgraded to v3. Also, we can't move this call to reanimated.cljs file, ;; because that causes component tests to fail. (as function is not mocked in library jestUtils) (reanimated/enable-layout-animations true) diff --git a/src/status_im2/navigation/events.cljs b/src/status_im2/navigation/events.cljs index fffd5164a6..ca727e69d0 100644 --- a/src/status_im2/navigation/events.cljs +++ b/src/status_im2/navigation/events.cljs @@ -1,5 +1,6 @@ (ns status-im2.navigation.events (:require [utils.re-frame :as rf] + [re-frame.core :as re-frame] [status-im2.contexts.shell.jump-to.state :as shell.state] [status-im2.contexts.shell.jump-to.utils :as shell.utils] [status-im2.contexts.shell.jump-to.events :as shell.events])) @@ -27,13 +28,13 @@ [_ comp-id] {:navigate-to-within-stack comp-id}) -(rf/defn open-modal - {:events [:open-modal]} - [{:keys [db]} comp screen-params] - {:db (-> (assoc db :view-id comp) - (all-screens-params comp screen-params)) - :dispatch [:hide-bottom-sheet] - :open-modal-fx comp}) +(re-frame/reg-event-fx :open-modal + (fn [{:keys [db]} [component screen-params]] + {:db (-> db + (assoc :view-id component) + (all-screens-params component screen-params)) + :fx [[:dispatch [:hide-bottom-sheet]] + [:open-modal-fx component]]})) (rf/defn dismiss-modal {:events [:dismiss-modal]} diff --git a/src/status_im2/setup/dev.cljs b/src/status_im2/setup/dev.cljs index 72e07fd72b..fe088e0df7 100644 --- a/src/status_im2/setup/dev.cljs +++ b/src/status_im2/setup/dev.cljs @@ -35,6 +35,7 @@ [] (rf/set-mergeable-keys #{:filters/load-filters :pairing/set-installation-metadata + :fx :dispatch-n :status-im.ens.core/verify-names :shh/send-direct-message diff --git a/src/status_im2/setup/interceptors.cljs b/src/status_im2/setup/interceptors.cljs new file mode 100644 index 0000000000..6c70da8342 --- /dev/null +++ b/src/status_im2/setup/interceptors.cljs @@ -0,0 +1,29 @@ +(ns status-im2.setup.interceptors + (:require [re-frame.core :as re-frame] + [re-frame.std-interceptors :as std-interceptors] + [utils.re-frame :as rf])) + +(defn register-global-interceptors + [] + (re-frame/reg-global-interceptor rf/debug-handlers-names) + (re-frame/reg-global-interceptor (re-frame/inject-cofx :now)) + + ;; Interceptor `trim-v` removes the first element of the event vector. + ;; + ;; Without the interceptor: + ;; + ;; (re-frame/reg-event-fx + ;; :some-event-id + ;; (fn [{:keys [db]} [_event-id arg1 arg2]] + ;; {:db ... + ;; :fx [...]})) + ;; + ;; With the interceptor: + ;; + ;; (re-frame/reg-event-fx + ;; :some-event-id + ;; (fn [{:keys [db]} [arg1 arg2]] + ;; {:db ... + ;; :fx [...]})) + ;; + (re-frame/reg-global-interceptor std-interceptors/trim-v)) diff --git a/src/utils/re_frame.clj b/src/utils/re_frame.clj index 8f38a892e1..afd9445ed9 100644 --- a/src/utils/re_frame.clj +++ b/src/utils/re_frame.clj @@ -3,10 +3,10 @@ (defn- register-events [events interceptors name argsyms] (mapv (fn [event] - `(utils.re-frame/register-handler-fx + `(re-frame.core/reg-event-fx ~event ~interceptors - (fn [cofx# [_# ~@argsyms]] (~name cofx# ~@argsyms)))) + (fn [cofx# [~@argsyms]] (~name cofx# ~@argsyms)))) events)) (defn- fully-qualified-name diff --git a/src/utils/re_frame.cljs b/src/utils/re_frame.cljs index 647f03f2b9..bff7c1231d 100644 --- a/src/utils/re_frame.cljs +++ b/src/utils/re_frame.cljs @@ -21,15 +21,6 @@ (log/debug "Handling re-frame event: " (first (interceptor/get-coeffect context :event))) context))) -(defn register-handler-fx - ([name handler] - (register-handler-fx name nil handler)) - ([name interceptors handler] - (re-frame/reg-event-fx - name - [debug-handlers-names (re-frame/inject-cofx :now) interceptors] - handler))) - (defn- update-db [cofx fx] (if-let [db (:db fx)]