Order transitive dependencies #21
Conversation
| :start "i am someone" | ||
| :dependencies nil)) | ||
|
|
||
| (defn undef-someone |
There was a problem hiding this comment.
This is hacky, but I didn't see a prior example of removing a previously defined state.
robhanlon22
force-pushed
the
order-transitive-dependencies
branch
from
ca358fc to
d20c36a
Compare
robhanlon22
force-pushed
the
order-transitive-dependencies
branch
from
d20c36a to
a7d8e13
Compare
robhanlon22
force-pushed
the
order-transitive-dependencies
branch
3 times, most recently
from
8711604 to
c17a2f6
Compare
Currently, transitive dependencies are being started in the order they were loaded, not in dependency order. This can cause issues in situations where new states which are needed for substitutes are loaded much later than the original state that is being overridden to depend on those new states. An example of this is in a test environment where you may have configuration overrides that you wouldn't define in your production overrides that are loaded from dev-only code. Without this change, the new test that was written failed because the newly added dependency was not loaded first. I had to do a bit of a hack to remove a state defined by defstate-let me know if there is a better way of doing this.
robhanlon22
force-pushed
the
order-transitive-dependencies
branch
2 times, most recently
from
8113709 to
7d41838
Compare
robhanlon22
force-pushed
the
order-transitive-dependencies
branch
from
7d41838 to
16c686f
Compare
Additionally, make sure depedencies are correctly ordered when starting/stopping with no up-to/down-to var
| (def graphs | ||
| {:dependencies {::overrides #{::overrides-a ::overrides-b} | ||
| ::config-a #{} | ||
| ::overrides-a #{::config-a} | ||
| ::config-b #{} | ||
| ::registry #{} | ||
| ::config #{::overrides ::env} | ||
| ::system #{::config ::debug ::db} | ||
| ::debug #{} | ||
| ::db #{::config} | ||
| ::env #{} | ||
| ::overrides-b #{::config-b}} | ||
| :dependents {::overrides #{::config} | ||
| ::config-a #{::overrides-a} | ||
| ::overrides-a #{::overrides} | ||
| ::config-b #{::overrides-b} | ||
| ::registry #{} | ||
| ::config #{::system ::db} | ||
| ::system #{} | ||
| ::debug #{::system} | ||
| ::db #{::system} | ||
| ::env #{::config} | ||
| ::overrides-b #{::overrides}}}) |
There was a problem hiding this comment.
This is a real-world dependency graph from an application that I'm working on, with the names changed to protect the innocent 😄
There was a problem hiding this comment.
Not sure if this is as clear as it could be, with these names.
|
👍 for consistent ordering! |
There was a problem hiding this comment.
Hi @robhanlon22,
Thank you for your PR. I can see where you're coming from and I'm inclined to merge this. I placed some review comments.
| @@ -40,6 +57,15 @@ | |||
| (deps/start #'state-3)) | |||
| (is (status) {#'state-1 :stopped #'state-2 :started #'state-3 :started})) | |||
|
|
|||
| (deftest start-with-overridden-dependency | |||
There was a problem hiding this comment.
Substitutes should always be non-global, i.e. not defined using defstate but by using state. Furthermore, all defstates one intends to use should be loaded on application boot-up. Defining new a defstate later is not how mount-lite should be used. Could you rewrite this test to reflect that?
| (def graphs | ||
| {:dependencies {::overrides #{::overrides-a ::overrides-b} | ||
| ::config-a #{} | ||
| ::overrides-a #{::config-a} | ||
| ::config-b #{} | ||
| ::registry #{} | ||
| ::config #{::overrides ::env} | ||
| ::system #{::config ::debug ::db} | ||
| ::debug #{} | ||
| ::db #{::config} | ||
| ::env #{} | ||
| ::overrides-b #{::config-b}} | ||
| :dependents {::overrides #{::config} | ||
| ::config-a #{::overrides-a} | ||
| ::overrides-a #{::overrides} | ||
| ::config-b #{::overrides-b} | ||
| ::registry #{} | ||
| ::config #{::system ::db} | ||
| ::system #{} | ||
| ::debug #{::system} | ||
| ::db #{::system} | ||
| ::env #{::config} | ||
| ::overrides-b #{::overrides}}}) |
There was a problem hiding this comment.
Not sure if this is as clear as it could be, with these names.
|
@robhanlon22, are you still interested in updating this PR? |
Currently, transitive dependencies are being started in the order they
were loaded, not in dependency order. This can cause issues in
situations where new states which are needed for substitutes are
loaded much later than the original state that is being overridden to
depend on those new states.
An example of this is in a test environment where you may have
configuration overrides that you wouldn't define in your production
overrides that are loaded from dev-only code. Without this change, the
new test that was written would fail because the newly added dependency was
not loaded first.
I had to do a bit of a hack to remove a state defined by defstate-let me
know if there is a better way of doing this.