aroemers / mount-lite

mount, but different and light
https://cljdoc.org/d/functionalbytes/mount-lite/
Eclipse Public License 1.0
102 stars 8 forks source link

Starting up-to-state starts irrelevant states #15

Closed dryewo closed 6 years ago

dryewo commented 6 years ago

Hi. First, thanks for the effort. I found mount-lite and tried it out because of one specific feature:

I'd like to adopt mount-lite, but there is one edge case that I found:
Starting up-to-state (http://www.functionalbytes.nl/mount-lite/03-start-stop-options.html) starts states that are not dependencies of the specified states. To illustrate this, I created an example project and provided a test:

https://github.com/dryewo/mount-lite-example

In a nutshell:

Given:

        +-- state1
base <--+
        +-- state2

Desired:

(m/start #'state1)
; => [#'base #'state1]

(m/start #'state2)
; => [#'base #'state2]

Actual:

(m/start #'state1)
; => [#'base #'state1]

(m/start #'state2)
; => [#'base #'state1 #'state2] ;; #'state1 shouldn't have been started 

This stands in the way of independently testing one state without starting the others. Did you ever encounter this case? Is there any solution to it?

Thanks in advance.

aroemers commented 6 years ago

Hi @dryewo,

In understand the confusion about the "up-to" concept. In older versions of mount-lite, your expected behaviour was exactly as you describe. It would calculate a graph of states and their dependencies, instead of a sequence, and start only those dependencies that were actually required.

However, the feature was not used often, but did add quite some complexity and (sometimes conflicting) dependencies. Therefore the current mount-lite has become more "lite" in that regard. It calculates a sequence of states. So, "up to" is up to the given state in that sequence.

I do understand your desire to have this graph-like functionality. Better yet, one of the included extensions brings back this graph functionality: http://www.functionalbytes.nl/mount-lite/mount.extensions.explicit-deps.html

It comes at a cost though, you will have to specify all the defstate's dependencies explicitly. If you want to make this trade-off is up to you :)

Does this answer clarify things for you?

dryewo commented 6 years ago

Thanks for the quick reply. That's a little bit sad that I'm in the minority now :) The workaround works, I tried. It will be needed for tests only, so let me try it out more. Thanks.

aroemers commented 6 years ago

Hi @dryewo,

If it is just for tests, then you might already be helped with the with-only macro from http://www.functionalbytes.nl/mount-lite/mount.extensions.basic.html#var-with-only. Using that macro would safe you from having to declare explicit dependencies everywhere.

For example (from the top of my head):

(basic/with-only [#'state1 #'state2] ; <-- can be in any order
  (m/start))
dryewo commented 6 years ago

That would mean that in every test I'll need to specify the components I'm testing (basically one component and all its dependencies). That's duplication of information that can be inferred from namespace dependencies anyway. That means, every time I add a new dependency to a component, I'll probably need to update every test fixture that has that component as a part.

The mount.extensions.explicit-deps workaround is also a duplication, of course, but to a lesser extent :)

The beauty of the original mount idea was that no duplication was required whatsoever, the dependency graph was defined exactly once - by :requires in namespace declarations.

Would it be possible to create a function infer-dependencies and use it like this in tests?

(basic/with-only (infer-dependencies #'state1)
  (m/start))

This would solve the duplication problem.

dryewo commented 6 years ago

I managed to write a function:

(defn infer-dependencies
  "Returns a list of states given state depends on."
  [state]
  {:pre [(var? state)]}
  (let [ns-deps        (-> @#'clojure.tools.namespace.repl/refresh-tracker :clojure.tools.namespace.track/deps :dependencies)
        state-ns       (symbol (str (:ns (meta state))))
        state-deps-nss (set (get ns-deps state-ns))]
    (filter #(contains? state-deps-nss (symbol (namespace %))) @m/*states*)))

It relies on a private var clojure.tools.namespace.repl/refresh-tracker, which happens to contain the dependency information. clojure.tools.namespace is a hack on its own, using its private vars is a double hack :)

dryewo commented 6 years ago

Final solution: https://gist.github.com/dryewo/7f3332797cce93293b7c825b494574f8

aroemers commented 6 years ago

Nice work! Which version of tools.namespace are you using? Maybe I can add similar code as an extension, if you'd like that.

dryewo commented 6 years ago

That would be great :) I'm using the latest version, "0.2.11". I found a minor issue in my implementation already, when you have more than one state in the same namespace:

(m/defstate s1 :start {})
(m/defstate s2 :start {})

;; Won't start s1
(start-up-to #'s2)

That should be easily fixable though.

aroemers commented 6 years ago

Hi @dryewo,

I just updated the master branch with work on using tools.namespace for state dependencies. If you like, can you try the mount.extension.namespace-deps extension?

aroemers commented 6 years ago

Hi @dryewo,

I released the current master on clojars as version "2.1.0-SNAPSHOT". This makes testing easier.

dryewo commented 6 years ago

Hi @aroemers, Thanks for doing this! I tried it quickly, seems to work. The only thing left is support for the case I described earlier: https://github.com/aroemers/mount-lite/issues/15#issuecomment-353557835. I opened a PR to fix it, please take a look. And Merry Christmas :)

aroemers commented 6 years ago

Hi @dryewo,

You are right, I overlooked the states in the same namespace as well. I released a new "2.1.0-SNAPSHOT" with a fix. This time I had some time to test it on a sample project as well. It seems to work. Hope it works for you as well.

Cheers and merry christmas to you too!

dryewo commented 6 years ago

Hi @aroemers, Thanks for the update, I updated my local copy, will test it out.

dryewo commented 6 years ago

Hi Arnout, I hope you are doing well. I'm running one application on mount-lite 2.1.0-SNAPSHOT in production, and I have an elaborate test suite on it, heavily relying on "start-up-to" functionality. I also switched my project template https://github.com/dryewo/cyrus to use mount-lite instead of mount. This all looks good to me, so I'm now interested in having mount-lite 2.1.0 release :) Do you have any plans for this? Thanks for your work! Happy New Year and have nice holidays.

aroemers commented 6 years ago

Hi Dmitrii,

That's good news. Thank you for testing! I will make a release soon; I think I want to extend mount-lite's test suite a bit with this new functionality.

Cheers and happy new year to you too! 🍾

dryewo commented 6 years ago

Thanks! Looking forward to it. 🎉

aroemers commented 6 years ago

Hi Dmitrii,

Version [functioalbytes/mount-lite "2.1.0"] has been released. Maybe I'll write about it in a blog post. I will mention you (and the cyrus project) for inspiring the development of this feature, and because your gist has helped as well.

dryewo commented 6 years ago

Hi Arnout,

Awesome, thanks a lot! It was a pleasure to work with you.