I’m not quite clear on why/how this
# 💻｜contributinga
alert-helicopter-61082
11/01/2023, 1:54 PMI’m not quite clear on why/how this fixes the issue. Could we throw in a test as well?
s
swift-garage-61180
11/01/2023, 1:57 PMYeah, definitely. I mostly just wanted to open a PR to start the discussion.
This fixes the issue because previously, the destructuring operator created a new object (instead of adding to the old map of templates, which is essentially just mutating the old object that's still being referred to by the parent instance).
swift-garage-61180
11/01/2023, 1:58 PMIMHO this is sort of working by accident as is (same applies to the action configs).
swift-garage-61180
11/01/2023, 2:01 PM... I noticed we were adding the action configs and config paths to an existing data structure instead of creating a new one (like we were doing with the config templates), and that turned out to be reason why the second child Garden still had an empty map of config templates.
swift-garage-61180
11/01/2023, 2:03 PMThat's why I thought a singleton
ConfigRegistryswift-garage-61180
11/01/2023, 2:03 PMThere's a need to persist & share the configs & templates across the instance lifecycle in an easy-to-reason-about way.
a
alert-helicopter-61082
11/01/2023, 2:04 PMGotcha. I mean, we need more tests to start. That’s really the main issue. Singletons always feel off to me and make testing more difficult. But it’s entirely possible there’s a better way than what’s done atm.
s
swift-garage-61180
11/01/2023, 2:06 PMWe definitely need more tests—I'll do my best to write some around all this before I open this PR for review.
The main evil we're trying to avoid here is to implicitly rely on shared mutable state that has unclear semantics/invariants.
Tests would make all this less scary, but imho it's better to also make the state sharing semantics more clear.
swift-garage-61180
11/01/2023, 2:07 PMI'll just have a go at it and see how it feels.
If that ends up too clunky, then I'll just write tests around the current logic and verify the behaviour under the relevant circumstances.
b
brief-restaurant-63679
11/01/2023, 9:25 PMDoes this need to be a singleton though?
Can't this be a normal class that lives on the
InstanceManager