Renaming or removing rules in CLJS requires recompilation
Closed this issue · 16 comments
In CLJS, if I remove a rule, the compiler appears to "remember" it. The rule continues to fire even though it's deleted from the rule namespace. When I rename a rule, I end up with a new rule with the new name that I can edit the conditions and consequences of per normal, but the rule with the old name still hangs around and fires. The only way I've found to remove a rule in either case is to restart figwheel. Issuing the (reset-autobuild) command doesn't work either -- the rule only clears when I fully shut it down and start back up.
I think it would greatly enhance the developer experience for those writing rules in CLJS if there was a way to remove rules without restarting figwheel. I'm not sure how to do this and I've been neglecting the issue for a while. :) Any help would be greatly appreciated.
I think there's a related problem Precept users have brought up regarding "hot reloading". I will open a separate issue for it if I can confirm it's isolated to Clara. Basically, changing a rule's condition or consequence requires the page to be refreshed manually for the changes to take effect. I believe I observed this outside of Precept when I was using Clara with re-frame, but I'm not positive what the ultimate cause is.
I agree that it would be desirable for redefining rules in ClojureScript to be reflected in the Figwheel environment, and if new rules can be added this is encouraging since it suggests that Figwheel can in fact update the session definitions. Unfortunately, it isn't obvious to me why the combination of Clara and Figwheel would behave this way, and I know basically nothing about Figwheel's internals.
Clara stores rule and query structures in the JVM ClojureScript compiler environment here; I could see something like this being an edge case for Figwheel. I wonder if the behavior you're observing is the result of a failure to do anything at all when the rule is replaced versus the result of having both the old rule and the new rule in the session.
@WilliamParker there isn't much magic to Figwheel I don't believe. It basically does a forced reload of namespaces that have been modified. It auto-detects file changes on its server-side.
The consumer-code (eg precept) won't be changing the clara.macros ns file, so it won't get reloaded.
It is odd to me that the Clara cljs impl stores things in the cljs.env/*compiler*. I haven't seen it used that way before and it seems a bit "low-level" to me. However, it is fine for now (I think).
However, to get the rules to reload in cljs then, it looks like you'd have to do something like:
(swap! env/*compiler* dissoc ::productions)If you were wanting more fine-grained control of what is removed, then:
(swap! env/*compiler* assoc-in [::productions (com/cljs-ns)] {})Perhaps Clara could provide a helper fn for this that allowed different ns's to be removed, different rules (by name) to be removed, etc.
(swap! env/*compiler* assoc-in [::productions <some-ns> <some-rule-name>] nil ; Or new compiled rule definition
)It is odd to me that the Clara cljs impl stores things in the cljs.env/compiler. I haven't seen it used that way before and it seems a bit "low-level" to me. However, it is fine for now (I think).
I'm not aware of another method that bridges the gap between runtime and compile time data. I've tried swapping rules data into an atom and that doesn't appear to work. It may in Clojure but not CLJS.
I was thinking if rule names could be stored in an atom that we could have a function that uses ns-unmap but it appears CLJS doesn't support that.
@WilliamParker @mrrodriguez Thanks very much for your responses. I'm thinking for env/*compiler* to be surfaced at runtime in any form, we'd have to pull in the CLJS compiler. I'm not sure if this is correct at all so would be curious to know your thoughts. We've been considering pulling in the compiler anyway despite the cost of doing so to enable runtime compilation of rules and macros.
If there's any interest in this I managed to implement this in Precept: https://github.com/CoNarrative/precept/blob/master/src/cljc/precept/repl.cljc#L134
Unfortunately a lot of the way I implemented it seems Precept specific but it might be possible to achieve the same in Clara. There was a fair amount of trial and error that went into it in order to get it to work.
If there's no interest I'm fine to close the issue.
@alex-dixon I completely agree that it would be helpful to have this supported in Clara itself in a generic fashion, so I don't think we need to close the issue. For my part, I haven't gotten around to looking at this in any detail yet and may not for some time unfortunately.
A PR addressing issues in this space from you or anyone else would be welcome. From a brief look over the Precept code you linked I'm guessing that the ClojureScript compiler environment is maintained rather than being restarted by Figwheel; otherwise I wouldn't expect those strategies to have any impact. It seems to me that some sort of helper that removed a namespace's rules from the compiler environment and then reloaded the namespace could probably work. This might require addressing the discrepancies in the namespace productions are attached to that are discussed at #327.
I took an initial try at fixing this. I haven't submitted a PR yet as I suspect it needs testing with more use-cases than the simple ones I've tried so far. I also only tested with boot cljs, not figwheel, though I'm pretty sure it should work in both cases. If anyone has suggested cases to try, I'll see how my changes hold up.
Changes are in this fork: https://github.com/Provisdom/clara-rules. The approach was to set a flag in the ::productions maps for a namespace when it was compiled into a session. Subsequent calls to defrule (actually clara.macros/add-production) would check for the presence of this flag for the current namespace, and if found would clear the productions for that namespace.
Is this issue related to #140?
@sparkofreason As I understand it, the algorithm you're proposing is as follows:
- Whenever the ClojureScript compiler expands a defrule macro, check if a "session created" marker for the current ClojureScript namespace is true. If it is, then clear the productions for that namespace and remove the marker.
- Set the marker to true each time defsession is macroexpanded with that namespace as a source.
This works if you make the assumption that a defsession consuming the namespace's rules, with the namespace as a source, always follows recompilation of a namespace. That doesn't seem like a valid assumption to me. For example, I think this approach would break in the following circumstance:
(ns rules-ns)
(defrule rule1
[A]
=>
(do-stuff))
;;;; New namespace
(ns session-ns)
(defsession user-session [rule1])
If the user then did:
(require 'rules-ns)
;;; Deleted rule1
(require 'rules-ns :reload)
(require 'session-ns)
Basically, there would never have been a session defined, so nothing would trigger the removal of rule1.
One approach that comes to mind for me is to have something like the following:
(ns rules-ns)
(clear-ns-productions!)
(defrule rule1
[A]
=>
(do-stuff))
This isn't quite as nice as being able to automatically detect a reload, but a single line of user code per namespace wouldn't be too bad IMO and might be more transparent to users about what is actually going on. This would also have the benefit of being able to be cross-platform between Clojure and ClojureScript; in Clojure it could just scan the namespace and remove all existing rules and queries, with detection based on their metadata marker. I'll note that this is actually a fairly general problem in Clojure. In REPL-driven development extra definitions for removed things stay if they aren't explicitly removed, so this could be of utility on the clj side as well.
Other related issues:
- In #327 @alex-dixon reported experiencing cases when the productions were keyed on session namespaces rather than rule namespaces. I haven't investigated this (just haven't gotten around to it unfortunately), but incorrect namespace keying would pose a problem for removing rules from a cache keyed on namespaces. If we do have a bug here, I think the fix would be to index under the correct namespace though. I wouldn't expect such a fix to fundamentally impact reload strategies like the ones we're discussing, though of course I could be mistaken. Note that the :ns-name of productions appears to be broken in ClojureScript, as discussed there some, although I'd expect it to work in many practical cases; I need to log an issue with more details/thoughts on that.
- Regarding #140, frankly when I first investigated that I had a weaker understanding of the ClojureScript compilation model than I do now. It might be possible to use the ClojureScript analyzer API to do this, specifically the cljs.analyzer.api namespace, but I do wonder if that would be worth the complication. I haven't thought about that issue in some time. If we did change the rule resolution model for ClojureScript obviously that would impact rule clearing strategies. I think a helper macro like I described above could be changed in the future if necessary to respect such a model though, probably by expanding to a call to cljs.core/ns-unmap. This might depend on the exact design used, but I suspect that we can resolve the issue at hand without dealing with #140 for now.
@mrrodriguez @rbrush Thoughts/objections, particularly on the API questions?
Hello! I implemented a version of this in Precept a while back. I'm confident the implementation could be better but will post the code here in case anyone wants to have a look.
Files from the PR here:
https://github.com/CoNarrative/precept/pull/101/files
This one is probably the most relevant:
https://github.com/CoNarrative/precept/pull/101/files#diff-88598b588c04414aa3cd41c0455d146c
@WilliamParker clear-ns-productions! seems like a reasonable idea. I'm guessing it's most useful for REPL interactions like you describe. For boot/figwheel reload scenarios, you're going to have to wire things up to ensure the session always gets reloaded anyway, otherwise it won't pick up any changes to the rule-base. Probably would still be best practice to include clear-ns-productions! to cover the general case.
I may have at least a partial solution for #327, will take the discussion over there.
I beat on #140 for awhile. The best I achieved was being able to identify the rules from metadata, which is a little different than in clj. Still don't see how to get at the value required to load the rules.
Another possibility would be to extend the rule definition DSL to allow for rules to be grouped under a symbol or keyword, rather than a namespace. That provides direct control over updating the associated entry in ::productions stored in the CLJS environment. Something like this:
(ns provisdom.eala-dubh.app
(:require-macros [clara.rules :refer [defrules defsession]]
[provisdom.eala-dubh.rules :refer [deffacttype]])
(:require [clara.rules :refer [insert insert-all retract fire-rules query insert! retract!]]
[provisdom.eala-dubh.session :as session]))
(deffacttype Foo [foo])
(defrules rools
foob ([`Foo [{foo :foo}] (= ?foo foo)]
=>
(println "FOOOOO" ?foo))
baab (['Foo [{foo :foo}] (= foo "bar")]
=>
(println "BAAAAA")))
(defsession session 'provisdom.eala-dubh.app/rools :fact-type-fn session/gettype)I tried a very quick prototype of this, and it does behave as desired on reload a la boot/figwheel. And though I haven't tried it yet, it should behave correctly in the REPL scenario described by @WilliamParker .
But, I guess that's not really significantly different than clear-ns-productions!, and the latter avoids some potential non-uniformity in how you reference queries.
I will take a shot at implementing clear-ns-productions!. Suggestions welcome on how to incorporate into the test suite.
@WilliamParker
The (clear-ns-productions!) at the top of a ns seems like a reasonable compromise to get started at least. It seems there may be more complex solutions (such as what was done in precept), but could be done later. I also think it is nice that putting this on the top of the ns would also work for CLJ cases. I believe that addresses the question you asked about the API?
I see I never responded to some comments on the issue here. I've merged #364 which addressed this issue. @mrrodriguez as you can presumably infer, I agree with the API in the PR since I merged it. :) We ended up working out how to test this on the PR itself. Thanks @sparkofreason.
Closing this issue.
I realized that we might want to discuss other sorts of more complicated approaches in the future.. my thought would be to log new issues as appropriate since there's enough here already IMO, but we can reopen this one if necessary/others prefer that as well.