Lack of consistency on how re-frame events are named

[ilmotta]

Problem

As years went by, the Status Mobile codebase accumulated many different flavors of naming schemes for re-frame events. There are events using fully qualified keywords, namespaced keywords, non-namespaced keywords, events starting with nouns, sometimes with verbs, and so on.

• PRs have more back and forth discussions about how events should be named.
• Some events have too common names, thus polluting the global registry with names that could be easily reused by mistake in other parts of the app, thus causing silent bugs. For example: :close-chat.
• Many events use Fully Qualified Keywords, which is known to make file renames more brittle and subject to bugs. Tools like lsp-clojure can help, but not everyone uses it.
• External contributors have a harder time knowing how to name events. This increases the cost for first contribution.

Out of scope

This issue doesn't care about how all the many events are organized in the codebase. Even though this too is extremely important, it only cares about a proper naming convention that works for the Status Mobile team.

Things we should keep in mind

1. Some events are respecting the namespace they're part of. For example src/status_im/multiaccounts/recover/core.cljs defines :multiaccounts.recover/re-encrypt-pressed. Sometimes the keyword needs to broken down a bit further (see point 2).

2. It's common to have event namespaces responsible for more than one thing, in practice, this means there's often a need to add a noun to the keyword, to make it clear what we're referring to in the same namespace. For instance, status-im.bootnodes.core namespace defines the events :bootnodes.callback/qr-code-scanned and :bootnodes.ui/delete-pressed, hence we can clearly see bootnode and ui are two different things.

3. There's a reasonably well known article about some of the issues of fully qualifying keywords. Fully qualified keywords can also be problematic for consumers of the namespace, as we can see from Clojure's 1.11 release, which added the option :as-alias (similar to :as in require) to help with circular dependencies. https://vvvvalvalval.github.io/posts/clojure-key-namespacing-convention-considered-harmful.html

4. Events can use different verb conjugations (e.g. past tense or present tense). For example, :status-im.add-new.core/qr-code-scanned, when successful, dispatches :status-im.add-new.core/qr-code-handled. There's always a way to convert between different conjugations though, so it's a matter of establishing a convention.

5. Effects usually dispatch subsequent success and/or error events. It's common in re-frame to add a suffix to the original event name, like -success. This is a very strong convention in re-frame apps, even suggested in re-frame's documentation.

Implementation

We all know the famous quote from Phil Karlton, that naming things is one of the most difficult things in Computer Science.

We believe there's no right or wrong in how events are named, and that it's simply a matter of defining a convention that's flexible enough and easy to follow. We don't want to define a hard rule that covers 100% of all use cases.

Convention 1 (suggestion)

This is just a suggestion among many others, feel free to suggest anything else, but please, try to share examples and a little bit of explanation on why you think it might be better.

1. Name event keywords with part of the fully qualified namespace, but not all of it. This patterns can scale well as the codebase grows and it's an easy rule to follow and make consistent. Whenever you look at an event name, you can be pretty sure where it's defined.

;; In src/status_im/multiaccounts/create/core.cljs

;; Old
(dispatch [:multiaccount-generate-and-derive-addresses-success])

;; New
(dispatch [:multiaccounts.create/generate-and-derive-addresses-success])

2. Name the event keyword in the present tense, as a verb, expressing what it does, not what happened, just like you would name a function. The book Elements of Clojure, by Zach Tellman has an interesting section on how to name functions. Additionally, naming them for what they do can help decouple them from lower level concepts, such as which React Native event was fired, which in turn can help come up with more stable keywords.

;; Old
(dispatch [:intro-wizard/on-key-selected (:id acc)])

;; New
(dispatch [:intro-wizard/mark-as-selected (:id acc)])

3. Add logical contexts to event keywords when necessary, even though they are not in the fully qualified namespace.

;; In src/status_im/multiaccounts/login/core.clj
(dispatch [:multiaccounts.login.callback/<xyz>])
(dispatch [:multiaccounts.login.ui/<xyz>])

4. When an event can succeed or fail, name them the same as the originating event, but add the -success or -error suffixes, respectively.

Acceptance Criteria

This issue can be closed once we can define a convention and a strategy on how to eliminate this tech debt, as well as an example PR showing some namespaces using the newly established convention.

We have around 900 events defined in the codebase, so it's definitely too much work to redefine all of them, even if we consider that 50% of them are perfectly fine.

[yqrashawn]

I suggest we come up the simplest rule that can be easily implemented with tools like clj-kondo or semgrep and implement the checks, run them in CI. After which, with the implemented tool/checks, we enrich the rules.

[ilmotta]

I suggest we come up the simplest rule that can be easily implemented with tools like clj-kondo or semgrep and implement the checks, run them in CI.

semgrep is a great tool, but unfortunately it doesn't support Clojure, but okay clj-kondo is more than capable and probably more adequate for Clojure.

+1 For gated checks in the CI.

Although I really prefer to automate, there are some important subtleties. I think we can definitely automate some checks, but event names have semantic meaning and we'll never be able to automate some things without generating lots of false warnings/errors. Even grammatical checks, like checking events are conjugated in the past/present tense would probably not work reliably. So what I'm saying is that even if we come up with the simplest rules first that can be automated by clj-kondo, later on we won't be able to automate some anyway, so let's just keep that in mind.

In terms of automation feasibility, I think Convention 1 (see issue description) points (1) (3) (4) can be automated by clj-kondo, but not (2). Do you think Convention 1 is reasonable for the mobile codebase? If not, do you have suggestions?

[erikseppanen]

Another dimension along which to consider Icaro's points is in the context of tests.

For example, when I was writing integration tests, sometimes I'd run into re-frame events called something like 'button-pressed' or some such and I'd die a little inside knowing that this test is supposed to be exercising business logic, and ideally (I am an idealist after all) it should be below the surface of the UI and whatever UI controls the user is interacting with.

I wouldn't ask others to write tests during development, but if it were me, and I had the time and approval, I would do it to help myself maintain clarity of the distinction between UI events and business logic. It kinda forces you to put some thought into naming, because they're exposed in the test.

[yqrashawn]

For me, naming is really complex and depends on the mind set of the person when editing the file. People familiar with status-go may come out a different name compared to people familiar with both status-go and waku. These affects what present tense, ogical contexts and can succeed or fail in 2,3,4.

So shall we come out a doc of dos and don'ts with simple descriptions as a starting point and point contributors to the doc? Or maybe we can open issues for each rule. Just make it more convenient for contributors to discus and see the history about each rule.

[ilmotta]

For me, naming is really complex and depends on the mind set of the person when editing the file. People familiar with status-go may come out a different name compared to people familiar with both status-go and waku. These affects what present tense, ogical contexts and can succeed or fail in 2,3,4.

So shall we come out a doc of dos and don'ts with simple descriptions as a starting point and point contributors to the doc? Or maybe we can open issues for each rule. Just make it more convenient for contributors to discus and see the history about each rule.

I agree with you @yqrashawn, it's too much effort with little value to define the rules for semantic meaning, past/present tense, etc, but I think we can align the very basic patterns at least.

I'll suggest the following to help us reach consensus, it's pretty straightforward IMO and it's something many parts of the codebase already do. For anyone collaborating, if you agree this a sound suggestion, please like/dislike. If you dislike, please suggest something else and why.

Convention 2 (suggestion)

1. Name event keywords with part of the fully qualified namespace, but not all of it. Don't use fully qualified keys. Example:

;; In src/status_im/multiaccounts/create/core.cljs

;; Old
(dispatch [:multiaccount-generate-and-derive-addresses-success])

;; New
(dispatch [:multiaccounts.create/generate-and-derive-addresses-success])

2. Add logical contexts to event keywords when necessary, even though they are not in the fully qualified namespace.

;; In src/status_im/multiaccounts/login/core.clj
(dispatch [:multiaccounts.login.callback/<xyz>])
(dispatch [:multiaccounts.login.ui/<xyz>])

3. When an event can succeed or fail, name them the same as the originating event, but add the -success or -error suffixes, respectively.

[OmarBasem]

Thanks @ilmotta for bringing up this issue. I noticed several naming inconsistencies across the codebase, not just re-frame events. Another naming inconsistency is in naming files/namespaces. For example, for list items files you can find:

1. X.cljs
2. X_item.cljs
3. X_list.cljs

It would be good if we can list all the spots that have such inconsistencies, and agree on the conventions to be followed, and have them in one document, which also any new contributor would be able to follow without introducing new inconsistencies.

[ilmotta]

It would be good if we can list all the spots that have such inconsistencies, and agree on the conventions to be followed, and have them in one document, which also any new contributor would be able to follow without introducing new inconsistencies.

Totally in agreement @OmarBasem and thanks for further raising the problem. An issue seems appropriate to me, if you're up to creating one ;) I'd personally try to limit the discussion to one specific problem at a time, given that establishing standards can be quite tricky in a team of our size, that's also highly distributed.

[OmarBasem]

Sure, I will create an issue for that. Let's focus here on agreeing on re-frame events naming.

[cammellos]

I'll move this to a discussion, feel free to create issues if there's anything that needs to be addressed