nextjournal / clerk

⚡️ Moldable Live Programming for Clojure
https://clerk.vision
ISC License
1.83k stars 78 forks source link

ClojureScript namespaces missing from required deps in .cljs files loaded via `:require-cljs` #718

Open formsandlines opened 1 month ago

formsandlines commented 1 month ago

Since the most recent version it is possible to load .cljs files from Clerk viewers using :require-cljs true. This works fine, but when I try to require a library in such a file (added in my deps.edn), several namespaces normally included in ClojureScript are reported missing: "Could not find source for CLJS namespace x"

When testing it with a library of my own[1], the following namespaces are missing:

In our discussion on Clojurians Slack yesterday, @mk added clojure.math: https://github.com/nextjournal/clerk/commit/319c00221862ee3fb975a3b75bdc1d6b5d9e0754 which worked fine when I tested it. Maybe the other namespaces can also be added to increase compatibility?

Here is a test repo with my library as a dependency to reproduce the issue: https://github.com/formsandlines/clerk-cljs-test If all namespaces are included, the test-viewer in notebooks/index.clj using the render-test function from src/render.cljs should display :I without any errors.


[1]: https://clojars.org/eu.formsandlines/formform (works both in clj and cljs) - I would like to render some visualizations I created as web components that need functions from that lib, so I hope to get it working there

mk commented 1 month ago

Think we should check the impact on bundle size for things we're adding. @borkdude could you take a look at this in the coming days?

borkdude commented 1 month ago

@mk Sure!

borkdude commented 1 month ago
 jar | cljs/pprint.cljs                                             |     93,5 KB |   2,98 % |    514,2 KB |    128,1 KB |

Exposed that explicitly now in the PR https://github.com/nextjournal/clerk/pull/723

Shall I continue with the other namespaces as separate PRs?

borkdude commented 1 month ago

@formsandlines I made a lot of progress incorporating most stuff, including spec.

You can check it out in branch more-namespaces-ctd of the clerk repo. You can try commit 61a2dbe49ae1ab08c29326564d9b2783edffdd87 with your notebook. Right now I'm seeing:

Screenshot 2024-10-22 at 18 23 01

About the first two messages:

The third issue I don't know yet, perhaps this just happens because of the first two issues.

formsandlines commented 1 month ago

@borkdude That is good to hear!

I forgot that the version of formform on Clojars is quite outdated and may not work as well with ClojureScript - updated that now in the repo and quickly tested it on shadow-cljs to make sure it works. However, the errors I see after trying your new commit remain the same as the ones in your screenshot.

Since this is not the place to fix issues with my lib, I can also post this on Clojurians Slack to discuss it there if you see no relevance to Clerk or sci here.

I only ever used with-meta on a return value, not on a var and never had any problems with this in JVM Clojure. Oddly, in the consts->quaternary function mentioned from the error I didn’t use with-meta at all, so I wonder where this is coming from.

From the call-stack, it seems like this has something to do with my function specs, but I only defined these on API-functions and this is not one of them. Is there an issue with function specs in sci? Other than that, the function itself is very simple and I can’t see where metadata would be added to the var: https://github.com/formsandlines/formform/blob/0caa931ae42e6fd87b6917e88ceab49cb21174c8/src/formform/calc/core.cljc#L57

As for the spec-macros, I have seen that reagent.core got included in sci for Clerk, which also uses :require-macros. Is it just the :refer part that doesn’t work in sci or is the problem actually using the macros in the cljs source code? Would clojure.core.match also not work in sci, since it is a macro?

borkdude commented 1 month ago

Apparently I'm handling :require-macros in nbb differently than in SCI:

https://github.com/babashka/nbb/blob/6041d27d814a026dcf767b4a9f01cac01eff8786/src/nbb/core.cljs#L195-L197

Let me fix that tomorrow in SCI, then at least the first issue will disappear

borkdude commented 1 month ago

The :require-macros thing already worked, I'm just running into some error with vars not being recognized as vars:

:var? #'clojure.core/int? false sci.lang/Var

Once I fix that, I'll report back.

borkdude commented 1 month ago

Works now after fixing a var? check in the SCI spec alpha configuration:

Screenshot 2024-10-23 at 12 04 19

Pushed to branch in commit 82574a97077b00d53c398a4510f4aba946ac913d, you should be able to run that locally.

borkdude commented 1 month ago

Another version: bd701395ce4686f8aa95eda0e29c040cfdae3c0c

borkdude commented 1 month ago

Additional bundle size for cljs.spec:

|     | sci/configs/cljs/spec/alpha.cljs                             |     69,3 KB |   2,12 % |    352,7 KB |     23,3 KB |
| jar | cljs/spec/alpha.cljs                                         |     47,9 KB |   1,46 % |    261,9 KB |     54,1 KB |

On main, spec.alpha was already loaded thus contributed somewhat to the bundle size already:

| jar | cljs/spec/alpha.cljs                                         |     14,6 KB |   0,46 % |    261,9 KB |     54,1 KB |
borkdude commented 1 month ago

Made a clean PR here now:

commit: 36de8d94e73a1a0ab7cff7013c0568734355308c

https://github.com/nextjournal/clerk/pull/725

formsandlines commented 1 month ago

Awesome, thanks! It also worked in my test with other formform.calc functions.

I guess this issue can be closed now. From my experiments, I still found some missing namespaces, but this is quite the hydra, so I just throw a list in here if you want to investigate (no need to do it right now or at all, if it doesn’t align with Clerks goals to support everything in cljs):

borkdude commented 1 month ago

I guess this issue can be closed now.

once the PR is merged I guess, that's up for @mk to decide, if that aligns with clerk's goal :)

The original goal of :require-cljs wasn't really to support loading existing libraries from the CLJS ecosystem, but it's a nice side effect that this kind of works for SCI+config-compatible libraries