Swap should have an option to compose the original state

[rkaippully]

When I use start-with or start-with-states, I need to provide a new value/state that replaces the value specified by the :start expression. I would like to have an ability to wrap/compose the original :start expression.

For e.g., let us say we have a state holding a DB connection.

(defstate db-conn :start (init-db-conn))

I want to run my tests with a mock DB connection that simulates a slow connection. So, it will be very handy to have something like this:

(defstate mock-db-conn :start-with-compose (fn [orig-conn] (wrap orig-conn)))

(mount/start-with-states {#'db-conn #'mock-db-conn})

The :start-with-compose key specifies a function that takes the original DB connection initialized in the state db-conn as a parameter and produces a new connection that will be swapped in. So the initialization proceeds like this.

1. Evaluate the :start expression, in this case (init-db-conn).
2. Pass that to the :start-with-compose and get the returned value.
3. The returned value gets bound to db-conn state.

Of course, this is a trivial example and one can very well argue that I can directly use (init-db-connection) in my mock-db-conn instead. But the ability to compose initialization expressions will be very handy when the initialization expressions are lot more complex.

[tolitius]

thanks for the idea.

I changed swap-with-states and start-with-states a bit in 0.1.11-SNAPSHOT to take values vs. actual vars. It was done to solve this issue.

It does not really affect "composing the original", but changes the way states are swapped.

As to the start-with-compose / swap-with-compose, the way I see it, it is a convenience API rather than a missing feature.

I understand that:

the ability to compose initialization expressions will be very handy when the initialization expressions are lot more complex

And the way I usually address this is by not having a complex init block being explicit. For example, let's say we have a state with fairly complex start/stop logic:

(defstate consumers :start (let [{:keys [threads poll-ms] :as conf} (to-consumer-conf (config :kafka))
                                 running? (atom true)
                                 pool (new-executor (if (number? threads)
                                                      threads
                                                      42))]
                             (dotimes [t threads]
                               (let [c (consumer (dissoc conf :threads :poll-ms))]
                                 (info "subscribing to:" (kafka/subscription c))
                                 (.submit pool #(consume c process running? poll-ms t))))
                             (info "started" threads "consumers ->" conf)
                             {:pool pool :running? running?})
                    :stop  (let [{:keys [pool running?] consumers}]
                             (reset! running? false)
                             (.shutdownNow pool)))

Instead of ^^^ I would split in in two functions:

(defn run-consumers [process conf]
  (let [{:keys [threads poll-ms] :as conf} (to-consumer-conf conf)
        running? (atom true)
        pool (new-executor (if (number? threads)
                             threads
                             42))]
    (dotimes [t threads]
      (let [c (consumer (dissoc conf :threads :poll-ms))]
        (info "subscribing to:" (kafka/subscription c))
        (.submit pool #(consume c process running? poll-ms t))))
    (info "started" threads "consumers ->" conf)
    {:pool pool :running? running?}))

(defn stop-consumers [{:keys [pool running? consumers]}]
  (reset! running? false)
  (.shutdownNow pool))

then the state is nice and lean:

(defstate event-listener :start (run-consumers process
                                               (config :kafka))
                         :stop (stop-consumers event-listener))

In this case if it needs to be swapped with a run-consumer wrapper, I would either create a wrapper as a separate function or just wrapped it in :start if it is something concise.

[rkaippully]

Thanks for the explanation, I agree that there are ways to avoid this and it is not necessarily a missing feature. In fact, I am refactoring some of my code in a similar way.

It is very useful if the person writing a test has not much control over the code that defines the state in the first place and hence cannot refactor it as you explained above. May be I am using a library with a defstate that I want to override.

Do you mind if I take a stab at adding this and send a PR?

[tolitius]

may be I am using a library with a defstate that I want to override

can you give an example of a library that have states? or maybe your particular use case?

In general I would think libraries should not have defstate in them, since states are better kept on the edges of an application: i.e. in the outmost circle here.

I really appreciate your willingness to contribute, but I'm more in a "let's remove most of APIs that are not needed, rather than add some for convenience" camp :) The thing is all APIs get old, need to be separately maintained and change, so there is a significant cost of adding an API just for convenience.

[rkaippully]

Ok, I understand the concern about maintainability. I'll find another workaround. Feel free to close this issue.

To give an example of the scenario I am talking about: I am working on a few microservices all of which uses a few internal libraries that provide basic services such as a config layer, db access layer etc. I only have semi-control over these libraries. So composability will be a big win.

[tolitius]

I am working on a few microservices all of which uses a few internal libraries that provide basic services such as a config layer, db access layer etc.

can you give an example of one such library? i.e. I am trying to understand why would a db access layer have a defstate vs. functions to create connection / connect / disconnect?

[rkaippully]

Unfortunately this is not public code, it is a set of internal libraries used within my organization but will have different owners/maintainers.

[tolitius]

just so I understand: and these different owners/maintainers have mount's defstate in a "datasource" library?

[rkaippully]

Yes, that is the plan; it is still under development. It'll be cleaner if the library itself handles the :start/:stop events rather than leaving those portions to the app. Do you have some recommendations?

[tolitius]

I think there is a difference between a "library" and an application "layer".

I would not use defstate in a library. First it would couple the library to mount, and I always try to minimize dependencies of moving pieces. Secondly, a library, in my mind provides tools. Which makes it side effect free until the tools are used.

In the case of an application "layer", I would use defstate only in cases where this layer is a part of the service / application: i.e. single deliverable. This enhanced code navigation, and also a state that starts and stops X is better understood when it lives along with its helpers: start and stop or other functions related to X. But again this would be the case where this "layer" lives together with the app.

It'll be cleaner if the library itself handles the :start/:stop events

I think a library would still be the one handling its :start/:stop events without having a defstate in it. A defstate would merely be a named reference to these events from the outside.

As we discussed before:

(defstate event-listener :start (run-consumers process))
                         :stop (stop-consumers event-listener))

is way cleaner and easier to understand and maintain than defining these functions inside the defstate definition.

[tolitius]

closing this. feel free to reopen if you have more questions