Improve CLJS dev setup and testing org

Question

Improve CLJS dev setup and testing org

Opened this issue 6 years ago · 12 comments

The CLJS dev setup is less than ideal. I'd like to get it working with a more modern figwheel/sidecar sort of setup.

Also, the cljs testing seems to be disorganized. I think it is not very clear where cljs & cljc files are found at. I'm not even sure it makes sense to have a "common", "clojurescript", and "clojure" directories under "src/test" anymore since read conditionals and the various file extensions are respected now. It makes the sharing of files more confusing and hard to find in my opinion.

I only bring up the testing setup since there seem to be issues getting Figwheel compiling these namespace and to get it all working again, I'd likely want to get this stuff sorted out.

Answer 1 · 2018-03-14T16:40:54.000Z

+1 to doing this. I haven't had the time or familiarity/more time to become familiar myself but I agree that the CLJS setup could use improvement and from what I read it sounds like the overall CLJS dev ecosystem has come a long way since Clara's CLJS build was set up.

Answer 2 · 2018-03-15T14:53:37.000Z

You might take a look at shadow-cljs as well. We went from boot to figwheel and finally settled on shadow-cljs, have been very happy with it.

Answer 3 · 2018-03-15T15:14:45.000Z

@sparkofreason

I’ve been following shadow cljs a bit. I agree it seems useful in some cases. For Clara though there really isn’t any fancy JS interop dependency management to worry about. So I planned to just do a pretty straightforward lein figwheel setup for now. The biggest problem right now is some of these defsession sort of cljs namespaces aren’t compiling with figwheel.

Also, Clara supports a quite old cljs version. For development time, I think I’m going to use a more recent one for dev tooling sake. We can still run tests against whatever old version we want to support.

I am going to look closer at the figwheel compilation problem stuff and try to figure out what to do about our defsession Usage once I understand it a bit more.

Answer 4 · 2018-03-15T15:31:47.000Z

Here's the leiningen/ figwheel config I borrowed from Luminus that I posted a while back: #285 (comment).

I still use this for CLJS REPL with figwheel. There may be better alternatives these days but it works for me.

Answer 5 · 2018-03-16T17:40:48.000Z

Thanks @alex-dixon I'll check it out. I'm pretty familiar with cljs and figwheel stuff nowadays since I've been using it at work for a while now. It's good to see what you have though.

There are some deeper issues in the cljs (test) setup in Clara right now where I think we are abusing the cljs compiler and "getting lucky" to have the build pass.
Some details are in #382 (comment)
but I plan to do more investigation and follow-up on this, likely when I get a PR working as I want for this issue.

Answer 6 · 2019-04-04T15:47:14.000Z

In doing this work, I think we should make the project compatible with JDK11 since it is being used more often now.

The WIP branch is https://github.com/mrrodriguez/clara-rules/tree/iss388-cljs-project

https://www.deps.co/blog/how-to-upgrade-clojure-projects-to-use-java-11/
is a good guide to this end.
Our problems right now are primarily only in the cljs-side, so I think it relates to this issue. I'm looking into this now.

https://stackoverflow.com/a/52502208/924604
mentions how in jdk11 the "--add-modules=java.xml.bind" will no longer work, since the module doesn't exist in the jdk distribution at all, it's a separate lib jar. However, we don't need this in newer cljs versions from this change:
https://dev.clojure.org/jira/browse/CLJS-2377

from cljs version 1.10.238

I am planning to at least make a jdk11 profile that uses newer versions of clj and cljs and doesn't have this issue for those wishing to build and run that way.

This means clj 1.10 as well for cljs 1.10.x

Answer 7 · 2019-04-08T17:58:35.000Z

Adding a note on some current progress:

When using JDK8 and bump to versions:

[org.clojure/clojure "1.10.0"]
[org.clojure/clojurescript "1.10.520"]

then lein test from the current state of master
it fails in clara.test-complex-negation ns due to a malformed throw expression. This one is easy to fix, it is a typo and should have wrapped an ex-info call around the args.

Fixing this, then it passes the tests, but fails the subprocess with:

Running ClojureScript test: phantom-simple
SyntaxError: Invalid regular expression: range out of order in character class

  file:///my/path/to/clara-rules/target/js/simple.js:5640 in RegExp
TypeError: undefined is not an object (evaluating 'clara.test.run')

  undefined:2
  :3

I’ll look at this some, just noting it here for now.

Answer 8 · 2019-04-10T14:08:08.000Z

Update:

I addressed the issue of #388 (comment)

The html files used for the test runners did not have the

  <meta charset="utf-8">

This was causing UTF-8 encoded string parsing problems in newer compiled JS that was coming from Google Closure lib, that is bundled in automatically by CLJS. More recent versions of CLJS take more recent versions of Google Closure. So at some point, this problem was introduced. However, our html files should have this meta tag anyways to be well-behaved.

The last thing I'm working on is getting figwheel running without problems in the ways that is most common to use it with. I'm hitting some old issues I fought with before about how defsession is compiled and cannot resolve symbols during figwheel's cljs compilation. So once I have that sorted out, I will have a PR for this issue.

Answer 9 · 2019-05-06T19:37:00.000Z

I've looked a bit more into this recently and created this gist
https://gist.github.com/mrrodriguez/dc48281196f99e30ee84c56c0ec6f63a#file-foo-cljs

to demonstrate a problem with trying to use clojure.core/eval during CLJS macroexpansion. After discussing this more in the Clojurians Slack channel #clojurescript , I've been convinced that it is fundamentally flawed for clara.macros/defsession to attempt to perform a clojure.core/eval on arbitrary forms the way it currently is doing it. clojure.core/eval operates with no built-in "awareness" of the CLJS compilation cycle. This includes it not necessarily being aware of vars being defed during CLJS compilation, etc. This was confirmed to me by David Nolen [1] in the Slack channel.

There is some history behind why defsession is the way that it is now, but I think some things may have just "happened to work" somehow in the past. They are not reliable however. I immediately run into trouble when working with this in different, modern CLJS dev environments, such as Figwheel. However, the process isn't stable anyways.

I'm going to explore changing this, but it'll require a few things to consider:
(1) defsession is expected to support namespace symbols, as well as collections of symbols referencing production structures. These need to be resolved fully to productions at macroexpansion time, in a way that doesn't rely on clojure.core/eval.
(2) defsession was changed in #94 that was meant to allow arbitrary forms to be evaluated at macroexpansion time to production structures within defsession. This relies on the use of clojure.core/eval and the clj environment happening to be able to eval the forms given.

(1) can probably be addressed by explicitly removing quoted symbols - since we support them for backward compatibility & also resolving symbols via CLJS analyzer functions.

(2) is more difficult. Perhaps it can still fall back to eval if the CLJS-side symbols are already resolved and removed from consideration. However, this will still lead to brittle calls to eval.

From the referenced #94 issue

It is possible to set up a server which can compile and serve up new
rule-containing JavaScript files on demand, generating sets of rules
dynamically from (for example) a database.

It isn't even clear what this means. defsession is a compile-time macro, I don't see how it facilitates on-demand rule generation via a service. If that is being done, I'm guessing it is via abusing eval some more to expand the macro at runtime. I don't think we should tailor to that case. If we want on-demand CLJS session assembly, we can expose some of the underlying parts of defsession as functions.

[1] David Nolen conversation snippets regarding CLJS macroexpansion-time eval:

Dnolen:

clojure.core/eval is exactly that
has nothing to do with ClojureScript, doesn't know anything about ClojureScript
if you need it, expect to do a lot of work

Me:

sure, I know it’s a stretch, but was curious if there are anyways to try to “do things” during macroexpansion that may involve eval-like behavior

Dnolen:

there isn't
it's just Clojure eval
there's no way to magically make it know about ClojureScript stuff
it doesn't know about nses, vars, nothing
if anything [you have done before] worked it was probably simple and you got lucky
yeah trying to make it do something useful would make my brain hurt

Answer 10 · 2020-05-08T18:21:46.000Z

Picking up work on this again:
https://github.com/mrrodriguez/clara-rules/tree/iss388-cljs-project-new

NOTE: this is work in progress - still broken in many ways.

Notes on the changes proposed

My plan is going to be aggressive - it’s the only way I see to get the CLJS build aligned w/the future. Here are the steps:

Upgrading some deps, including clj min version - surveys report huge majority well past 1.7 (edited)
Move all cljs build issues into profiles - won’t affect clj build at all
Upgrade cljs testing to use something more modern - like jsdom integration with figwheel
I might use figwheel-main if it’s more well suited for modern cljs testing than lein-figwheel - this isn’t really a big deal beyond setup
I’m going to make breaking changes to cljs compilation model - there is no other way - it’s done in an outdated way that isn’t reliable and will not work on future cljs versions most likely (or already).
specifically, nothing about mk-session etc, can use clojure.core/eval - that is completely off the table for proper cljs compilation. this means the arguments to make a cljs session will have to be non-dynamic. like symbols representing ns’s, rule struct vars, or rule struct literals themselves.
I will bump clara-rules to 1.x after with some increased clj/cljs min versions.

Answer 11 · 2020-07-21T21:52:43.000Z

I've started looking at that branch @mrrodriguez . I noticed that the defrule macro isn't returning quoted code in ClojureScript:

#?(:clj
  (defmacro defrule
    "Defines a rule and stores it in the given var. For instance, a simple rule would look like this:
    (defrule hvac-approval
      \"HVAC repairs need the appropriate paperwork, so insert
        a validation error if approval is not present.\"
      [WorkOrder (= type :hvac)]
      [:not [ApprovalForm (= formname \"27B-6\")]]
      =>
      (insert! (->ValidationError
                :approval
                \"HVAC repairs must include a 27B-6 form.\")))
See the [rule authoring documentation](http://www.clara-rules.org/docs/rules/) for details."
    [name & body]
    (let [doc (if (string? (first body)) (first body) nil)
          def-name (vary-meta name assoc :rule true :doc doc)
          rule (dsl/build-rule name body (meta &form))]
      (when (platform/compiling-cljs?)
        (def def-name rule))
      `(def ~def-name ~rule))))

As compared to the current master:

#?(:clj
  (defmacro defrule
    "Defines a rule and stores it in the given var. For instance, a simple rule would look like this:
    (defrule hvac-approval
      \"HVAC repairs need the appropriate paperwork, so insert
        a validation error if approval is not present.\"
      [WorkOrder (= type :hvac)]
      [:not [ApprovalForm (= formname \"27B-6\")]]
      =>
      (insert! (->ValidationError
                :approval
                \"HVAC repairs must include a 27B-6 form.\")))
See the [rule authoring documentation](http://www.clara-rules.org/docs/rules/) for details."
    [name & body]
    (if (com/compiling-cljs?)
      `(clara.macros/defrule ~name ~@body)
      (let [doc (if (string? (first body)) (first body) nil)]
        `(def ~(vary-meta name assoc :rule true :doc doc)
           ~(dsl/build-rule name body (meta &form)))))))

Defquery has similar changes. This will create definitions in the Clojure namespace of the compiling process rather than in ClojureScript correct? I don't understand why we'd want this. I suppose namespaces could be used as a state store along the lines of what add-production is doing but I don't understand why it would be preferable.

Answer 12 · 2020-07-21T22:12:32.000Z

@WilliamParker the branch is certainly not complete but these changes are intentional. Clara is actually making a really tricky usage of cljs in general and it was exploiting eval in a unsafe way to do some things previously. What we need is the defsession macro to emit a form that either has data structures literally inlined or function and vars that are resolvable on cljs.

Claras own compiler ns is not compatible with cljs right now. So all usage of it must be done by time macroexpansion finishes.

References to rules ends up needing to be made during compilation. This means the vars need resolved. These are still in clj at this point. However I’m still playing with this in general. We do actually need to add the rule definitions to the cljs analyzer info since during macroexpansion in cljs it’ll need it for symbol resolution. Not clj ns objects. There may be little value having them defined as clj side vars so this is one part it may make sense to go back to putting them in the analyzer compilation data similar to before. I actually confirmed to some extent that this is a stable trustworthy part of the analyzing path API. I just haven’t had time to tweak this yet.