bensu / doo

doo is a library and lein plugin to run cljs.test on different js environments.
Eclipse Public License 1.0
324 stars 63 forks source link

Support Lein 2.7.0 :managed-dependencies #145

Closed MatthewDarling closed 7 years ago

MatthewDarling commented 7 years ago

Current behaviour

As of version 2.7.0, Leiningen supports Maven's managed dependencies. When managed dependencies are used, version numbers in the :dependencies vector can be nil or left off entirely. If a dependency without a version is found, a separate :managed-dependencies key has to provide a version instead.

Here's what a very simple project using this would look like:

(defproject foo "0.0.1-SNAPSHOT"
  :dependencies [[org.clojure/clojure]]
  :managed-dependencies [[org.clojure/clojure "1.8.0"]])

Crashing with doo

Because make-subproject uses select-keys and doesn't keep the :managed-dependencies key, these dependencies can't resolve and you get a crash like this:

$ lein doo phantom test once
java.lang.IllegalArgumentException: Provided artifact is missing a version: [cljs-ajax/cljs-ajax nil]
... imagine another 100 lines of stacktrace ...

Fixing it, or choosing which fix to apply

The easy fix is to select the :managed-dependencies key as well.

That's what lein-figwheel does now.

lein-cljsbuild also hit this bug, but the fix there was quite different. Compare its implementation of make-subproject.

Looking at the different versions and their git blame, it seems like make-subproject was first part of lein-cljsbuild, then copied over to lein-figwheel. So I'm tempted to consider lein-cljsbuild as the primary "upstream" for this function. And that version only changes :prep-tasks.

A possible different fix

If we're willing to make a more drastic change, I think we could simplifymake-subproject. I don't think doo needs to change :prep-tasks like cljsbuild. So:

(defn make-subproject [project]
  (with-meta
    (assoc project :local-repo-classpath true)
    (meta project)))

Possibly with a docstring that explains it's a simplified version of what lein-cljsbuild has.

And to be honest, I don't even know if :local-repo-classpath is needed - there's no mention of it in Leiningen now, and the only commits for it are from 2011. It's possible it's needed to support very old Leiningen versions...?

Testing

I ran lein test in the plugin folder and everything passed.

Both the currently committed fix and the simplified version avoid the crash on our project.

MatthewDarling commented 7 years ago

Oops, I just noticed there's an existing PR in #143 for the same thing. And that one adds an extra test. Which makes it definitely better :)

I'll mention the possible simplification over there, instead.