New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Starting up-to-state starts irrelevant states #15

Closed
dryewo opened this Issue Dec 19, 2017 · 18 comments

Comments

Projects
None yet
2 participants
@dryewo
Contributor

dryewo commented Dec 19, 2017

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

  • When a state is not started, dereferencing it throws an exception (while the original mount lazily starts it, which I don't like).

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

This comment has been minimized.

Owner

aroemers commented Dec 19, 2017

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

This comment has been minimized.

Contributor

dryewo commented Dec 19, 2017

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

This comment has been minimized.

Owner

aroemers commented Dec 19, 2017

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

This comment has been minimized.

Contributor

dryewo commented Dec 19, 2017

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

This comment has been minimized.

Contributor

dryewo commented Dec 19, 2017

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

This comment has been minimized.

Contributor

dryewo commented Dec 20, 2017

@aroemers

This comment has been minimized.

Owner

aroemers commented Dec 22, 2017

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

This comment has been minimized.

Contributor

dryewo commented Dec 22, 2017

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

This comment has been minimized.

Owner

aroemers commented Dec 23, 2017

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

This comment has been minimized.

Owner

aroemers commented Dec 24, 2017

Hi @dryewo,

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

@dryewo

This comment has been minimized.

Contributor

dryewo commented Dec 24, 2017

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: #15 (comment).
I opened a PR to fix it, please take a look.
And Merry Christmas :)

@aroemers

This comment has been minimized.

Owner

aroemers commented Dec 24, 2017

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

This comment has been minimized.

Contributor

dryewo commented Dec 24, 2017

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

@dryewo

This comment has been minimized.

Contributor

dryewo commented Dec 28, 2017

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

This comment has been minimized.

Owner

aroemers commented Dec 28, 2017

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

This comment has been minimized.

Contributor

dryewo commented Dec 28, 2017

Thanks! Looking forward to it. 🎉

@aroemers

This comment has been minimized.

Owner

aroemers commented Dec 30, 2017

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.

@aroemers aroemers closed this Dec 30, 2017

@dryewo

This comment has been minimized.

Contributor

dryewo commented Dec 30, 2017

Hi Arnout,

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment