babashka-sci-dev 2022-04-08

Afternoon. Has anyone tried adding new dependencies to nbb and noticed different behavior between dev and release compiled versions? Adding datascript and seeing d/q fail for some unexpected cases

What is the unexpected error? Maybe an advanced compilation error?

Nothing fails during compilation. When I run this test ns, https://github.com/tonsky/datascript/blob/master/test/datascript/test/query.cljc, half of the tests fail in advanced mode but they pass fine in dev mode

That's not what I meant. I mean if you notice a difference between dev and prod and then it might be an error as the result of advanced compilation

if you compile with --debug you might be able to see what goes wrong in the case (if it is that)

Ah gotcha. The buggy release build definitely seems caused by advanced compilation. Will try --debug

If you're curious, here's the branch - https://github.com/babashka/nbb/compare/main...logseq-cldwalker:add-datascript?expand=1 . Steps to repro test failure:

$ NBB_FEATURES=datascript,datascript-transit bb compile
# Tests pass
$ node lib/nbb_main.js test-scripts/datascript-test/query.cljc
$ NBB_FEATURES=datascript,datascript-transit bb release
# Tests fail
$ node lib/nbb_main.js test-scripts/datascript-test/query.cljc

With NBB_FEATURES=datascript,datascript-transit bb release --debug --verbose didn't see any obvious failures or culprits. My next troubleshooting steps are: • see if it's an externs issue. Dunno how good shadow's :auto inference is • Look into the ns behind d/q , https://github.com/tonsky/datascript/blob/master/src/datascript/query.cljc, and see if any of the underlying namespaces aren't behaving as expected by running tests for the underlying namespaces I'm open to troubleshooting other ways too

Is it doing anything with other JS libs?

Have you used datascript in other advanced compiled projects before?

It's been awhile but used to use it a fair amount on browser targets. Don't know if I've used it against nodejs

then what's the actual error you're getting

Failed assertions when I run node lib/nbb_main.js test-scripts/datascript-test/query.cljc e .g.

FAIL in (test-q-in) (/Users/me/code/work/nbb/test-scripts/datascript-test/query.cljc:99)
expected: (= (d/q query db :name "Ivan") #{[1] [3]})
  actual: (not (= #{} #{[1] [3]}))
No explicit exceptions. Would be much more helpful if I had that

And this does work in dev mode?

yep. works fine. Interestingly enough, this issue so far seems specific to d/q behavior. These two other test namespaces pass fine in dev and advanced mode - https://github.com/tonsky/datascript/blob/master/test/datascript/test/query_rules.cljc and https://github.com/tonsky/datascript/blob/master/test/datascript/test/pull_api.cljc

interesting

and also specific to usage within SCI or also happening outside?

Looks like this is a known issue - https://github.com/tonsky/datascript/wiki/Tips-&-tricks . I'm able to manually modify the advanced compile js per one of the linked issues. I should eventually be able to get this right with a shadow compiler option 🙏 Thanks for chatting with me. Sometimes a quick chat helps unblock us. This is all the more motivating to get this working with nbb. I don't want logseq users and most cljs users to have to mess with compiler options for esm to be productive with datascript and cljs. 😄 For integration tests, I see the ci:test task runs off of one release build. Should I add another release build with features enabled or just reuse the existing one to also test datascript?

Maybe you can fork nbb and activate the tests in the CI of that fork?

Similar to bb, it doesn't run tests for features in CI because it would take way too long

That works too. I didn't know if you wanted to test that features worked in some way but happy to run them in my fork

Maybe this can be done through env vars as well or so

Then you'd only have to change your CI env settings

👍 1

and no code diffs, that would be optimal

Meaning no whitespace changes in PRs or something else? The recent clojure.test -> cljs.test was helpful. The only manual changes for datascript tests I have to do now are change ExceptionInfo to Error

We could add ExceptionInfo

I mean, that all of the feature code lives in the central repo

👍 1

but to kick off the tests for your fork which you are going to distribute I assume, you run the tests

without changing the code

Ah. That would be nice

> We could add ExceptionInfo Would be happy to do this. Would I add it under :classes in sci/init?

not sure, since it lives in the cljs.core namespace

Maybe for now yes, just in classes

👍 1

would probably make most of the code work

is that class used like (instance? ExceptionInfo ...)?

Used with a try/catch by thrown-with-msg? testing expression