Define some consistent approach to Re-Frame root keys

[J-Son89]

The current structure defined in status-im.subs.root - https://github.com/status-im/status-mobile/blob/develop/src/status_im/subs/root.cljs lacks a clear pattern or strategy. Perhaps there is one 🤷‍♂️
However for developers joining the project it can be unclear of how/when we want to add root keys for given contexts (Wallet, Communities etc)
Providing some guidelines on this would greatly help, and part of that process is defining what we want to get out of our guidelines.

How would you like to see the structure defined? What changes should we make? Or perhaps you like it as it currently is.

Please discuss below.

Some related discussion which helped initiate this
: #18756 (comment)

[cammellos]

It's hard to find an exact way to structure the db, I don't have an answer, but I know certain things about how the database should not be structured.

Historically, we had many performance issues caused by structuring the database based on how elements are logically together. For example one, at some point the database was structured like:

{:chats {:some-chat-id {:messages {:some-message-id {}}}}}

This leads to really poor performance in most cases, since it's very natural to subscribe to (get-in [:chats :some-chat-id]) to say render the header of the chat, but that would re-render every time a new message is re-rendered for that chat.

A better structure is {:chats {:chat-id {}} :messages {:chat-id {:message-id {}}}}

The pattern above has been used recently with communities.
{:communities {:community-id {:name "some-name" :permissions-check {}}}} where permission-check is a check we made where we want to check the whether we can join the community. Any time that check was performed, the whole view was re-rendering, since most of the components would want to subscribe to [:communities :community-id].
This to me are very clear cut cases where we should not be nesting things, and having root level subscription is better.

Nesting itself is not problematic, it's only an issue when data that has different read/write patterns is coupled together.

{:communities {:communities {:community-id {}}
                         :permissions-checks {:community-id {}}}}

would be fine in terms of performance (potentially better, since as @ulisesmac pointed out, this allows us to leverage caching of subscriptions better), as nobody would really subscribe to :communities in a view.

Note that {:communities {:community-id {:community-data {} :permissions-checks {}}} would not be a good idea since views might want to subscribe to [:communities] at this point, so an update to permissions-checks might retrigger subscriptions (or rendering) of a view that shows a list of communities.

In wallet, as far as I can see, a lot of stuff is structured in a way that doesn't impact performance, but with one exception that seems to have a similar pattern as above, where we nest collectibles within each account. {:wallet {:accounts {:account-id {:collectibles {}}}}, something like that. An update on collectible will likely re-trigger anything that rely on wallet/accounts (I might be wrong, I just had a cursory look).
I don't know anything about how collectibles are updated and what their lifecycle is, maybe that's fine, but I think that's a case as above where it's easy to make mistakes.

I could talk about this forever to be honest, I think it's a very nuanced topic and there's a lot of things to consider, also, my knowledge is limited in terms of actual performance, certain things I haven't really benchmarked etc.

But just to come up with something workable, and finding a middle ground between nesting/root keys.

Here some assumptions:

1. root db subscriptions are cheap (constant time), re-rendering are orders of magnitude more expensive. So having lots of root level keys, that trigger at each change in the db, but don't cause a re-render in any of the component, will likely be less expensive then an event that triggers fever subscriptions but causes a re-renderer. Error are very costly, so simpler is better.

2. This is effectively a database, so we should apply database design as a first consideration, how intuitive it looks in my opinion is of secondary importance (albeit it's fair to consider it), that can be easily abstracted by functions, as you would do with a normal database layer. Currently, there's no separation layer between the two, as subscriptions are not quite it, but you would normally have one.
   i.e if you have (defn community-by-id [db id]...) or (defn update-community-name [db community-id name]), that's agnostic of database structure for the developer, so we can easily change one over the other.

3. We should think about the lifecyle of objects and query patterns when you structure the database, query patterns should be the thing we think about when designing a structure. We should make sure things are easy to query and that those queries won't be re-run unnecessarily and that we minimize "user errors" that tends to oversubscribe.

The best thing I can come up with, and I am sure is far from perfect is:

1. Structure things as you would structure a SQL database, normalized, and index by the most common query pattern (that's something that was suggested by Mike Thompson I believe in some forums, I can look up the link if needed, though the discussion was not about perf.).
2. If you want to nest, nest by namespacing or prefixing, as you would name sql tables, nobody should subscribe to a namespace.
3. It's fine to nest collections if their lifecycle is identical, if it's not, most likely you should not group them together.

So, to give an example from the cases above, if we were to structure a sql database:

communities would be stored in a table, unique key is id.
permissions_checks would be stored in a separate table, unique key is community-id.

You would likely namespace/prefix the two tables.

So first example using only root keys:
{:communities/communities {:community-id-1 {} :community-id-2 {}}
{:communities/permissions-checks {:community-id {} :community-id-2 {}}

And using namespaces/prefixes:
{:communities {:communities {:community-id-1 {} :community-id-2 {}} :permissions-checks {:community-id-1 {} :community-id-2 {}}}}

Subscriptions should be (only one example, they are the same basically)

(defn all-communities [] (vals (get-in [:communities :communities]))
(defn community-by-id [id] (get-in [:communities :communities id))
(defn permissions-check-by-community-id [community-id] (get-in [:communities :permission-checks community-id]))

The latter example better leverage layer-2 subscriptions I would think.

Overall the SQL analogy seems to hold for the most part, there's some examples where that's a bit more difficult to apply (i.e settings, which is just a mixup of random fields, but that should be an exception), but it's the closest mental model I can find that leads to both a readable database, and a structure that should be fairly performant and avoid the most obvious pitfalls.

[cammellos]

Talking with @clauxx , another way to describe the above approach is to think about inner nodes (namespaces) and leafs (tables).

An inner should never be subscribed to in a view, only leafs should be subscribed to.

So in the case above:

{:community {:communities {} :permission-checks {}}} -> :community is an inner node, permissions-checks and :communities are leafs. No view should subscribe to :community, but they can be subscribed to :communities and :permissions-checks.

https://groups.google.com/g/clojurescript/c/x5RHF0710CU/m/Th1fizgnlmkJ is the link I was mentioning

As much as possible, I try to conceptualise "app-db" to be a database. That's the reason for that "db" in the name.

And, if you wanted to "update" a database, you need:
- the unique key of the thing to be updated
- the name of the table (collection).

and a bit out of context, but still relevant:

Tim Peters guides us in this area by recommending: "Flat is better than nested".

[ilmotta]

After reading @cammellos's very thoughtful and complete answer I realized one thing...

It seems that the main drive for me and others to have grouped state under a root key based on the bounded context was to make it both easier and simpler to understand that part of the app-db state and how it affected the views. It's undeniable, from practical experience in our team, that spreading state into multiple root keys can be detrimental to the comprehension of the app state, at least for many of us.

But technically speaking, I think Andrea is right. The app-db should be designed more like a relational database (of course, with all the exceptions considered between this analogy). In the long run, and if we do it right, it can lead to a simpler design, which should offset the more immediate impact in understanding the app state.

We can have both things I guess. We can design the app-db with more root keys, normalize more, and so on. We can also devise ways to make multiple root keys be even more "logically connected" (from the dev's perspective). Ideas:

• Have a concise documentation in status-im.subs.root explaining what root keys are supposed to store, why they exist, etc. Nowadays we just hope the dev will be able to reverse engineer from reading lots of events and subs. It would need to be concise, otherwise it would easily get outdated.
• Re-structure the directory status_im/subs/. For example, the wallet subs are organized under the wallet/ directory. We could do something similar for other contexts. But we would still keep the state unnested.
• Re-structure the sub names. For example, :delete-profile/error doesn't have the same qualification as :profile/login, but both refer to profiles in some way. But truth is, most of our subs are already logically grouped quite well given most share the same qualification (e.g. the majority of communities state is qualified by :communities/). Naming is hard, so there's probably valuable work to be done in this area.

I don't hold any strong opinion. But ever since Andrea shared with me his thoughts and his experience on the mobile team, I'm inclined to favor more root keys (less nesting) and find a way to logically keep them connected without grouping too much at the app-db level. If we move in that direction I could unnest the state for the activity center as a way to share an example.

[Parveshdhull]

Thank you Andrea and Icaro for sharing your thoughts. This knowledge of why was very important before we reach to the conclusion. I like the analogy of database, and it makes sense to me.

Have a concise documentation in status-im.subs.root explaining what root keys are supposed to store, why they exist, etc.

And I think not only roots, but we should define clear pattern/instructions for creating new leafs.

Otherwise developers will face same question, where should I store it? And as Andrea shared above, we will end up in situation where we are storing everything inside a single key.

Nesting itself is not problematic, it's only an issue when data that has different read/write patterns is coupled together.

Something like this can be part of the guideline, as it explains the logic behind which/why data needs to be separated.

[J-Son89]

Yep, agreed!