Modernize eval

[cldwalker]

I've cherry-picked this fork which gives us basic cljc eval (#49) and fixes cljs eval (LightTable/LightTable#1845). Thanks @stephenbrady! I've verified this fix for the latest cljs version 1.7.48 with a mies-om app and clj eval on a cljc file in datascript.

I'd love to merge but this currently breaks LT's eval (Light Table UI) because LT is on an old version of clojure and clojurescript. This branch also forces users to eval in projects with clj >= 1.7 and cljs >= 0.0-3308. Two possible solutions to these issues:

1. After the LT release, we upgrade clojurescript. We release this plugin which is a breaking change but we also provide a X-month window of support for the old version and release patch versions on 0.1.x. This gives users enough time to transition to a modern clj/cljs version.
2. If we can inline clj and cljs deps, we inline the plugin's dependencies #48 despite the startup cost.

I'm inclined to go with whichever path is faster as we risk never getting this in and I think it's important LT continue to be a serious work tool. Not being able to eval in modern cljs projects or cljc files is eroding LT's usefulness for work

@rundis @kenny-evitt @stephenbrady Feedback welcome

[kenny-evitt]

@cldwalker This looks good. [1] definitely seems like it will be faster but [2], or something that solves the same problem, is ideal.

[rundis]

I don't there's any chance of us being able to inline a dependency to clojurescript (or clojure). At least not using mranderson, we'd have to come up with something new. Most other dependencies I think we should be able to inline using mranderson (at a startup cost).

Cider/refactor-nrepl faces the same problem with requiring clojure >= 1.7 / cljs >= 0.0-3308. I think they are heading down the route of supporting an old version (for a limited time period) for users that can't use these versions or higher, whilst all new features (like cljc support) will require users to upgrade (atleast for their development setup).

The introduction of cljc, whilst great, made life quite a bit harder for tooling projects and IDE's (:

I'm inclined to say we might just have to bite the bullet and require users to upgrade their clojure and clojurescript dependencies when moving to the version of Light Table that has upgraded clojurescript support. We might loose some users and generate quite a few issues, but I think not upgrading pretty soon is going to be the end of Light Table for clojure/Clojurescript developers

[stephenbrady]

Thanks for cherry picking those changes. I've been using LightTable daily with those cljc-related changes for a few months now.

With regards to dependency conflicts and inlining, I think the best strategy is to simply reduce the surface area of dependencies. Someone recently pulled out the fs dependency and manually added those functions. I did something similar in my fork. We can reduce the surface area further by removing cheshire and therefore its transitive dependencies and just useclojure.data.json since that's already in the environment via Clojurescript. This commit can be cherry picked for that: https://github.com/fact/Clojure/commit/d74b05cb26c46d5fc2e06439e1efe90778b4c40d

mranderson can be a great tool, but I think it's a bit of a sledgehammer here to solve dependency conflicts. The code in this repo just pulls in too many unnecessary dependencies, like the fs one and cheshire. ibdknox/analyzer looks like another one where we should just copy and paste in the code into this repo. I looked into getting rid of commons-io but think that one is more trouble unfortunately.

To me, what's more worse than breakage is irrelevance. In that vein, I'd just stick with [1].

[kenny-evitt]

What's different about LightTable versus Leiningen itself? How can Leiningen support older versions of Clojure when it depends on 1.7.0?

Leiningen is doing something explicitly:

Project Isolation

When you launch Leiningen, it must start an instance of Clojure to load itself. But this instance must not affect the project that you're building. It may use a different version of Clojure or other dependencies from Leiningen itself, and Leiningen's code should not be visible to the project's functions.

Leiningen currently implements this by launching a sub-process using leiningen.core.eval/eval-in-project. Any code that must execute within the context of the project (AOT compilation, test runs, repls) needs to go through this function. Before the process is launched, the project must be "prepped", which consists of running all the tasks named in the project's :prep-tasks key. This defaults to javac and compile, but defproject or profiles may add additional tasks as necessary. All prep tasks must be cheap to call if nothing has changed since their last invocation.

The sub-process (referred to as the "project JVM") is an entirely new invocation of the java command with its own classpath calculated from functions in the leiningen.core.classpath namespace. It can even use a different version of the JVM from Leiningen if the :java-cmd key is provided. It can only communicate with Leiningen's process via the file system, sockets, and its exit code.

The exception to this rule is when :eval-in-leiningen in project.clj is true, as is commonly used for Leiningen plugins. Since Leiningen plugins are intended to be used inside Leiningen itself, there's no need to enforce this isolation.

[cldwalker]

@stephenbrady You'll be happy to know that cheshire, fs and our forked analyzer have been removed in the latest plugin release.

I agree with the prevailing sentiment that we'll need to be pragmatic and thus may end up choosing choice 1. The clojurescript upgrade blocking this issue is being tracked in LightTable/LightTable#1973. Until this PR lands, I will be keeping this branch up to date to switch to it for work

[cldwalker]

I've actually found a way we can release this now! :) Since the runner knows the project and clojure version it's connecting to, we can switch between the new lein-light-nrepl middleware and the old as needed. This means that we'll have two separate middlewares to maintain but I'm ok with supporting the old one for 2-3 months. After that, people are welcome to stay on older clj/cljs versions but we won't be backporting fixes to the old middleware. I've also made clojure 1.7 a requirement for cljs "0.0-2341" or greater as that is when our old middleware starts to break on cljs eval.

Can I get two of you to QA this on a couple of different projects, recent clojure +clojurescript especially? Once I get thumbs up, I'll add runner tests and relevant docs and merge

[cldwalker]

I forgot to mention to QA, you need to cd lein-light-nrepl && lein install to have the new middleware. I've already QAed a fair amount but it's always helpful to see if anyone uncovers anything with different QA cases.

[stephenbrady]

Clever.

FYI, there's still the reliance on the fs dependency lingering in the runner. See here: https://github.com/LightTable/Clojure/blob/modernize-eval/runner/src/leiningen/light_nrepl.clj#L82-L83

Probably best to just to copy in the appropriate functions from earlier: https://github.com/LightTable/Clojure/blob/modernize-eval/lein-light-nrepl/src/lighttable/nrepl/fs.clj

It's less problematic since it's on the lein side of things, but in my experience, fs is a real source of conflicts with other third-party libs.

[rundis]

First try not so successful:

Steps taken:

1. Fetched and checked out branch modernize-eval
2. In lein install for the nrepl middleware
3. lein uberjar for the runner and copied uberjar to my deploy/plugins/Clojure/runner/target folder (since that's apparently where my electron dev version prefers to resolve plugin-dir)

(lein uberjar gives two warnings btw; WARNING: cat already refers to: #'clojure.core/cat in namespace: net.cgrand.parsley.fold, being replaced by: #'net.cgrand.parsley.fold/cat and Warning: skipped duplicate file: project.clj

Trying to connect to guestbook works okay But connecting to this sample project: https://github.com/rundis/table-stacking-fun ...

Gives me: WARNING: cat already refers to: #'clojure.core/cat in namespace: net.cgrand.parsley.fold, being replaced by: #'net.cgrand.parsley.fold/cat Light Table requires Clojure Version >= 1.7.0 for ClojureScript versions >= 0.0-2341 Which is cool except for the annoying warning

Upping clojure to 1.7 gives me this though on first eval (i.e connects ok, but eval fails): Failed trying to require stacking-fun.web with: clojure.lang.ArityException: Wrong number of args (2) passed to: StringReader AFn.java:429 clojure.lang.AFn.throwArity AFn.java:36 clojure.lang.AFn.invoke cfg.clj:163 instaparse.cfg/eval7429[fn] ...

Subsequent evals gives me: Failed trying to require stacking-fun.web with: java.lang.Exception: namespace 'instaparse.abnf' not found core.clj:5716 clojure.core/load-lib ...

Some dependency fun stuff going on I suppose. (BTW uncommented pretty much everything I have in ~/.lein/profiles.clj)

[cldwalker]

I've added tests and relevant docs. @rundis I've just resolved the #'net.cgrand.parsley.fold/cat warning by upgrading lein on runner. Do you still get the failure if you copy the uberjar that comes with this branch and mv ~/.lein/profiles.clj somewhere else?

[rundis]

Warning gone ! The problem seemed to be that compojure 1.3.1 depends on instaparse 1.3.4 which again wasn't particularly compat with Clojure 1.7. So upping compojure to 1.4.0 go me going on clojure eval :-)

So now I'm good to start testing it at least.

[rundis]

cljs-eval; Regardless of what clojurescript version I add to a sample project, *clojurescript-version* always reports 0.0-3208. Can't wrap my head around why though.

[cldwalker]

@rundis If I try that project with to clj 1.7.0 and compojure 1.4.0, I'm able to eventually see different *clojurescript-version* values: 0.0-3211 and 1.7.48. I say eventually because I did have to eval the dependant namespaces e.g. fixed-data-table.components (clj) and then fixed-data-table.components (cljs). It seems we don't pick up macro definitions for this project. While this is a bug, I don't think it's big enough to block releasing this

As for not being able to see different cljs versions, did you lein clean && lein cljsbuild once dev when changing cljs versions? If this doesn't work, please put up a branch of what you are using and describe where you are evaling. You should also try using this branch on lein new mies-om om-test as I've verified that works and it would be useful to confirm you can see basic cljs projects working.

Also, I also noticed that when running your app in advanced optimized mode: I came across the exception Uncaught ReferenceError: cljs is not defined coming from browserInjection.js. This makes sense looking at that implementation and I'm going to update docs to mention :advanced mode isn't supported currently

[rundis]

Ah... macros :-) Got it working, also with figwheel on my sample project. *clojurescript-version* now making sense. Tried om-mies with a couple of clojurescript versions, evaling a few 1.7 features too. Sweet !

[cldwalker]

@rundis Great! @stephenbrady We only have lein and fs as deps so I'm not worried about conflicts in runner yet. More importantly fs is in a separate process and won't have any effect in a user's process. Could you give this branch a try on your work project? If I don't hear back in a few days I'll assume this is good to go and merge