babashka-sci-dev

2022-04-11T18:19:14.128959Z

@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"}}) ?

borkdude 2022-04-11T18:20:39.888439Z

yeah something like that is good

2022-04-11T18:21:03.940959Z

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

borkdude 2022-04-11T18:21:29.757319Z

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

borkdude 2022-04-11T18:21:46.928619Z

but the JS only gets loaded because of a require

borkdude 2022-04-11T18:23:58.775539Z

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

2022-04-11T18:25:10.755259Z

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

borkdude 2022-04-11T18:25:59.238659Z

How would that work?

borkdude 2022-04-11T18:26:13.108519Z

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

2022-04-11T18:27:49.992619Z

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

borkdude 2022-04-11T18:35:27.266879Z

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

borkdude 2022-04-11T18:38:26.054199Z

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

borkdude 2022-04-11T18:43:29.383429Z

Maybe something like:

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

borkdude 2022-04-11T18:44:46.252389Z

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.

borkdude 2022-04-11T18:45:27.954599Z

@cldwalker Does that make sense to you?

2022-04-11T18:51:03.701419Z

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

borkdude 2022-04-11T18:52:10.814059Z

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?

2022-04-11T18:54:08.599699Z

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

borkdude 2022-04-11T18:55:45.420389Z

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

2022-04-11T18:58:52.217029Z

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

borkdude 2022-04-11T18:59:05.760369Z

yes, totally

👍 1
borkdude 2022-04-11T19:18:39.315419Z

@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).

borkdude 2022-04-11T19:19:39.782279Z

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

borkdude 2022-04-11T19:21:30.494869Z

But I'm fine with proceeding on the current approach

👍 1
borkdude 2022-04-11T20:32:29.222189Z

@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?

2022-04-12T15:26:47.531529Z

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

borkdude 2022-04-12T15:51:27.291419Z

yeah, PR welcome!

✅ 1
2022-04-11T20:34:52.881699Z

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

2022-04-11T20:35:15.729929Z

Good point on all features. Will fix that

borkdude 2022-04-11T20:35:17.495749Z

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

borkdude 2022-04-11T20:35:33.679369Z

Just deal with a potentially absent file I'd say

2022-04-11T20:37:05.010649Z

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?

borkdude 2022-04-11T20:37:59.841249Z

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

borkdude 2022-04-11T20:40:01.790809Z

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

borkdude 2022-04-11T20:42:13.059219Z

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

borkdude 2022-04-11T20:42:26.188079Z

no garbage and diffs on the filesystem

2022-04-11T20:45:58.974749Z

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

borkdude 2022-04-11T20:47:36.938499Z

I can take that part on me

👍 1
borkdude 2022-04-11T20:49:24.966379Z

I'll take another look at this tomorrow

borkdude 2022-04-11T20:49:38.215659Z

I think for the rest, the PR looks good

2022-04-11T20:50:21.708069Z

Cool

borkdude 2022-04-11T20:50:49.287339Z

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

borkdude 2022-04-11T20:52:15.097689Z

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

borkdude 2022-04-11T20:58:27.943399Z

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

2022-04-11T20:59:20.681559Z

> 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

borkdude 2022-04-11T21:30:03.749059Z

We could add those tests to the repo too maybe