0.6.124 has a regression for cljs.test/assert-expr

[logseq-cldwalker]

Hey @borkdude. Noticed this regression while looking to bump nbb-logseq to latest nbb

version

0.6.124

platform

osx 12.0.1 and node 16.13.1

problem

When bumping nbb-features to latest nbb, CI fails on this line with exception Message: No protocol method IMultiFn.-add-method defined for type function:

repro

nbb -e "(require '[cljs.test :as t]) (defmethod t/assert-expr 'thrown-msg? [menv msg form] nil)"

expected behavior

I expected nbb-feature tests to run like they did for nbb 0.5.103

[borkdude]

I'll look into it tomorrow.

[logseq-cldwalker]

@borkdude Thanks for the fix! I bumped to 0.6.125 and ran into a cryptic Could not resolve symbol: G__107. I was able to work around this by changing the t/assert-expr to have 4 args like sci expects instead of the 3 args cljs expects. Is there a place where you want to documents differences between nbb and cljs behavior like this? This was tricky to track down and I could see this biting others

[borkdude]

@logseq-cldwalker The issue is this:

SCI needs to pass the context to this multimethod

https://github.com/babashka/sci.configs/blob/3eed9938d0f3b76326ed81005c5ac9e5d6aeca3a/src/sci/configs/cljs/test.cljs#L535

since the context is used here:

https://github.com/babashka/sci.configs/blob/3eed9938d0f3b76326ed81005c5ac9e5d6aeca3a/src/sci/configs/cljs/test.cljs#L547

And there is a special field in a SCI var which indicates that the interpreter should pass the context as the first argument when it's called. This should be invisible to the code.

But I didn't realize that when you implement this multi-method, that the user does in fact has to provide the corresponding arity.

The automatic context passing might not work so good here. Another alternative might be to let sci.configs or sci itself have the idea of a global context (atom or so) so that libraries like sci.configs can use that instead.

[borkdude]

The previous issue also resulted from this: since the context needed to be passed, the multimethod was wrapped with (partial multi-fn ctx) which made it not a multimethod anymore.

[borkdude]

So perhaps sci.core can introduce a *ctx* dynamic var and (set-ctx! ...) so the var can be set to a global context. Then sci.configs can use that context and require the user to call set-ctx! first. A dynamic var so multiple threads can use different contexts on the JVM. When we have that I can reliably solve the above problem.

[logseq-cldwalker]

I'm ok with whatever you solution you'd prefer. Agree that the more compatible the end solution is with clj/cljs the better.

require the user to call set-ctx! first

I'm guessing this is a sci user in which case the nbb experience is indeed better

[borkdude]

Yes, indeed, it's a SCI config thing, not something the script writer would see.

[borkdude]

Will take a stab at this tomorrow.

[logseq-cldwalker]

Confirmed this works as intended on nbb-features - https://github.com/babashka/nbb-features/runs/7344301638?check_suite_focus=true . Thanks!