Fork me on GitHub
#babashka-sci-dev
<
2022-04-08
>
cldwalker19:04:53

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

borkdude19:04:29

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

cldwalker19:04:17

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

borkdude19:04:50

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

borkdude19:04:53

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

cldwalker19:04:50

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

cldwalker19:04:52

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
# Passes
$ 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

cldwalker19:04:18

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

borkdude19:04:27

Is it doing anything with other JS libs?

borkdude19:04:33

Have you used datascript in other advanced compiled projects before?

cldwalker19:04:10

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

borkdude19:04:24

then what's the actual error you're getting

cldwalker19:04:36

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

borkdude19:04:29

And this does work in dev mode?

cldwalker19:04:19

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

borkdude20:04:36

interesting

borkdude20:04:52

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

cldwalker20:04:03

Looks like this is a known issue - https://github.com/tonsky/datascript/wiki/Tips-&amp;-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?

borkdude20:04:40

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

borkdude20:04:42

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

cldwalker20:04:51

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

borkdude20:04:04

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

borkdude20:04:14

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

👍 1
borkdude20:04:26

and no code diffs, that would be optimal

cldwalker20:04:10

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

borkdude20:04:37

We could add ExceptionInfo

borkdude20:04:48

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

👍 1
borkdude20:04:02

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

borkdude20:04:13

without changing the code

cldwalker20:04:32

Ah. That would be nice

cldwalker20:04:27

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

borkdude20:04:15

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

borkdude20:04:38

Maybe for now yes, just in classes

👍 1
borkdude20:04:44

would probably make most of the code work

borkdude20:04:59

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

cldwalker20:04:08

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