Potential guidelines for Malli

[ilmotta]

Problem

Since we merged https://github.com/status-im/status-mobile/pull/17867 on Nov 2023, we have been steadily exploring Malli and using it more and more. This has always been our goal, but as is the case with highly flexible/open libraries, we have to make numerous choices.

We already have a few guidelines for Malli in schema/README.md, but more are under discussion.

This issue's goal is to track choices/discussions that could eventually become guidelines if the team agrees on their utility and universality.

Feel free to add ideas as comments or by editing this issue's description.

Structure for common schemas

Where should we define global schemas? How to structure them in the project?

• https://github.com/status-im/status-mobile/pull/18707#discussion_r1476235521

Predicates as functions or as keywords?

Schemas registered in the global registry, such as :string, usually have a 1:1 relationship with core Clojure functions, in this case, string?. Using a function instead of a keyword gives us compile-time protection against typos, which is relatively useful, given that Malli tends to show cryptic errors when schemas are invalid.

• https://github.com/status-im/status-mobile/pull/18707#discussion_r1476084764

When to use catn versus the less verbose cat in function schemas?

Most components have a simple function signature: i.e. one arity and one argument (the map of props). Using catn requires wrapping the argument schema to give it a name. We've being using :props as the name.

Since the function signature is so common and simple, when its schema fails, having used catn won't make it easier to understand the cause of the error compared to just using cat because the developer clearly sees in the error there's only one argument that could have possibly failed.

• https://github.com/status-im/status-mobile/pull/18707#discussion_r1476261389

[2024-02-23] What to do when an instrumented function is used by :render-fn?

https://github.com/status-im/status-mobile/pull/18949#discussion_r1500698127

[2024-05-21] Should quo schemas be always defined in a separate file?

Components can wildly vary in their API (their props). Some are complex due to complexity in the Design System and use malli's multidispatch feature. But the majority is simpler.

By always writing the component schema in a separate file schema.cljs we create an indirection for something that's very core the component API, not like the file style.cljs, which is often tangential to the understanding of the component. This is arguably not the best DX for simple schemas.

So, should we always put quo schemas in separate schema.cljs files? Or should we do it only when the readability of the schema affects the component.cljs file due to its size/complexity?

[clauxx]

While writing some schemas & tests for events, I started wondering whether it makes sense to have more specific schemas for the event's cofx argument instead of just :schema.re-frame/cofx (which is an empty map). Say an event expects the db to have a :profile/profile with a :key-uid, the event will not work as expected if it doesn't. e.g. for the following event

(defn disable-biometrics
  [{:keys [db]}]
  (let [key-uid (get-in db [:profile/profile :key-uid])]
    {:db (assoc db :auth-method constants/auth-method-none)
     :fx [[:keychain/clear-user-password key-uid]]}))
(rf/reg-event-fx :biometric/disable disable-biometrics)

we could have the schema assure the db has the data the event needs, as here

(schema/=> disable-biometrics
  [:=>
   [:catn
    [:cofx
     [:map
      [:db
       [:map
        [:profile/profile [:map [:key-uid :string]]]]]]]]
   :schema.re-frame/event-fx])

Not set in any way on this, just wondering what you think. It seems fine for a simple example, but for larger db dependencies we could end up with a lot of duplication unless the whole db is spec-ed.

[clauxx]

Also might as well bring in the topic of open vs. closed maps. Would be good to reach a consensus on when it is appropriate to use one or the other and have it in the guidelines, so might as well finish the conversation here (or in a separate issue) @ilmotta @ulisesmac @J-Son89 @cammellos @mmilad75.

To summarise, I remember we ended up with the thesis was that we should use open maps whenever possible and maybe* use closed maps for quo components.

The maybe comes from some edge cases, where a component receives a prop taken directly from the db (or a selector) while it only uses a subset of that data. Making the map closed would add bloat by adding schemas for all the data, even if unused by the component.

The counter-argument to this is that passing data this way leads to potential perf issues and worse DX. Alternatives mentioned were either having layer 3 selectors or select the needed keys before passing to the quo component.

[ilmotta]

While writing some schemas & tests for events, I started wondering whether it makes sense to have more specific schemas for the event's cofx argument instead of just :schema.re-frame/cofx (which is an empty map). Say an event expects the db to have a :profile/profile with a :key-uid, the event will not work as expected if it doesn't. e.g. for the following event

Not set in any way on this, just wondering what you think. It seems fine for a simple example, but for larger db dependencies we could end up with a lot of duplication unless the whole db is spec-ed.

I certainly agree with you @clauxx. We would end up with lots of ad-hoc schemas, duplicated and scattered, with varying degrees of quality and coverage. Sounds like a nightmare to have multiple "types" for the same thing. I do believe to really profit from Malli in a re-frame app, we should explore spec'ing the app-db. It's the view's source of truth, and almost every piece of important information travels through the re-frame layer.

We would need to establish clear goals about why we would want to write & maintain them to justify the constant development cost. It's something that would require a lot more devs in the team be on board and have experience with Malli. I think we're not there tbh.

There was an attempt in the past to spec the app-db with clojure.spec in status-mobile, but it didn't fly. Things changed, but still.

Once schemas for certain parts of the app-db are defined, a completely new world of possibilities open... I won't share the ideas here to not make this comment even bigger.

[ilmotta]

so might as well finish the conversation here (or in a separate issue)

Maybe better is if we just use this issue to track potential guidelines and their pros/cons, but not necessarily decide them, otherwise this issue could become a monstrosity.

[ulisesmac]

Just giving some ideas.

@ilmotta

we should explore spec'ing the app-db.

Yeah, it'd be useful to guarantee the app-db consistency, but I don't think adding schemas to event handlers or subs is the right way to do it.

Also this si very related to what @J-Son89 is doing with contract tests, schemas for the db could be shared.

[ilmotta]

@ulisesmac 👍🏼 Ideally to me, the app-db would be spec'ed simultaneously (on-demand) as we build the app. Contract tests, integration tests, unit tests, schema generators, screen components, subscriptions, events, effects, we name it, all of them should be able to share domain entities, just like we tend to avoid duplicating types in status-go.

We will only be able to effectively instrument subscriptions if we have the schemas in place. But once we have them, it'll be natural to explore this. Meanwhile, without a spec'ed app-db, I don't see how instrumenting subscriptions could work, that's why we're not doing that. We're far away from that future.

[vkjr]

As for open/closed maps in schemas, I'd vote for closed.

Reason: If an optional key is misspelled or renamed during refactoring, schema will remain technically valid and it is wrong. For example component schema with non-closed map was:

[:map
  [:network {:optional true} :keyword] 
  ...]

and after component refactoring became

[:map
  [:network-name {:optional true} :keyword] 
  ...]

Now any component instance called with props {:network :eth} won't take that prop into account, but schema will still consider component instantiation as valid because :network will be treated as key that should be omitted.

[clauxx]

As for open/closed maps in schemas, I'd vote for closed.

@vkjr sorry forgot to post it here as well. There's a dedicated discussion for open/closed schemas over here