Fork me on GitHub
#babashka-sci-dev
<
2022-04-11
>
cldwalker18:04:14

@borkdude Heyo. Following up on the requires suggestion, how about we introduce a nbb/register-feature! that gets placed in an init similar to nbb/register-plugin! e.g. (nbb/register-feature ::datascript {:requires {'datascript.core "./nbb_datascript.js"}}) ?

borkdude18:04:39

yeah something like that is good

cldwalker18:04:03

We could also make it a part of register-plugin! but didn't know if you wanted to mix impl and feature concerns

borkdude18:04:29

This is a chicken and egg problem: since you need to load the JS first, which calls register-plugin

borkdude18:04:46

but the JS only gets loaded because of a require

borkdude18:04:58

I think it doesn't hurt if we just add the datascript namespaces into the case statement. If the JS isn't there, then you will get an error, but you would get an error when requiring those nss anyway

cldwalker18:04:10

Ah ok. I hadn't tried this yet but happy to keep with it as is if register-feature! isn't possible

borkdude18:04:59

How would that work?

borkdude18:04:13

Where would register-feature! for datascript be called and when?

cldwalker18:04:49

Ah. Now I understand the chicken and egg reference. It's not possible. We could also have a map in nbb.features if it helps to not have new features modify nbb.core

borkdude18:04:27

We could solve this maybe using a macro which using "JVM" time looks at some env variables or find all features.edn files on the classpath and merges them or so

borkdude18:04:26

Maybe the feature.edn idea is the cleanest, similar to data_readers.cljc

borkdude18:04:29

Maybe something like:

nbb_features.edn
[{:name logseq/datascript
  :namespaces [datascript.core]
  :js "./nbb_datascript.js"}
 ...
]

borkdude18:04:46

The macro will find all nbb_features.edn files on the classpath, and then store this in a CLJS runtime map. This map can be queried in nbb.core for feature namespaces and then call load_modules with the right stuff.

borkdude18:04:27

@cldwalker Does that make sense to you?

cldwalker18:04:03

It makes sense but doing this through macros seems more complex than it has to be. We could do this within the bb task itself, generate an edn map and then slurp it in with shadow's https://github.com/thheller/shadow-cljs/blob/650d78f2a7d81f33cb2590e142ddcbcbd756d781/src/main/shadow/resource.clj#L53 during compilation

borkdude18:04:10

Here is an example of how you can detect the nbb_features.edn maps in a macro at compile time:

(enumeration-seq (.getResources (.getContextClassLoader (Thread/currentThread)) "META-INF"))
(replace META-INF with nbb_features.edn). How does moving that to the bb task make it less complex?

cldwalker18:04:08

There are no macros or classpath thinking involved. The bb task just focuses on reading edn and generating data to an edn file

borkdude18:04:45

ok, if we can accomplish the same "no code diff" that way, I'm fine with that too

cldwalker18:04:52

Cool. With any approach, there's still an entry in handle-libspecs that looks in a features map. That's fine, right?

borkdude18:04:05

yes, totally

šŸ‘ 1
borkdude19:04:39

@cldwalker One of the reasons why I was thinking about the classpath is that you could have several libraries floating around which contain extra features for nbb, without adding them to the central repo (for whatever reason, perhaps corporate code for example).

borkdude19:04:39

Then the datascript feature could live in its own dep. Adding it to your classpath would then automatically activate it. Along with a piece of shadow-cljs config which we could add via the shadow clojure API and a clojure function to build nbb

borkdude19:04:30

But I'm fine with proceeding on the current approach

šŸ‘ 1
borkdude20:04:29

@cldwalker About the new commits: ā€¢ you will still have a git difference when you're building with features ā€¢ it seems this will discover all features, not just the ones you've selected in your CI via env vars or so?

cldwalker20:04:52

Yes there would be a git diff during build but it doesn't effect a fork using the original source. I could clean the modified file after release if you'd like

cldwalker20:04:15

Good point on all features. Will fix that

borkdude20:04:17

I think it's cleaner to not have the stub into source control at all

borkdude20:04:33

Just deal with a potentially absent file I'd say

cldwalker20:04:05

Hmm. I'm not seeing any way to do that shadow's API - https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/resource.clj . Any ideas?

borkdude20:04:59

Just don't use shadow's API for this, just use a macro ;). See e.g. nbb.core/version

borkdude20:04:01

I think we could still do the classpath thing in there too, imo it'd be a lot cleaner

borkdude20:04:13

The thing you're doing in the bb task you could move to the macro, read the feature files from the classpath, merge and present the EDN to the CLJS runtime

borkdude20:04:26

no garbage and diffs on the filesystem

cldwalker20:04:58

Hehe. I'll check out the macro. Not keen on moving at all to the macro but I'll timebox it

borkdude20:04:36

I can take that part on me

šŸ‘ 1
borkdude20:04:24

I'll take another look at this tomorrow

borkdude20:04:38

I think for the rest, the PR looks good

borkdude20:04:49

So shall I just merge and give the macro thing a go later? Then you won't be blocked by anything you want to do

borkdude20:04:15

Have you tested building the features in a CI with the features enabled?

borkdude20:04:27

Welcome to nbb v0.3.4!
user=> (require '[datascript.core :as d])
nil
user=> (let [schema {:aka {:db/cardinality :db.cardinality/many}}
      conn   (d/create-conn schema)]
  (d/transact! conn [ { :db/id -1
                        :name  "Maksim"
                        :age   45
                        :aka   ["Max Otto von Stierlitz", "Jack Ryan"] } ])
  (d/q '[ :find  ?n ?a
          :where [?e :aka "Max Otto von Stierlitz"]
                 [?e :name ?n]
                 [?e :age  ?a] ]
       @conn))
#{["Maksim" 45]}
:-D

cldwalker20:04:20

> So shall I just merge and give the macro thing a go later? Then you won't be blocked by anything you want to do Sure! > Have you tested building the features in a CI with the features enabled? That's the next thing I wanted to do. A number of test namespaces run great for me locally

borkdude21:04:03

We could add those tests to the repo too maybe

cldwalker15:04:47

Nice! Was easier than I imagined. Pushed up a branch with datascript tests - https://github.com/logseq-cldwalker/nbb/tree/add-datascript-tests . Passing at https://app.circleci.com/pipelines/github/logseq-cldwalker/nbb/2/workflows/78c34c22-59cf-4445-830c-c12867804c8c/jobs/2?invite=true#step-105-505 . Happy to PR it if you'd like. It optionally runs tests if $NBB_FEATURES is enabled. Eventually I'd like to make it more data driven like we have for library tests in bb but this works for now

borkdude15:04:27

yeah, PR welcome!

āœ… 1