Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
formsandlines opened this issue Oct 20, 2024 · 13 comments
Assignees

Comments

@formsandlines
Copy link

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:

  • cljs.core
  • clojure.math
  • clojure.pprint
  • clojure.spec.alpha
  • clojure.spec.gen.alpha
  • goog (yeah, just "goog")
  • goog.math.Long

In our discussion on Clojurians Slack yesterday, @mk added clojure.math: 319c002 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
Copy link
Member

mk commented Oct 21, 2024

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
Copy link
Collaborator

@mk Sure!

@borkdude
Copy link
Collaborator

borkdude commented Oct 21, 2024

  • cljs.pprint adds about half a megabyte (yes, it's a pretty wild dependency, but optimized only 93,5kb) but it was already part of the bundle.
 jar | cljs/pprint.cljs                                             |     93,5 KB |   2,98 % |    514,2 KB |    128,1 KB |

Exposed that explicitly now in the PR #723

Shall I continue with the other namespaces as separate PRs?

@borkdude
Copy link
Collaborator

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

  • with-meta on a var doesn't work (currently) in SCI since with-meta usually returns a new (immutable) value. In CLJS this works, but it mutates the var, which I think isn't good behavior. In JVM Clojure this doesn't work at all. alter-meta! does work in SCI on vars.
  • SCI works differently with respect to :require-macros (since everything runs in JS anyway).
    Thus (ns foo (:require-macros [foo :refer [dude]])) is kinda undefined behavior.

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

@formsandlines
Copy link
Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

Another version: bd701395ce4686f8aa95eda0e29c040cfdae3c0c

@borkdude
Copy link
Collaborator

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 added a commit that referenced this issue Oct 23, 2024
@borkdude
Copy link
Collaborator

Made a clean PR here now:

commit: 36de8d94e73a1a0ab7cff7013c0568734355308c

#725

@formsandlines
Copy link
Author

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):

  • cljs.core.match (made use of this in formform.expr)
  • goog.i18n.uChar (instaparse needs this)
  • cljs.reader (com.lambdaisland/garden needs this)
  • goog (me.raystubbs/zero, a nice web-components library, needs this, apparently it is just goog)
  • goog.math.Long (seems like math.combinatorics 0.3.0 needs this, I had 0.1.6 in my library where it was fine without)

@borkdude
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants