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.cljcWith 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?
Don't see any other npm/js lib for datascript. It's only other external dep is https://github.com/tonsky/persistent-sorted-set/blob/master/src-clojure/me/tonsky/persistent_sorted_set.cljs
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 thatAnd 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
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
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?
I think so
not sure, since it lives in the cljs.core namespace
Maybe for now yes, just in classes
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
right