Fork me on GitHub
#clj-kondo
<
2021-10-31
>
benedek17:10:11

👋 @borkdude we discussed adding support for re-frame to morpheus (https://github.com/benedekfazekas/morpheus) to visualise re-frame event graphs/deps a while ago. now i forked clj-kondo to add the necessary changes to the analyzer (fork: https://github.com/benedekfazekas/clj-kondo and corresponding branch for morpheus https://github.com/benedekfazekas/morpheus/tree/support-re-frame) it kinda works but not really as it can’t really handle the case when keywords (namespaced or not) used as re-frame event ids are not unique. which is an absolute valid thing to do in re-frame ppl even tend to reuse the keywords as keys in the app-db. so to solve this i started looking into analyze certain re-frame things a bit more in depth (specially reg-sub and reg-event-fx) so i can make sure that referencese to re-frame event ids (keywords) are only considered if they occur at the right place, eg in a (rf/subscribe ...) or in a {:dispatch [:event-id...]). for that i planned to use some re-write clj zippers only i realised they are not available in clj-kondo… just curious about the reasons: perfromance? graalvm don’t like them? suppose you don’t really want them to be part of the inline re-write clj package, right?

borkdude18:10:38

@U0508JT9N Do you mean, zippers in the hooks?

benedek18:10:07

nope, not really. i checked out hooks briefly maybe did not really understand what they were for. i just went for modifing clj-kondo source directly

borkdude18:10:34

then where did you mean, zippers?

borkdude18:10:46

yes, clj-kondo itself doesn't use zippers for performance

benedek18:10:49

i mean i clj-kondo I can't really use rewrite-clj zippers can i?

benedek18:10:17

but if i implement my stuff as a hook i can?

borkdude18:10:25

@U0508JT9N There used to be a zipper API in the hooks: https://github.com/clj-kondo/clj-kondo/commit/40710fe755a10ef0623ae30d224b2d4bea0f5eea#diff-e170b652b1781fe9cad415205220a92f520f94f3c896160a3126e63e5b935694 Which is just the raw Clojure zip namespace which is sufficient in clj-kondo I think since there is no whitespace anymore. But I ended up removing this since I thought nobody was using this. I could add it back though.

borkdude19:10:51

that project is specifically to detect unused subscriptions, etc

benedek19:10:14

cheers will check it out

borkdude19:10:23

@U0508JT9N the reg-keyword! function is a bit weird: the name is not correct, since it doesn't cause a side effect, it just adds a :reg thing to the node.

borkdude19:10:34

sorry for that, it slipped through in a PR

borkdude19:10:52

I want to support a more broad :reg like thing, named :context so you can add any key to a node in a hook and it will appear in the analysis. https://github.com/clj-kondo/clj-kondo/issues/1211

borkdude19:10:09

I think we need this first and then move the reg stuff to use that

borkdude19:10:41

but looking at your code, it might also be nice to have something like :context {:foo :bar} which is returned from the hook and gets added recursively to all children nodes, which you can then override whenever you return another :context {:foo :baz} or so, so it will work with nested calls

borkdude19:10:56

feedback welcome

benedek20:10:10

oh somehow my phone did not update the slack thread properly. will check out re-frame related projet you sent the link for and let me think about this a bit more

benedek10:11:16

back on this, and yet an other commit on the top of what I showed you on Monday https://github.com/benedekfazekas/clj-kondo/commit/71644e49976c41f23b063de2c3d63f621421604f — this is kinda my whole proposal/solution only analysing a reg-event-fx body to find :dispatch (and its variations) is missing, subscriptions are simpler. the whole branch https://github.com/benedekfazekas/clj-kondo/commits/analyse-subs-disps — sorry for showing you half baked code, suppose it is just easier to discuss if there is some code, tests to back up what we are talking about. as you can see i am putting this in-subs and in-disp flags in the context, maybe that is what the :context could be used for.

borkdude10:11:59

Cool, I'll take a look at this later today or tomorrow

benedek14:11:56

i think i cracked the last bit of this, so created a PR https://github.com/clj-kondo/clj-kondo/pull/1446 i realise you might want this slightly or entirely differently. i have some unit tests as part of the PR and have not tested this yet with morpheus — plan to do it shortly. let me know what you think whenever you have time, thx!

borkdude17:11:01

Thanks, I'll take a look hopefully tomorrow, busy day :)

👍 1
benedek12:11:37

not to hurry you, just reporting really some tests on the re-frame example todo app with morpheus and this PR showed some encouraging results. I do need to add a bit to the PR for inject-cofx calls tho it seems. will test further wonder if this PR would enable a find references and unusued re-frame things as well in lsp

borkdude12:11:44

I have started looking into the PR. I would like to keep things a bit general instead of building in bespoke things for re-frame, so it will work for other libraries too. I'll be coming back with feedback on this. Perhaps the :context issue should be solved as a first step towards this.

👍 1
borkdude12:11:57

I think after your PR has been merged, lsp can benefit from this too