search

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

mount.extensions.namespace-deps does not discover state dependencies via empty namespaces #18

Closed dryewo closed 4 years ago

dryewo commented 5 years ago

Hello Arnout!

We are using mount-lite extensively in several projects. It works quite nicely and makes testing subsystems really easy thanks to "start-up-to" function variants. However, there is a small issue that makes the experience less than perfect.

Consider this example: namespaces A and C contain states a and c, but are required via namespace B, which contains no states:

A(a) -> B() -> C(c)

Expected behavior when starting the app up to state a is that it starts both a and c, because the inferred state graph is:

a -> c

In practice it does not find the dependency and starts only a. A workaround is to put dummy states in all empty namespaces. It's not ideal, because people who are new to the codebase don't understand it and usually spend some time debugging some weird issues.

I found that the problem lies in mount.extensions.namespace-deps/ns-graph->state-graph function, which only considers namespaces that have states.

I'm eager to help fixing it, I already tried to do it locally, but before opening a PR I wanted to discuss solution approach with you. I have the following thoughts:

First, the simplest fix involves pre-processing namespace dependency graph by excluding empty namespaces from it and replacing them with additional dependencies, for example:

A(a) -> B() -> C(c)
we remove B and add an arrow from A to C
A(a) -> C(c)

However, for a general DAG it's not an easy procedure. It can be easily solved by means of loom, a graph processing library.

After the preprocessing is done, the existing algorithm works just fine.

Right now mount-lite already depends on clojure.tools.namespace (requiring it only when you use mount.extensions.namespace-deps). I think we can bring another optional library that makes our life much easier.

Second, if we bring loom, we can also simplify the existing ns-graph->state-graph algorithm by relying on loom. Current implementation is quite hard to understand because it does not operate graph concepts like edges and vertexes but rather low-level representation details.

Third, I could also imagine that mount-lite extensions can be distributed as separate libraries:

functionalbytes/mount-lite
functionalbytes/mount-lite.ext.explicit-deps
functionalbytes/mount-lite.ext.namespace-deps
...

For simplicity they can always be released at the same time and assigned the same version using some Leiningen plugin like lein-monolith. Having separate libraries for extensions would facilitate using of additional third-party libraries such as loom in those extensions - there will be no need to dynamically load them.

Then, users might use the extensions on opt-in basis, as well as for development only by including them under :dev profile (as we do, we use mount.extensions.basic, mount.extensions.data-driven and mount.extensions.namespace-deps just for tests).

Please let me know what you think.

aroemers commented 5 years ago

Hi Dmitrii!

Sorry for not responding earlier; busy times.

About the issue at hand. I either do not fully understand the problem, or your use case seems to work fine. I created a small test project to reproduce the issue. There it all seems to work fine, both in the REPL, and running it using lein run. If you still have the issue, can you supply a minimal codebase that reproduces it?

Third, I could also imagine that mount-lite extensions can be distributed as separate libraries:

You are right, that would be nicer indeed. Moving namespace-deps to another library would break current users that expect it in the core library. If major work will be done for mount-lite, I will definitely consider this!

dryewo commented 5 years ago

Hello Arnout!

I opened a PR to illustrate: https://github.com/aroemers/mount-lite-18/pull/1 The issue is reproduced, it only happens when you rely on "start-up-to" functionality - which we use in tests extensively.

aroemers commented 5 years ago

Hi Dmitrii,

Could you check out https://github.com/aroemers/mount-lite/tree/fix-18 and see if it fixes the issue for you?

dryewo commented 5 years ago

Hi Arnout,

Thanks for the quick fix! It indeed works, it let us get rid of roughly half of dummy states in the codebase. The other half has to do with needing to start parts of system in slightly different way that "start-up-to" currently supports, I will open a separate issue for that soon.

Could you please release the changes so that we can officially upgrade? Thanks again.

jmcs commented 5 years ago

@aroemers are you going to do a release with this fix?